build: optionally skip external warnings #18750

vasild · 2020-04-23T20:12:50Z

Add an option to ./configure to suppress compilation warnings from
external headers. The option is off by default (no change in behavior,
show warnings from external headers).

This option is useful if e.g. Boost or Qt is installed outside of
/usr/include (warnings from headers in /usr/include are already
suppressed by default) and those warnings stand in the way of compiling
Bitcoin Core with -Werror[=...] or they just clutter the build output
too much and make our own warnings hard to spot.

-isystem /usr/include bricks GCC's #include_next, so we use
-idirafter instead. This way we don't have to treat /usr/include
specially.

vasild · 2020-04-23T20:13:53Z

cc @Empact @hebasto @theuni

DrahtBot · 2020-04-23T20:37:51Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc, wallet: Expose database format in getwalletinfo #20125 (rpc, wallet: Expose database format in getwalletinfo by promag)
wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)

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.

vasild · 2020-04-24T09:02:16Z

To test that this works as intended plant some warning in a Qt header (if you don't have already!). For example:

inline bool f(int iii, unsigned uuu) { return iii < uuu; }

in /qt/installed/here/include/qt5/QtCore/qglobal.h and see that the warning is gone with ./configure --enable-suppress-external-warnings.

vasild · 2020-04-24T09:16:15Z

We currently compile our code with some warnings explicitly silenced: -Wno-unused-local-typedef -Wno-deprecated-register -Wno-implicit-fallthrough.

Currently Bitcoin Core is free of such warnings, but they can sneak in at anytime completely unnoticed - due to the above flags nobody will notice if a new code introduces such a warning.

If this PR gets merged, then we can consider removing the above -Wno-... flags. If it turns out that some external header produces such warnings, then they can be silenced using the functionality from this PR. This way we will spot if a new code introduces such a warning in Bitcoin Core.

jonatack · 2020-04-25T16:18:48Z

Concept ACK

brakmic · 2020-05-01T10:12:56Z

Concept ACK.

hebasto · 2020-05-03T23:56:52Z

Tested on Ubuntu 20.04 LTS (gcc 9.3.0) and macOS 10.15.4 (llvm clang 10.0). Works as expected.

I've looked through our docs, and didn't find any compiler warning policy. Or did I miss it?

From the POV of a user who respects one's own safety and builds one's own node binaries from the source, I believe there is no a such thing like a "harmless compiler warning". At least for major OSes (the two latest versions of Ubuntu LTS, Debian stable, Fedora, macOS, Windows with default system compilers) on the x86_64 platform. In the worst case, a rare warning should be documented in the release notes.

Concept ACK.

Is it possible to increase the granularity of --enable-suppress-external-warnings?
E.g., --enable-suppress-external-warnings=qt?
Related: #16722.

vasild · 2020-05-04T06:35:41Z

Tested on Ubuntu 20.04 LTS (gcc 9.3.0) and macOS 10.15.4 (llvm clang 10.0). Works as expected.

Excellent!

From the POV of a user who respects one's own safety and builds one's own node binaries from the source, I believe there is no a such thing like a "harmless compiler warning".

I agree. This is why the new option is off by default. And I do not think it should ever be changed (to on by default).

Is it possible to increase the granularity of --enable-suppress-external-warnings?
E.g., --enable-suppress-external-warnings=qt?

I did consider this! It would increase the complexity of this feature to some extent, but there is a bigger problem - we suppress warnings based on the directory in which the header which produced the warning is located. So, if both qt and boost are installed in the same directory (e.g. /usr/local/include) then such separation will not work as expected.

Add an option to `./configure` to suppress compilation warnings from external headers. The option is off by default (no change in behavior, show warnings from external headers). This option is useful if e.g. Boost or Qt is installed outside of `/usr/include` (warnings from headers in `/usr/include` are already suppressed by default) and those warnings stand in the way of compiling Bitcoin Core with `-Werror[=...]` or they just clutter the build output too much and make our own warnings hard to spot. `-isystem /usr/include` bricks GCC's `#include_next`, so we use `-idirafter` instead. This way we don't have to treat `/usr/include` specially. Github-Pull: bitcoin#18750 Rebased-From: 1e4e981

vasild · 2020-06-15T17:06:15Z

Rebased (trivial conflict with the merged #18297).

vasild · 2020-08-11T12:18:44Z

Rebased to resolve conflicts.

DrahtBot · 2020-10-01T07:28:37Z

Guix builds

File	commit `1769828` (master)	commit `75b42f9` (master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`2c90411d96eeef0a...`	`d1b1c6711dab92dc...`
*-aarch64-linux-gnu.tar.gz	`9316a0f77c5cf478...`	`895d7365e18f30bc...`
*-arm-linux-gnueabihf-debug.tar.gz	`222c50ce8f2c0ffc...`	`9ec9ea5de526a12f...`
*-arm-linux-gnueabihf.tar.gz	`4e2977e8a8a33266...`	`3dc0c21c7abc1d80...`
*-riscv64-linux-gnu-debug.tar.gz	`820db95fd8891d61...`	`25306dd7bfd1f2e5...`
*-riscv64-linux-gnu.tar.gz	`4d51430614ca2fc7...`	`a945cc0f5faf05c8...`
*-win-unsigned.tar.gz	`51f33713e3d706f9...`	`be3b56bbda932053...`
*-win64-debug.zip	`dc6355584170c13d...`	`adb36e02fc77d847...`
*-win64-setup-unsigned.exe	`6280c2c1ee411214...`	`673ddc3eb2c13da5...`
*-win64.zip	`1c63275334074f40...`	`7544fbb63518b312...`
*-x86_64-linux-gnu-debug.tar.gz	`f249f8a755a16fbb...`	`95c9072e4bf7595f...`
*-x86_64-linux-gnu.tar.gz	`3cab2d29de25d7ac...`	`fc93fa6117ce766c...`
*.tar.gz	`bac5492f2d68c97c...`	`bb2f70ea90c584ac...`
guix_build.log	`e97379d33c122448...`	`86e53b0040f36f5c...`
guix_build.log.diff		`57e8da602d723af1...`

DrahtBot · 2020-10-02T16:53:32Z

Gitian builds

File	commit `3487e42` (master)	commit `075064e` (master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`610763669e7e3cb7...`	`33035bce46b95145...`
*-aarch64-linux-gnu.tar.gz	`51812ca091622ed9...`	`b406d50cec49a3f2...`
*-arm-linux-gnueabihf-debug.tar.gz	`f93c0427cd047290...`	`06cbc1ca11daa6f1...`
*-arm-linux-gnueabihf.tar.gz	`6df65db3f3a7a123...`	`c6ff1969ea796a5f...`
*-osx-unsigned.dmg	`0d4a4bf011b2019e...`	`3c1b7b22e158ebfa...`
*-osx64.tar.gz	`0aa9fd6f4187d33f...`	`b3f3b6a4df2af0a3...`
*-riscv64-linux-gnu-debug.tar.gz	`58914f8feac72537...`	`33b5790f09aaad56...`
*-riscv64-linux-gnu.tar.gz	`1a30336e17f7cd6b...`	`022fb921d6fa95ef...`
*-win64-debug.zip	`2f06f34ffd627e79...`	`d721eda9e9bfb8e8...`
*-win64-setup-unsigned.exe	`f23026e90602164d...`	`a5ee4164e3273583...`
*-win64.zip	`8d3acee5e2584d3b...`	`c3de365b9adceb46...`
*-x86_64-linux-gnu-debug.tar.gz	`be688faf54cfea87...`	`5c5bc415d8c7b2c3...`
*-x86_64-linux-gnu.tar.gz	`2a78323df8c70758...`	`c0f268c3ebf21bdc...`
*.tar.gz	`7bf0d038afb59072...`	`ae81bc977248923e...`
bitcoin-core-linux-0.21-res.yml	`2b37aee7c44927ee...`	`e4ebc15cddc8666b...`
bitcoin-core-osx-0.21-res.yml	`1c3793321abc2f61...`	`9d5183fb50a6f3e9...`
bitcoin-core-win-0.21-res.yml	`5de911b923fdda5a...`	`9bd083214ee81fd2...`
linux-build.log	`e44bb3ce2f5117e8...`	`8e0d48eaf53688fe...`
osx-build.log	`886fd63ccaf3e7cb...`	`36567233ae47375f...`
win-build.log	`aba8b79f48c7ff90...`	`237c646313597eeb...`
bitcoin-core-linux-0.21-res.yml.diff		`11b392af50349d0e...`
bitcoin-core-osx-0.21-res.yml.diff		`7aef47fbcda76204...`
bitcoin-core-win-0.21-res.yml.diff		`5178884b6c020cf6...`
linux-build.log.diff		`d835a581c9dbfcd2...`
osx-build.log.diff		`e877416b362f608b...`
win-build.log.diff		`75c0c7e2054c8217...`

configure.ac

Add an option to `./configure` to suppress compilation warnings from external headers. The option is off by default (no change in behavior, show warnings from external headers). This option is useful if e.g. Boost or Qt is installed outside of `/usr/include` (warnings from headers in `/usr/include` are already suppressed by default) and those warnings stand in the way of compiling Bitcoin Core with `-Werror[=...]` or they just clutter the build output too much and make our own warnings hard to spot.

hebasto

ACK ba8950e, tested on Linux Mint 20 (x86_64).

With --enable-suppress-external-warnings having warnings from leveldb only:

$ make > /dev/null 
leveldb/db/c.cc: In function ‘leveldb_filterpolicy_t* leveldb_filterpolicy_create_bloom(int)’:
leveldb/db/c.cc:474:17: warning: ‘virtual const char* leveldb_filterpolicy_create_bloom(int)::Wrapper::Name() const’ can be marked override [-Wsuggest-override]
  474 |     const char* Name() const { return rep_->Name(); }
      |                 ^~~~
leveldb/db/c.cc:475:10: warning: ‘virtual void leveldb_filterpolicy_create_bloom(int)::Wrapper::CreateFilter(const leveldb::Slice*, int, std::string*) const’ can be marked override [-Wsuggest-override]
  475 |     void CreateFilter(const Slice* keys, int n, std::string* dst) const {
      |          ^~~~~~~~~~~~
leveldb/db/c.cc:478:10: warning: ‘virtual bool leveldb_filterpolicy_create_bloom(int)::Wrapper::KeyMayMatch(const leveldb::Slice&, const leveldb::Slice&) const’ can be marked override [-Wsuggest-override]
  478 |     bool KeyMayMatch(const Slice& key, const Slice& filter) const {
      |          ^~~~~~~~~~~

luke-jr

utACK ba8950e

practicalswift · 2020-10-13T14:33:44Z

ACK ba8950e: diff looks correct!

Really looking forward to having this in master :)

laanwj · 2020-10-14T17:11:16Z

Thanks for adding this option, it definitely helps reduce the flood of warnings in boost and qt etc.

ba8950e build: optionally skip external warnings (Vasil Dimov) Pull request description: Add an option to `./configure` to suppress compilation warnings from external headers. The option is off by default (no change in behavior, show warnings from external headers). This option is useful if e.g. Boost or Qt is installed outside of `/usr/include` (warnings from headers in `/usr/include` are already suppressed by default) and those warnings stand in the way of compiling Bitcoin Core with `-Werror[=...]` or they just clutter the build output too much and make our own warnings hard to spot. `-isystem /usr/include` bricks GCC's `#include_next`, so we use `-idirafter` instead. This way we don't have to treat `/usr/include` specially. ACKs for top commit: practicalswift: ACK ba8950e: diff looks correct! hebasto: ACK ba8950e, tested on Linux Mint 20 (x86_64). luke-jr: utACK ba8950e Tree-SHA512: 9b54fae8590be6c79f2688a5aca09e0a9067f481dabecdd49bb278c08a62ac2b0cc704c894fbd53240e77ac84da0c7a237845df0a696cfbdb0359e1c8e2e10c9

* Merge bitcoin#18750: build: optionally skip external warnings ba8950e build: optionally skip external warnings (Vasil Dimov) Pull request description: Add an option to `./configure` to suppress compilation warnings from external headers. The option is off by default (no change in behavior, show warnings from external headers). This option is useful if e.g. Boost or Qt is installed outside of `/usr/include` (warnings from headers in `/usr/include` are already suppressed by default) and those warnings stand in the way of compiling Bitcoin Core with `-Werror[=...]` or they just clutter the build output too much and make our own warnings hard to spot. `-isystem /usr/include` bricks GCC's `#include_next`, so we use `-idirafter` instead. This way we don't have to treat `/usr/include` specially. ACKs for top commit: practicalswift: ACK ba8950e: diff looks correct! hebasto: ACK ba8950e, tested on Linux Mint 20 (x86_64). luke-jr: utACK ba8950e Tree-SHA512: 9b54fae8590be6c79f2688a5aca09e0a9067f481dabecdd49bb278c08a62ac2b0cc704c894fbd53240e77ac84da0c7a237845df0a696cfbdb0359e1c8e2e10c9 * add --enable-suppress-external-warnings to matrix.sh Co-authored-by: Wladimir J. van der Laan <laanwj@protonmail.com>

This was referenced Apr 23, 2020

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

Closed

build: Suppress -Wdeprecated-copy warnings #18738

Merged

DrahtBot added the Build system label Apr 23, 2020

This was referenced Apr 23, 2020

build: Make the help string for --with-gui configure option unambiguous #18361

Closed

build: Fix Qt processing of configure script for depends with DEBUG=1 #18298

Merged

build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows #18297

Merged

vasild mentioned this pull request May 5, 2020

build: enable -Werror=gnu #18887

Merged

DrahtBot added the Needs rebase label Jun 13, 2020

vasild force-pushed the suppress_external_warnings branch from 1e4e981 to 426e6dc Compare June 15, 2020 17:05

DrahtBot removed the Needs rebase label Jun 15, 2020

This was referenced Jun 17, 2020

UI external signer support (e.g. hardware wallet) #16549

Closed

External signer support - Wallet Box edition #16546

Merged

util: add RunCommandParseJSON #15382

Merged

DrahtBot added the Needs rebase label Aug 5, 2020

vasild force-pushed the suppress_external_warnings branch from 426e6dc to ec1ea76 Compare August 11, 2020 12:18

DrahtBot removed the Needs rebase label Aug 11, 2020

DrahtBot mentioned this pull request Aug 20, 2020

build: Add Qt version checking #19689

Merged

DrahtBot added the Needs rebase label Aug 24, 2020

maflcko added Needs gitian build DrahtBot Guix build requested labels Sep 30, 2020

DrahtBot removed the DrahtBot Guix build requested label Oct 1, 2020

DrahtBot removed the Needs gitian build label Oct 2, 2020

DrahtBot mentioned this pull request Oct 12, 2020

rpc, wallet: Expose database format in getwalletinfo #20125

Merged

luke-jr suggested changes Oct 12, 2020

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

vasild force-pushed the suppress_external_warnings branch from 3598707 to ba8950e Compare October 12, 2020 16:18

hebasto approved these changes Oct 12, 2020

View reviewed changes

luke-jr approved these changes Oct 13, 2020

View reviewed changes

laanwj merged commit 99a1d57 into bitcoin:master Oct 14, 2020

vasild deleted the suppress_external_warnings branch October 14, 2020 16:06

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

build: optionally skip external warnings #18750

build: optionally skip external warnings #18750

Uh oh!

Conversation

vasild commented Apr 23, 2020 • edited by laanwj Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Apr 23, 2020

Uh oh!

DrahtBot commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

vasild commented Apr 24, 2020

Uh oh!

vasild commented Apr 24, 2020

Uh oh!

jonatack commented Apr 25, 2020

Uh oh!

brakmic commented May 1, 2020

Uh oh!

hebasto commented May 3, 2020

Uh oh!

vasild commented May 4, 2020

Uh oh!

vasild commented Jun 15, 2020

Uh oh!

vasild commented Aug 11, 2020

Uh oh!

DrahtBot commented Oct 1, 2020

Guix builds

Uh oh!

DrahtBot commented Oct 2, 2020

Gitian builds

Uh oh!

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 13, 2020

Uh oh!

laanwj commented Oct 14, 2020

Uh oh!

Uh oh!

vasild commented Apr 23, 2020 •

edited by laanwj

Loading

DrahtBot commented Apr 23, 2020 •

edited

Loading