makefile: workaround boost errors under latest compilers

[SteVwonder]

Problem: when using the latest gcc/clang compilers, older boost versions
create multiple warnings to be emitted. Our use of -Werror turns
these warnings in boost into hard errors, despite being in a third-party
dependency.

Solution: use -isystem rather than -I for the boost cppflags to
denote that the boost headers are system-provided headers, which turns
off most static analysis warnings for those headers.

Related Documentation:

• Clang: http://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-in-system-headers
• GCC: https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html

Closes #300
Closes #747
Closes #732

[trws]

This is how any system library like that should come in most likely, makes perfect sense.

[SteVwonder]

It looks like this workaround breaks when BOOST_PREFIX=/usr, as per this GCC bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70129. Which also explains why it works just fine under Spack, since the boost prefix will always be non-standard under Spack.

The three potential paths forward that I see are:

• Check if BOOST_PREFIX is one of the default system search paths, and if it is, still use -I. Unfortunately, I think this means the warnings will only be silenced if boost isn't in a default system location
• Check if BOOST_PREFIX is one of the default system search paths, and if it is, set BOOST_CPPFLAGS="" and rely on the compiler picking up the header from one of the default paths
• Insert compiler-specific pragmas to turn off warnings for certain includes:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#pragma GCC diagnostic ignored "-Wunused-local-typedefs"
#include <third-party-library/header.h>
#pragma GCC diagnostic pop

[trws]

Default system paths are included by the `-isystem` method unless otherwise specified. If they are not explicitly added to a -I the warnings should be supppressed just by nature of coming from those places. Note the test lower down in the bug where no listing and -I behave the same.

…

Sent from my iPad On Sep 30, 2020, at 2:18 PM, Stephen Herbein <[email protected]> wrote:  It looks like this workaround breaks when BOOST_PREFIX=/usr, as per this GCC bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70129 The two paths forward that I see are: * Check if BOOST_PREFIX is one of the default system search paths, and if it is, still use -I. Unfortunately, I think this means the warnings will only be silenced if boost isn't in a default system location * Check if BOOST_PREFIX is one of the default system search paths, and if it is, set BOOST_CPPFLAGS="" and rely on the compiler picking up the header from one of the default paths * Insert compiler-specific pragmas to turn off warnings for certain includes: #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" #pragma GCC diagnostic ignored "-Wunused-local-typedefs" #include <third-party-library/header.h> #pragma GCC diagnostic pop — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub<#757 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAFBFNI5X5H2HXSMZDTFIJDSIOOADANCNFSM4R7RCFRA>.

[SteVwonder]

Default system paths are included by the -isystem method unless otherwise specified. If they are not explicitly added to a -I the warnings should be supppressed just by nature of coming from those places.

Ok. IIUC it sounds like Check if BOOST_PREFIX is one of the default system search paths, and if it is, set BOOST_CPPFLAGS="" and rely on the compiler picking up the header from one of the default paths is the way to go. That also seems to be the way that CMake is handling this: https://gitlab.kitware.com/cmake/cmake/-/issues/16291

Any objections to just checking for /usr/include rather than going down the rabbit hole of querying each possible compiler for the full list of their default locations with a set of magic runes that is specific to each compiler? GCC's magic runes, for example. According to the filed issues, it looks like /usr/include covers 99% of cases and the 1% edge cases are mostly cross-compilation.

[SteVwonder]

That latest change seems to have done the trick. Works on both the Travis tests and under Spack. @trws , can you give the new couple of lines at the end of the boost.m4 a quick look?

[milroy]

LGTM! This builds correctly on my test system. I did ask one question, but I'm almost sure these changes can be merged. Thanks!

[milroy]

Is is possible that $BOOST_CPPFLAGS will resolve to something like -isystem __ /usr/include (where __ indicates multiple whitsepaces) or -isystem /usr/include/? I don't see that it's defined elsewhere in flux-sched (and based on the file here these cases won't occur), but I want to make sure.

[SteVwonder]

That's a good question.

I don't see that it's defined elsewhere in flux-sched (and based on the file here these cases won't occur), but I want to make sure.

That was the conclusion of my analysis as well. The only other case I could think of was the user manually specifying the variable at the configuration line BOOST_CPPFLAGS=/usr/include/ ./configure, but I don't think we can really protect against the user shooting themselves in the foot in that scenario.

[SteVwonder]

Thanks @milroy and @trws! Setting the label so that we can tag 0.12.0 with this fix included.

[codecov-commenter]

Codecov Report

Merging #757 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #757      +/-   ##
==========================================
+ Coverage   75.94%   75.95%   +0.01%     
==========================================
  Files          82       82              
  Lines        9220     9220              
==========================================
+ Hits         7002     7003       +1     
+ Misses       2218     2217       -1     

Impacted Files	Coverage Δ
qmanager/modules/qmanager_callbacks.cpp	78.20% <0.00%> (+0.64%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9031326...b7d7e19. Read the comment docs.