build: Make --enable-suppress-external-warnings the default

[hebasto]

This is split from #21667.

For users, one of the preferred ways to use the Bitcoin Core client is compiling from source, either downloaded as a part of release or acquired via git.

Only basic knowledge about terminal is required to run:

$ cd bitcoin
$ ./autogen.sh
$ ./configure
$ make check

Also, it is natural to expect that number of users that have such knowledge is higher than the number of advanced users or developers.

Assuming the basic knowledge level of users it seems unwise to allow compiler and linker warnings because they could undermine the users' trust in the Bitcoin Core quality/security/reliability.

Of course, warnings could be emitted by different parts of code: either the code we are responsible for, or third parties code, including sub-trees in our repo:

• https://github.com/google/crc32c
• https://github.com/google/leveldb

and other dependencies:

• https://github.com/libevent/libevent
• https://github.com/boostorg/boost
• https://github.com/qt/qtbase
• ...

Currently, warnings from leveldb are suppressed unconditionally:

bitcoin/src/Makefile.leveldb.include

Line 39 in 5990009

leveldb_libleveldb_a_CXXFLAGS = $(filter-out -Wconditional-uninitialized -Werror=conditional-uninitialized -Wsuggest-override -Werror=suggest-override, $(AM_CXXFLAGS)) $(PIE_FLAGS)

OTOH, warnings from Qt and Boost code could be suppressed with the --enable-suppress-external-warnings configure option.

Making the --enable-suppress-external-warnings the default has the following benefits:

• don't alarm users who compile from source
• increase signal/noise ratio for developers when a new warning appears in our code (as an example, Qt warnings on macOS, where Homebrew's Qt is pretty fresh)
• allows to apply more -W and -Werror options by default (for example, -Wdocumentation and -Werror=documentation introduced in build: enable -Wdocumentation #21613)

There was an objection:

I don't think we should make --enable-suppress-external-warnings the default, but it should be better documented. Someone needs to be motivated to fix things upstream and/or notice problems when we update dependencies.

The answer to this objection is that fixing "things upstream" is not the top priority for developers. It is always possible to build with the --disable-suppress-external-warnings. Even more, with this PR the --disable-suppress-external-warnings will reveal leveldb warning as well.

[hebasto]

cc @vasild @Sjors @fanquake

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23473 (build: boring autotools cleanup by fanquake)
• #15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)

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.

[practicalswift]

Concept ACK

There is a trade-off here: making --enable-suppress-external-warnings the default will allow us to apply additional -W and -Werror options which will increase the likelihood of finding issues local to our code base, and it will increase the signal to noise. OTOH doing so might reduce the likelihood of us finding issues in upstream code bases.

Since we have to choose a default I think it makes sense to make the higher signal-to-noise alternative which focuses on local issues the default (--enable-suppress-external-warnings), and make --disable-suppress-external-warnings opt-in.

[vasild]

Concept ~0

The idea behind --enable-suppress-external-warnings is to be able to compile with -Werror, lots of -Wall and not be dragged down by boost/qt/etc issues. -Werror is for devs.

I don't agree with hiding warnings from users in order to increase their trust. The users' mistrust may increase if they understand that devs have swept under the carpet "dangerous" things.

increase signal/noise ratio for developers when a new warning appears in our code

Devs can use --enable-suppress-external-warnings, we don't rely on default settings.

allows to apply more -W and -Werror options by default

IMO -Werror is for devs. More -W is fine without changing the default of --enable-suppress-external-warnings.

OTOH warnings from external libraries are already suppressed if they are installed in /usr/include even without --enable-suppress-external-warnings. So enabling that by default may be seen as making compilation experience more consistent across platforms.

[vasild]

I assume all CI and all/most devs already use --enable-suppress-external-warnings --enable-werror in their build scripts.

[hebasto]

@vasild

Thanks for sharing your opinion!

I don't agree with hiding warnings from users in order to increase their trust.

We already hide warnings from leveldb. Unconditionally.

The users' mistrust may increase if they understand that devs have swept under the carpet "dangerous" things.

Isn't it the same as not passing non-default -W to a compiler, or using compiler's pragmas?

More -W is fine without changing the default of --enable-suppress-external-warnings.

-Wdocumentation requires --enable-suppress-external-warnings, on master.

I assume all CI and all/most devs already use --enable-suppress-external-warnings --enable-werror in their build scripts.

I'm among them :)

[vasild]

Isn't it the same as not passing non-default -W to a compiler, or using compiler's pragmas?

Yes, it is! And also, if external headers are installed in /usr/include, then warnings from them are swept under the carpet by the compiler already, regardless of --enable-suppress-external-warnings.

There are arguments for both doing and not doing this. Would be interesting to see what others think.

[DrahtBot]

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

[jonatack]

Concept ACK