build: Do not repeat warning names in -Werror=... options #20544
Conversation
691168f to
5823be9
Compare
|
Can't do the |
CI fails without adding |
You are suggesting to reorder commits, right? @MarcoFalke are you agree? |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK: this redundancy is annoying :) |
This change also effectively activates -W... flags for builds with dependencies.
5823be9 to
2ed0a09
Compare
|
Rebased 5823be9 -> 2ed0a09 (pr20544.02 -> pr20544.03) due to the conflict with #20182. |
There was a problem hiding this comment.
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.
| dnl This is a temporary workaround. See https://github.com/bitcoin/bitcoin/pull/17919#issuecomment-674404780 | ||
| AX_CHECK_COMPILE_FLAG([-Wunused-command-line-argument],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-unused-command-line-argument"],,[[$CXXFLAG_WERROR]]) | ||
|
|
||
| dnl -Wmissing-braces is broken for aggregate initialization of std::array in Clang (where it is enabled by -Wall) before 5.0 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sorry for a typo. Should be "prior to 6.0". Going to fix it.
| @@ -435,6 +422,16 @@ if test "x$CXXFLAGS_overridden" = "xno"; then | |||
| AX_CHECK_COMPILE_FLAG([-Wdeprecated-register],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-register"],,[[$CXXFLAG_WERROR]]) | |||
| AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-implicit-fallthrough"],,[[$CXXFLAG_WERROR]]) | |||
| AX_CHECK_COMPILE_FLAG([-Wdeprecated-copy],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-copy"],,[[$CXXFLAG_WERROR]]) | |||
|
|
|||
| dnl This is a temporary workaround. See https://github.com/bitcoin/bitcoin/pull/17919#issuecomment-674404780 | |||
There was a problem hiding this comment.
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.
While that is true, strictly speaking, there is also another side to it: Also, to mitigate the "unusable in more scenarios" - it is easy to add I think this is hugely positive change, but yeah, maybe not well described in the PR title and description.
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.
Notice: win64 builds did not use
That seems like a good idea! |
Gitian builds
|
|
🐙 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". |
|
Closed in favor of #23149. |
38fd709 build: make --enable-werror just -Werror (fanquake) Pull request description: No longer special case a set of warnings, to make up our own -Werror, just use -Werror outright. This shouldn't really have any effect on existing builders, who were already using `--enable-werror`, and is more inline with what they would expect `--enable-werror` to be, which is erroring on any/all warnings. We keep `-Wno-error=return-type` because we know that is broken when using mingw-w64. It should only be applied when cross-compiling for Windows. Similar to the change in #20544, but with (hopefully) less work-arounds, and other bundled changes. A step towards some configure "cleanups". ACKs for top commit: hebasto: ACK 38fd709 (also see #23149 (comment)), tested: MarcoFalke: Concept ACK 38fd709 Tree-SHA512: 37f1857d9408442cab63e40f9280427b73e09cdf03146b19c1339d7e44abd78e93df7f270ca1da0e83b79343cd3ea915f7b9e4e347488b5bc5ceaaa7540e5926
This PR:
configure.acmaintainability by removing compiler warning/error names duplicationClose #18092.
Split from #20182 as requested by MarcoFalke,
and is required for it.