Are downstream projects meant to turn off IGRAPH_WARNINGS_AS_ERRORS themselves?

I’m pulling down igraph as a dependency in one of my CMake projects (with FetchContent, though that’s not really relevant) and compiling it with clang 13.0.0, and I’m getting errors such as:

[  5%] Building C object _deps/igraph-build/vendor/cs/CMakeFiles/cxsparse_vendored.dir/cs_qr.c.o
/home/luna/Code/JSscran/wasm/build/_deps/igraph-src/vendor/cs/cs_qr.c:7:21: error: variable 'm' set but not used [-Werror,-Wunused-but-set-variable]
    CS_INT i, k, p, m, n, vnz, p1, top, m2, len, col, rnz, *s, *leftmost, *Ap, *Ai,
                    ^
1 error generated.

Inspection of cs_qr.c in the current igraph master indicates that, indeed, m is set and assigned to but never used. There’s a few similar warnings-promoted-to-errors elsewhere in the code base that I won’t list here for the sake of brevity. I eventually worked around this by adding this to my CMakeLists:

set(IGRAPH_WARNINGS_AS_ERRORS OFF CACHE BOOL "" FORCE)

It seems that IGRAPH_WARNINGS_AS_ERRORS should default to OFF in the release tarball (I’m using the 0.9.4 release) to avoid these build problems. However, if downstream projects expected to set this themselves, then it should be listed in the installation instructions somewhere.

I would say that handling this is still under discussion on our side, so any input is welcome.

For future reference, as of today Clang 13 is not yet released. Nevertheless, I will give this a try with the version of Clang 13 I do have access to (may not be the same as your version). Please let me know what system you are using exactly (what Linux distro of macOS version), and where you got this prerelease version of Clang from.

Out of curiosity, what is your project? It helps us to be aware of downstream users, so we can better address their needs.

This is a recurrent topic :slight_smile: We try to keep igraph’s source code clean to a point when there are no warnings emitted by the compiler during compilation so we can easily spot any new warnings that are accidentally introduced when modifying the code. That’s why -Werror is on by default (it is all too easy to hastily run mkdir build && cd build && cmake .. and lose the benefits of -Werror if it is not on by default). On the other hand, maintainers of downstream packages may find this annoying and/or they could be working with a compiler that emits more warnings than the ones we use, so we provide an option to turn this off.

The particular warning that you see comes from a vendored library (CXSparse), and, incidentally, I have started receiving warnings from OSS-Fuzz about our build failing there due to the very same warning. It started a few days ago so I strongly suspect that Clang new started flagging something with a warning. We try to refrain from modifying vendored source code and prefer to turn off warnings explicitly for certain vendored dependencies instead. I tried to add a patch to prevent the warning, but I had to use -Wno-unused-variable instead of -Wno-unused-but-set-variable because older versions of Clang do not understand -Wno-unused-but-set-variable; let’s hope that it will be enough.

Apparently, this is a hot topic not only for us but also for far larger projects (e.g., the Linux kernel itself). I’m open to suggestions about how to make this more convenient to everyone, but at the moment I’m still leaning towards enabling -Werror by default as this particular breakage 1) was caused by a toolchain that newly introduced a warning (i.e. we could not have anticipated the warning in advance), 2) is easy to work around in the downstream project, 3) will be fixed in the next patch release on igraph’s side as well. Constructive ideas are welcome. One option would be to turn off -Werror by default but then turn it on in all CI tests; the downside would be that sometimes the builds would pass locally on a dev’s machine but fail in CI, which leads to a longer iteration time.

Thanks @szhorvat, @tamas.

I’m using the Emscripten toolchain to compile igraph’s C core to Webassembly, so that I can run igraph’s methods in browser applications. It works pretty well; if you want to know more, we have a functional app at https://www.jkanche.com/scran.js/app/app/ that uses igraph’s multi-level community detection functionality for identifying clusters in single-cell RNA sequencing data.

Incidentally, this is where I’m getting the pre-release Clang; it’s bundled with the later versions of Emscripten.

I’ll add that point 2 can be made even easier if the installation instructions could mention that the IGRAPH_WARNINGS_AS_ERRORS option exists, and also provide some code to turn it off. I recall having to dig around in the CMakeLists to figure out what to do here.

I’ll add that point 2 can be made even easier if the installation instructions could mention that the IGRAPH_WARNINGS_AS_ERRORS option exists, and also provide some code to turn it off.

That’s a good idea; there are tons of options in CMakeLists.txt that can be used to tweak the build, and as far as I know they are not documented anywhere. I’ll add an issue for it in the issue tracker.

1 Like

Discussion continues on the issue tracker here.