This PR:
- improves the
configure.acmaintainability 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~.
build: Do not repeat warning names in -Werror=… options #20544
pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:201202-werror changing 1 files +14 −17-
hebasto commented at 8:51 am on December 2, 2020: member
-
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: memberCan’t do the
configure.acchanges 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.acchanges 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.acchanges 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: contributorConcept 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
-
MarcoFalke deleted a comment on Dec 4, 2020
-
2ed0a09bae
build: Do not repeat warning names in -Werror=... options
This change also effectively activates -W... flags for builds with dependencies.
-
hebasto force-pushed on Dec 4, 2020
-
hebasto commented at 7:37 pm on December 4, 2020: memberRebased 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:
vasild commented at 3:12 pm on December 5, 2020:Thanks!vasild approvedvasild commented at 3:02 pm on December 5, 2020: memberACK 2ed0a09bDrahtBot commented at 2:00 am on December 6, 2020: memberGuix builds
DrahtBot removed the label Needs Guix build on Dec 6, 2020in 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_requestedfanquake commented at 6:54 am on December 6, 2020: memberCan 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-werroroption from being a select set of-Werror=xcases that we’ve added to over time, i.e-Werror=date-time, to just being-Werrorwholesale. As a result,--enable-werrorwould 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 inWARN_CXXFLAGSis 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-...toBITCOIN_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 whetherCXXFLAGShas 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-werrorwould 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-werrorwould 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-warningsif 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-werroreven 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: memberGitian builds
DrahtBot removed the label Needs gitian build on Dec 6, 2020DrahtBot commented at 1:40 am on February 2, 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”.
DrahtBot added the label Needs rebase on Feb 2, 2021hebasto marked this as a draft on Mar 28, 2021hebasto closed this on Oct 11, 2021
hebasto deleted the branch on Oct 11, 2021fanquake referenced this in commit 39872f5ed4 on Oct 13, 2021DrahtBot locked this on Oct 30, 2022
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
