build: add --enable-isystem and change --enable-werror to enable -Werror

vasild commented at 7:53 pm on February 14, 2020: member

Before this patch ./configure --enable-werror would selectively turn only some warnings into errors, not all, by using -Werror=foo -Werror=bar instead of a plain -Werror which turns all warnings into errors.

To accommodate the new stricter build config, introduce --enable-isystem to optionally suppress warnings from external headers (e.g. boost, qt). That may be useful on systems that have them installed outside of /usr/include. On Linux the external headers are typically installed in /usr/include and so warnings from them are always suppressed.

DrahtBot added the label Build system on Feb 14, 2020

DrahtBot commented at 8:28 pm on February 14, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18361 (build: Make the help string for –with-gui configure option unambiguous by hebasto)
#18298 (build: Fix Qt processing of configure script for depends with DEBUG=1 by hebasto)
#18297 (build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows by hebasto)
#18216 (test, build: Enable -Werror=sign-compare by Empact)
#16883 (WIP: Qt: add QML based mobile GUI by icota)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto commented at 3:17 pm on February 15, 2020: member

Suppress any warnings from Boost headers by marking them as system headers.

Suppressing any warnings from Boost does not look great. Some related discussions about warnings from leveldb could be find in #16710 and #16722.

vasild commented at 9:04 am on February 17, 2020: member

@hebasto

Warnings from external headers (boost, qt, etc) are already suppressed if they are installed in /usr/include. I guess this is the common case with Linux. So, we just add an option to achieve the same if external headers are installed in another location.
The new option is off by default.
It would allow us to reliably build our code with full -Werror (not a partial set of -Werror=...).

vasild force-pushed on Feb 17, 2020

vasild force-pushed on Feb 19, 2020

vasild force-pushed on Feb 20, 2020

vasild force-pushed on Feb 21, 2020

in configure.ac:1139 in f1f8e84a0a outdated

1134@@ -1133,6 +1135,10 @@ dnl counter implementations. In 1.63 and later the std::atomic approach is defau
1135 m4_pattern_allow(DBOOST_AC_USE_STD_ATOMIC) dnl otherwise it's treated like a macro
1136 BOOST_CPPFLAGS="-DBOOST_SP_USE_STD_ATOMIC -DBOOST_AC_USE_STD_ATOMIC $BOOST_CPPFLAGS"
1137 
1138+if test "x$enable_suppress_external_warnings" = "xyes"; then
1139+  BOOST_CPPFLAGS="`echo $BOOST_CPPFLAGS |sed 's/-I/-idirafter /g'`"

Empact commented at 5:08 pm on February 28, 2020:

Why not isystem? It would more closely match the behavior of the -I it replaces. https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html

vasild commented at 7:11 pm on February 28, 2020:

Because -idirafter /usr/include does not break gcc’s #include_next, so I do not have to explicitly handle /usr/include in a different way.

Anyway, the --enable-isystem introduced in https://github.com/bitcoin/bitcoin/commit/572db3ce8103940c58cc90f4f770e989cf789835 in #14920 is superior to the above.

vasild commented at 7:28 pm on February 28, 2020:

I just rebased on top of https://github.com/bitcoin/bitcoin/commit/572db3ce8103940c58cc90f4f770e989cf789835 and ditched the above piece.

Empact commented at 8:27 pm on February 28, 2020:

Here’s a much pared-down version of 572db3c that looks to be effective (applied to Boost only): https://github.com/bitcoin/bitcoin/compare/master...Empact:2020-02-werror

vasild commented at 5:43 pm on March 2, 2020:

https://github.com/bitcoin/bitcoin/compare/master...Empact:2020-02-werror looks good. I confirm it works as expected on FreeBSD 12 with clang 9 - it makes it possible to compile Bitcoin Core with full -Werror! \o/

Are you going to create PR from it? If yes, then I will ditch this PR.

nit: the new option’s description enable isystem includes for dependencies and stricter warning checks (default is no) warrants s/ and stricter warning checks//

Thanks!

Empact commented at 4:08 pm on March 3, 2020:

Feel free to cherry-pick and modify the subject. Deferring to you as the original author unless you want to punt.

vasild commented at 7:58 pm on March 3, 2020:

Done. I also picked up the QT changes to get this working also with Clang 10.

vasild force-pushed on Feb 28, 2020

vasild force-pushed on Mar 3, 2020

vasild renamed this:
~~build: change --enable-werror to enable -Werror~~
build: add --enable-isystem and change --enable-werror to enable -Werror
on Mar 3, 2020

vasild marked this as ready for review on Mar 4, 2020

vasild commented at 10:42 am on March 4, 2020: member

@hebasto this PR went through some significant changes, I edited my comments accordingly and changed its status from “Draft” to “Open”. Your re-review would be welcome.

vasild requested review from Empact on Mar 4, 2020

DrahtBot added the label Needs rebase on Mar 6, 2020

build: Optionally include dependency headers with -isystem

When configured with --enable-isystem.

Was necessary to split QT_INCLUDES into QT_INCLUDES and
QT_MOC_INCLUDES because moc does not understand -isystem, e.g.:

    Unknown options: isystem/usr/local/Cellar/qt/5.10.0_1/include/QtNetwork[...]

This patch treats just Boost and QT headers. The other external headers could be
added as well, should it be necessary.

Note -isystem should not be applied to /usr/include, see BITCOIN_SYSTEM_INCLUDE
for a helper to convert -I to -isystem with /usr/include excepted.

c9376ae34c

build: change --enable-werror to enable -Werror

Before this patch `./configure --enable-werror` would selectively
turn only some warnings into errors, not all, by using
`-Werror=foo -Werror=bar` instead of a plain `-Werror` which turns all
warnings into errors.

To accommodate the new stricter build config, use --enable-isystem to
suppress warnings from external headers (e.g. boost, qt) on systems
that have them installed outside of /usr/include. On Linux the external
headers are typically installed in /usr/include and so warnings from
them are suppressed by default.

9553271a7f

vasild force-pushed on Mar 6, 2020

DrahtBot removed the label Needs rebase on Mar 6, 2020

vasild commented at 6:54 am on April 23, 2020: member

Closing this as it contains two not directly related changes - optional suppression of warnings from external headers and -Werror expansion. I will submit them separately.

vasild closed this on Apr 23, 2020

vasild deleted the branch on Apr 23, 2020

vasild commented at 8:16 pm on April 23, 2020: member

Took the idea of the first commit and re-implemented it in a (hopefully) better/shorter way in https://github.com/bitcoin/bitcoin/pull/18750

DrahtBot locked this on Feb 15, 2022

build: add –enable-isystem and change –enable-werror to enable -Werror #18149

Conflicts