build: suppress external warnings by default

fanquake commented at 11:05 am on June 13, 2023: member

I think we are at the point where it make more sense to make this the default, than not. It’s already used in the CI, and I assume most building locally are also utilising it.

DrahtBot commented at 11:05 am on June 13, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, stickies-v, achow101
Concept ACK	TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Build system on Jun 13, 2023

maflcko commented at 11:18 am on June 13, 2023: member

I think there is still a benefit to developers being notified about warnings in headers. Otherwise no one may notice the false warnings and report them upstream. Also, if they point to real problems, it will be harder to track them down.

fanquake commented at 11:30 am on June 13, 2023: member

I think there is still a benefit to developers being notified about warnings in headers.

They can still be notified, by disabling the suppression.

Otherwise no one may notice the false warnings and report them upstream.

Then we should stop suppressing them globally in the CI, so we are more likely to see (and then report) the warnings across all relevant platforms / arches. Although then we can’t use -Werror (which is also why some devs would always be suppressing locally).

maflcko commented at 11:42 am on June 13, 2023: member

Also, it may be good to give some more background. IIRC this is only needed for bdb4 and qt5, so someone compiling bitcoind with sqlite shouldn’t have to suppress warnings, no?

DrahtBot added the label CI failed on Jun 13, 2023

fanquake commented at 12:43 pm on June 13, 2023: member

IIRC this is only needed for bdb4 and qt5,

There are likely warnings produced for all dependencies (includeing Boost and libevent), because warning output is dependant on:

architecture
operating system (& version)
compiler (& version)
compiler flags being used
other compile options i.e LTO
packages being used (& version) (& system vs depends)

Where a number of the above continue to change over time.

so someone compiling bitcoind with sqlite shouldn’t have to suppress warnings, no?

This isn’t the case, for example, for someone compiling on macOS, with only sqlite. They’ll see warnings for both Boost and libevent. Someone cross-compiling for riscv64 on Ubuntu will see warnings from Boost (Test) etc.

DrahtBot removed the label CI failed on Jun 13, 2023

in configure.ac:248 in 322b64eeb0 outdated

243@@ -244,10 +244,10 @@ dnl May be useful if warnings from external headers clutter the build output
244 dnl too much, so that it becomes difficult to spot Bitcoin Core warnings
245 dnl or if they cause a build failure with --enable-werror.
246 AC_ARG_ENABLE([suppress-external-warnings],
247-  [AS_HELP_STRING([--enable-suppress-external-warnings],
248-                  [Suppress warnings from external headers (default is no)])],
249+  [AS_HELP_STRING([--disable-suppress-external-warnings],
250+                  [Do not suppress warnings from external headers (default is too suppress)])],

dimitaracev commented at 1:58 pm on June 13, 2023:

Did you mean default is to suppress?

fanquake commented at 3:29 pm on June 13, 2023:

Fixed.

achow101 commented at 2:21 pm on June 13, 2023: member

so someone compiling bitcoind with sqlite shouldn’t have to suppress warnings, no?

I get a large number of warnings from boost.

fanquake force-pushed on Jun 13, 2023

DrahtBot added the label CI failed on Jun 13, 2023

hebasto commented at 12:58 pm on June 14, 2023: member

Concept ACK. #22041 :)

hebasto commented at 1:10 pm on June 14, 2023: member

I think this comment by @Sjors:

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.

is still relevant. Maybe its “should be better documented” part is worth being added to this PR?

fanquake commented at 1:12 pm on June 14, 2023: member

“should be better documented”

How? Where?

hebasto commented at 1:15 pm on June 14, 2023: member

“should be better documented”

How? Where?

Developer Notes? Maybe in the “Development tips and tricks” section?

cc @Sjors

TheCharlatan commented at 4:38 pm on June 14, 2023: contributor

Did this recently annoy you, or were there users opening issues / having trouble parsing through the warnings?

TheCharlatan commented at 4:44 pm on June 14, 2023: contributor

Concept ACK

I don’t ever run configure without this flag, so having it out of mind is a win for me.

stickies-v commented at 4:56 pm on June 14, 2023: contributor

Concept ACK. The large amount of warnings produced without this flag generally makes me not look at any warning when I forgot to set the flag. Agreed that we do need to be vigilant for upstream warnings, but I don’t think disabling the suppression by default really helps with that.

hebasto commented at 5:16 pm on June 14, 2023: member

Did this recently annoy you, or were there users opening issues / having trouble parsing through the warnings?

On Ubuntu 22.04:

0$ ./autogen.sh && ./configure CC=clang CXX=clang++
1$ make 2> >(wc -l >&2) > /dev/null 
2168634

It does not annoy me at all :)

It just makes spotting of warnings I am interesting in a bit harder.

hebasto approved

hebasto commented at 5:20 pm on June 14, 2023: member

ACK 925f1708a96008a0caf0c5d64d46132878babdd0

0$ git grep -l "enable-suppress-external-warnings"
1doc/developer-notes.md

Drop it as well?

fanquake commented at 10:44 am on June 15, 2023: member

Drop it as well?

I think it’s correct as-is?

hebasto commented at 11:47 am on June 15, 2023: member

Drop it as well?

I think it’s correct as-is?

Yes. Technically, it is correct. But it seems confusing to me as the line “The output is denoised of errors from external dependencies and includes with --enable-suppress-external-warnings and --config src/.bear-tidy-config. Both options may be omitted to view the full list of errors.” explains the options used the followed code snippet. And the --enable-suppress-external-warnings option is not used in there anymore.

build: suppress external warnings by default 3b2acfcfec

fanquake force-pushed on Jun 15, 2023

fanquake commented at 1:12 pm on June 15, 2023: member

Ok. Just deleted all that.

hebasto approved

hebasto commented at 1:17 pm on June 15, 2023: member

ACK 3b2acfcfec83a4e6e50b3f21e0810274bdb05afb

stickies-v approved

stickies-v commented at 1:45 pm on June 15, 2023: contributor

ACK 3b2acfcfec83a4e6e50b3f21e0810274bdb05afb

achow101 commented at 1:51 pm on June 15, 2023: member

ACK 3b2acfcfec83a4e6e50b3f21e0810274bdb05afb

achow101 merged this on Jun 15, 2023

achow101 closed this on Jun 15, 2023

fanquake deleted the branch on Jun 15, 2023

sidhujag referenced this in commit d1debfbf82 on Jun 15, 2023

fanquake referenced this in commit 3210f224db on Jun 29, 2023

fanquake referenced this in commit b5ebeb376d on Jun 30, 2023

sidhujag referenced this in commit 7e37006ba6 on Jun 30, 2023

bitcoin locked this on Jun 14, 2024

build: suppress external warnings by default #27872

Reviews