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

pull vasild wants to merge 2 commits into bitcoin:master from vasild:werror changing 6 files +40 −15
  1. vasild commented at 7:53 PM on February 14, 2020: contributor

    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.

  2. DrahtBot added the label Build system on Feb 14, 2020
  3. DrahtBot commented at 8:28 PM on February 14, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  4. DrahtBot cross-referenced this on Feb 14, 2020 from issue build: add Wreturn-type to Werror flags, check on more Travis machines by Sjors
  5. DrahtBot cross-referenced this on Feb 15, 2020 from issue build: Optionally enable -Wzero-as-null-pointer-constant by Empact
  6. DrahtBot cross-referenced this on Feb 15, 2020 from issue Build: enable -Wdocumentation via isystem by Empact
  7. 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.

  8. vasild commented at 9:04 AM on February 17, 2020: contributor

    @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=...).
  9. vasild force-pushed on Feb 17, 2020
  10. vasild force-pushed on Feb 17, 2020
  11. vasild force-pushed on Feb 19, 2020
  12. vasild force-pushed on Feb 20, 2020
  13. vasild force-pushed on Feb 21, 2020
  14. DrahtBot cross-referenced this on Feb 28, 2020 from issue test, build: Enable -Werror=sign-compare by Empact
  15. 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.

  16. vasild force-pushed on Feb 28, 2020
  17. DrahtBot cross-referenced this on Feb 29, 2020 from issue WIP: Qt: add QML based mobile GUI by icota
  18. DrahtBot cross-referenced this on Feb 29, 2020 from issue UI external signer support (e.g. hardware wallet) by Sjors
  19. DrahtBot cross-referenced this on Feb 29, 2020 from issue External signer support - Wallet Box edition by Sjors
  20. DrahtBot cross-referenced this on Feb 29, 2020 from issue util: Replace boost sleep with std sleep by MarcoFalke
  21. vasild force-pushed on Mar 3, 2020
  22. 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
  23. vasild marked this as ready for review on Mar 4, 2020
  24. vasild commented at 10:42 AM on March 4, 2020: contributor

    @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.

  25. vasild requested review from Empact on Mar 4, 2020
  26. DrahtBot cross-referenced this on Mar 5, 2020 from issue build: Remove Boost Chrono by fanquake
  27. DrahtBot added the label Needs rebase on Mar 6, 2020
  28. 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
  29. 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
  30. vasild force-pushed on Mar 6, 2020
  31. DrahtBot removed the label Needs rebase on Mar 6, 2020
  32. DrahtBot cross-referenced this on Mar 9, 2020 from issue build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows by hebasto
  33. DrahtBot cross-referenced this on Mar 9, 2020 from issue build: Fix Qt processing of configure script for depends with DEBUG=1 by hebasto
  34. DrahtBot cross-referenced this on Mar 9, 2020 from issue build: Require pkg-config for all of the hosts by hebasto
  35. DrahtBot cross-referenced this on Mar 16, 2020 from issue build: Make the help string for --with-gui configure option unambiguous by hebasto
  36. hebasto cross-referenced this on Apr 22, 2020 from issue build: Suppress -Wdeprecated-copy warnings by hebasto
  37. vasild commented at 6:54 AM on April 23, 2020: contributor

    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.

  38. vasild closed this on Apr 23, 2020
  39. vasild deleted the branch on Apr 23, 2020
  40. vasild commented at 8:16 PM on April 23, 2020: contributor

    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

  41. bitcoin locked this on Feb 15, 2022
Contributors
vasildDrahtBothebastoEmpact


Empact

Labels
Build system
Linked (view graph)
#16710 build: Enable -Wsuggest-override if available#16722 build: Disable warnings for leveldb subtree by default#18738 build: Suppress -Wdeprecated-copy warnings#18750 build: optionally skip external warnings
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC