build: Make –enable-suppress-external-warnings the default #22041

1. 
   hebasto commented at 11:20 am on May 24, 2021: member

   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:

   0$ cd bitcoin
   1$ ./autogen.sh
   2$ ./configure
   3$ 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: https://github.com/bitcoin/bitcoin/blob/599000903e09e5ae3784d15ae078a2e1ed89ab12/src/Makefile.leveldb.include#L39

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

2. build: Make --enable-suppress-external-warnings the default 887c2cfd07
3. build: Extend --enable-suppress-external-warnings on LevelDB 2b0427af1f
4. hebasto added the label Build system on May 24, 2021
5. 
   hebasto commented at 11:23 am on May 24, 2021: member

   cc @vasild @Sjors @fanquake
6. 
   DrahtBot commented at 10:07 pm on May 24, 2021: 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:

   • #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.

7. 
   practicalswift commented at 8:22 pm on May 25, 2021: contributor

   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.

8. 
   vasild commented at 4:47 pm on May 28, 2021: member

   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.

9. 
   vasild commented at 4:49 pm on May 28, 2021: member

   I assume all CI and all/most devs already use --enable-suppress-external-warnings --enable-werror in their build scripts.
10. 
    hebasto commented at 7:32 pm on May 29, 2021: member

    @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 :)

11. 
    vasild commented at 8:58 am on May 31, 2021: member

    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.

12. 
    DrahtBot commented at 0:42 am on November 13, 2021: member

    🐙 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”.

13. DrahtBot added the label Needs rebase on Nov 13, 2021
14. hebasto closed this on Nov 21, 2021

    ---

15. 
    jonatack commented at 10:26 am on March 28, 2022: member

    Concept ACK
16. DrahtBot locked this on Mar 28, 2023

Contributors
hebasto DrahtBot practicalswift vasild jonatack

Labels
Build system Needs rebase