build: Do not repeat warning names in -Werror=... options #20544

hebasto commented at 8:51 AM on December 2, 2020: member

This PR:

improves the configure.ac maintainability by removing compiler warning/error names duplication
shows compiler warnings when compiled with depends

Close #18092.

Split from #20182 as requested by MarcoFalke, ~and is required for it~.

fanquake added the label Build system on Dec 2, 2020

hebasto force-pushed on Dec 2, 2020

vasild commented at 9:46 AM on December 2, 2020: member

Can't do the configure.ac changes alone or before the CI changes: #20182 (comment)

MarcoFalke added the label Needs gitian build on Dec 2, 2020

MarcoFalke added the label Needs Guix build on Dec 2, 2020

hebasto commented at 9:50 AM on December 2, 2020: member

Can't do the configure.ac changes alone or before the CI changes: #20182 (comment)

CI fails without adding --enable-suppress-external-warnings. Or did I understand you wrong?

hebasto commented at 9:55 AM on December 2, 2020: member

Can't do the configure.ac changes alone or before the CI changes: #20182 (comment)

You are suggesting to reorder commits, right? @MarcoFalke are you agree?

DrahtBot commented at 4:02 PM on December 2, 2020: 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:

#21051 (Fix -Wmismatched-tags warnings by hebasto)
#20586 (Fix Windows build with --enable-werror by hebasto)

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 commented at 9:58 PM on December 2, 2020: contributor

Concept ACK: this redundancy is annoying :)

DrahtBot removed the label Needs Guix build on Dec 3, 2020

DrahtBot added the label Needs rebase on Dec 4, 2020

MarcoFalke deleted a comment on Dec 4, 2020

DrahtBot removed the label Needs gitian build on Dec 4, 2020

MarcoFalke deleted a comment on Dec 4, 2020

build: Do not repeat warning names in -Werror=... options

This change also effectively activates -W... flags for builds with
dependencies.

2ed0a09bae

hebasto force-pushed on Dec 4, 2020

hebasto commented at 7:37 PM on December 4, 2020: member

Rebased 5823be9c88df141d2c876447c1bbab9bfdc260c0 -> 2ed0a09baeac4f99460879e3d68741e3eeb69951 (pr20544.02 -> pr20544.03) due to the conflict with #20182.

DrahtBot removed the label Needs rebase on Dec 4, 2020

MarcoFalke added the label Needs gitian build on Dec 5, 2020

MarcoFalke added the label Needs Guix build on Dec 5, 2020

in configure.ac:390 in 2ed0a09bae

 402 | -  AX_CHECK_COMPILE_FLAG([-Werror=unreachable-code-loop-increment],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unreachable-code-loop-increment"],,[[$CXXFLAG_WERROR]])
 403 | +  ERROR_CXXFLAGS=$CXXFLAG_WERROR
 404 |  fi
 405 |  
 406 | -if test "x$CXXFLAGS_overridden" = "xno"; then
 407 | +if test "x$depends_prefix" != "x" || test "x$CXXFLAGS_overridden" = "xno"; then

vasild commented at 3:02 PM on December 5, 2020:

off-topic: why is "x$var" != "x" used all over the place instead of -n "$var"? Or why not just "$CXXFLAGS_overridden" = "no"?

hebasto commented at 3:07 PM on December 5, 2020:

https://www.gnu.org/software/automake/faq/autotools-faq.html#What-does-_0027test-_0022X_0024variable_0022-_003d-X_0027-mean_003f

vasild commented at 3:12 PM on December 5, 2020:

Thanks!

vasild approved

vasild commented at 3:02 PM on December 5, 2020: member

ACK 2ed0a09b

DrahtBot commented at 2:00 AM on December 6, 2020: member

Guix builds

File	commit f35e4d906fee907f28ac5d8f32d4948e6b7b14c3<br>(master)	commit bab61ba28f80391a76efdc30ab97822eb99b010c<br>(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`102cff8d52a60f94...`	`e7f44589d776853a...`
*-aarch64-linux-gnu.tar.gz	`66ebfaa0452e5e46...`	`16d2db6e0fc41a3c...`
*-arm-linux-gnueabihf-debug.tar.gz	`282f867ba11c0a4c...`	`e8c5caa35ac4ea8c...`
*-arm-linux-gnueabihf.tar.gz	`ec0e99ed1c16fd62...`	`08123a6c33679595...`
*-riscv64-linux-gnu-debug.tar.gz	`214c19ef6d344db0...`	`aa09b337e014057b...`
*-riscv64-linux-gnu.tar.gz	`d53a18d17d17628d...`	`591a6a5d274c8074...`
*-win-unsigned.tar.gz	`c3e8d8dfb899e281...`	`2a96380f1660cae0...`
*-win64-debug.zip	`54d642157a29e9de...`	`32b84e5ec7e56343...`
*-win64-setup-unsigned.exe	`1a646c73f7937864...`	`151759f0c7a58c02...`
*-win64.zip	`f18f660a2615faf9...`	`58885eb26110040d...`
*-x86_64-linux-gnu-debug.tar.gz	`1dea4423412fce06...`	`453d9ce625b61af7...`
*-x86_64-linux-gnu.tar.gz	`210c9562cb5de4d3...`	`9ef847bf9e3eb110...`
*.tar.gz	`cf54eeba51ad26d3...`	`0a4db0ece73c9f1b...`
guix_build.log	`c2950e5b47ddbf20...`	`fef3488bd919da16...`
guix_build.log.diff		`ae4413e310d4e1c3...`

DrahtBot removed the label Needs Guix build on Dec 6, 2020

in configure.ac:429 in 2ed0a09bae

 421 | @@ -435,6 +422,16 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
 422 |    AX_CHECK_COMPILE_FLAG([-Wdeprecated-register],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-register"],,[[$CXXFLAG_WERROR]])
 423 |    AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-implicit-fallthrough"],,[[$CXXFLAG_WERROR]])
 424 |    AX_CHECK_COMPILE_FLAG([-Wdeprecated-copy],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-copy"],,[[$CXXFLAG_WERROR]])
 425 | +
 426 | +  dnl This is a temporary workaround. See https://github.com/bitcoin/bitcoin/pull/17919#issuecomment-674404780
 427 | +  AX_CHECK_COMPILE_FLAG([-Wunused-command-line-argument],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-unused-command-line-argument"],,[[$CXXFLAG_WERROR]])
 428 | +
 429 | +  dnl -Wmissing-braces is broken for aggregate initialization of std::array in Clang (where it is enabled by -Wall) before 5.0

fanquake commented at 5:53 AM on December 6, 2020:

in Clang (where it is enabled by -Wall) before 5.0

Don't we only support Clang 5.0+ now? Why do we need to work around something that happens with Clang < 5.0?

hebasto commented at 3:56 PM on December 6, 2020:

Sorry for a typo. Should be "prior to 6.0". Going to fix it.

in configure.ac:426 in 2ed0a09bae

 421 | @@ -435,6 +422,16 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
 422 |    AX_CHECK_COMPILE_FLAG([-Wdeprecated-register],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-register"],,[[$CXXFLAG_WERROR]])
 423 |    AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-implicit-fallthrough"],,[[$CXXFLAG_WERROR]])
 424 |    AX_CHECK_COMPILE_FLAG([-Wdeprecated-copy],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-copy"],,[[$CXXFLAG_WERROR]])
 425 | +
 426 | +  dnl This is a temporary workaround. See https://github.com/bitcoin/bitcoin/pull/17919#issuecomment-674404780

fanquake commented at 5:56 AM on December 6, 2020:

If this is only relevant when compiling for darwin, can't we scope it as such? Otherwise you are turning off this warning to accommodate what is essentially a quirk of our depends system, which only happens when cross-compiling for a single target.

fanquake changes_requested

fanquake commented at 6:54 AM on December 6, 2020: member

Can the PR title / description be updated to summarize what's actually changing here? There's mention of duplication, which I'm not sure I understand, and no mentions of the main difference in behaviour between master and this PR.

This PR is changing our --enable-werror option from being a select set of -Werror=x cases that we've added to over time, i.e -Werror=date-time, to just being -Werror wholesale. As a result, --enable-werror would now seemingly be unusable in more scenarios (without --enable-suppress-external-warnings), given the potential of warnings from system libraries.

I'm not sure why the addition of a diagnostic flag to ERROR_CXXFLAGS, if it's also included in WARN_CXXFLAGS is duplication/redundancy, as they are doing different things.

It would also be nice to know what the broader goal is here. Is it to be able to compile with -Werror, at basically any cost (i.e potentially suppress lots of things)?

If that's the case, why are we disabling it entirely for a single warning, i.e in the win64 build, and losing the supposed benefits of having it on, rather than suppressing that single warning just in the CI? i.e adding CXXFLAGS=-Wno-... to BITCOIN_CONFIG.

I'll also note that with this change, our warnings flags are going to be added to CXXFLAGS, when performing a depends build, regardless of whether CXXFLAGS has been overridden. I'm not sure if that's desired, and would be another change in behaviour that isn't summarized in the PR description or commits.

vasild commented at 3:09 PM on December 6, 2020: member

--enable-werror would now seemingly be unusable in more scenarios (without --enable-suppress-external-warnings)

While that is true, strictly speaking, there is also another side to it: --enable-werror would now catch problems in more scenarios (i.e. scenarios where a warning is added to the code which is not covered by the existent -Werror=foo -Werror=bar).

Also, to mitigate the "unusable in more scenarios" - it is easy to add --enable-suppress-external-warnings if the compilation stops due to a warning/error from non-bitcoin code.

I think this is hugely positive change, but yeah, maybe not well described in the PR title and description.

It would also be nice to know what the broader goal is here

The way I see it, it is "Catch maximum warnings, suppress as little as possible". By "catch" I mean "stop the build" because if there is a warning printout but the build succeeds, then most likely the developer will miss the warning printout or it will be lost and ignored in CI logs.

why are we disabling it entirely for a single warning, i.e in the win64 build, and losing the supposed benefits of having it on,

Notice: win64 builds did not use --enable-werror even before #20182, so we are not disabling anything.

rather than suppressing that single warning just in the CI? i.e adding CXXFLAGS=-Wno-... to BITCOIN_CONFIG

That seems like a good idea!

DrahtBot commented at 6:15 PM on December 6, 2020: member

Gitian builds

File	commit f35e4d906fee907f28ac5d8f32d4948e6b7b14c3<br>(master)	commit bab61ba28f80391a76efdc30ab97822eb99b010c<br>(master and this pull)
bitcoin-core-linux-22-res.yml	`93a54c6c76995531...`	`9d1be7acc5fafaf9...`
bitcoin-core-osx-22-res.yml	`fb74169ae20d2bcc...`	`a7ea63274ab8e107...`
bitcoin-core-win-22-res.yml	`9e8fb04a08e1ee32...`	`63f7da8f76fa9f1d...`
*-aarch64-linux-gnu-debug.tar.gz	`35a6ec9a2abfe0f6...`	`2e735f73f462e68a...`
*-aarch64-linux-gnu.tar.gz	`5bb75fd8ff5cd37e...`	`6fa3de7e0f12bcfe...`
*-arm-linux-gnueabihf-debug.tar.gz	`edbb0de1b45c12f9...`	`143ca7c2226abf48...`
*-arm-linux-gnueabihf.tar.gz	`a29e555e0dbcafd9...`	`1df0084436dd0020...`
*-osx-unsigned.dmg	`f87c6afaa3ff8664...`	`d7791f9171c744a4...`
*-osx64.tar.gz	`428053489a722919...`	`852d1336a9f39b32...`
*-riscv64-linux-gnu-debug.tar.gz	`a5feb66133132cc1...`	`b802356d073d945d...`
*-riscv64-linux-gnu.tar.gz	`151ac968ffd74947...`	`4f60af227f63f597...`
*-win64-debug.zip	`2e33c72742a86fea...`	`03b07b39c9c659a0...`
*-win64-setup-unsigned.exe	`ea81dbe5ec995035...`	`c56434f44af1cf54...`
*-win64.zip	`ed7867b2911eef24...`	`34a8039e05562df7...`
*-x86_64-linux-gnu-debug.tar.gz	`bbdd566b00894694...`	`9714fcc609c9296e...`
*-x86_64-linux-gnu.tar.gz	`983780b0eb3459ba...`	`3e507e14b8392e3f...`
*.tar.gz	`cf54eeba51ad26d3...`	`0a4db0ece73c9f1b...`
linux-build.log	`897e95c03365c222...`	`3809536816174237...`
osx-build.log	`1b3bbdb8fca00edb...`	`dbcfdfc39fe8c228...`
win-build.log	`5010ebcce7cf6070...`	`b3b69ab6120caa86...`
bitcoin-core-linux-22-res.yml.diff		`998e74f96e5788e5...`
bitcoin-core-osx-22-res.yml.diff		`db25f48537367d4d...`
bitcoin-core-win-22-res.yml.diff		`b852618cfa10bc77...`
linux-build.log.diff		`13592381e68b49e6...`
osx-build.log.diff		`d972fa6d5dfb5548...`
win-build.log.diff		`6d30fac7f841f687...`

DrahtBot removed the label Needs gitian build on Dec 6, 2020

DrahtBot commented at 1:40 AM on February 2, 2021: member

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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

DrahtBot added the label Needs rebase on Feb 2, 2021

hebasto marked this as a draft on Mar 28, 2021

hebasto commented at 12:03 PM on October 11, 2021: member

Closed in favor of #23149.

hebasto closed this on Oct 11, 2021

hebasto deleted the branch on Oct 11, 2021

fanquake referenced this in commit 39872f5ed4 on Oct 13, 2021

DrahtBot locked this on Oct 30, 2022