ci: avoid using -Werror for older compilers #27013

1. 
   fanquake commented at 11:49 am on February 1, 2023: member

   Don’t enable -Werror (in the CI) for compilers at least older than our current release compiler (GCC 10). It provides little-to-no value, other than turning compiler bugs & false positives into build failures, and we aren’t going to mutate perfectly fine/working code, for the sake of avoid a warning that shouldn’t even exist.

   I also do not see the point of playing whack-a-mole and turning off various warnings/trying to further work around the broken compiler, just to acheive warningless builds for the sake of warningless builds.

   One anecdote from “How SQLite Is Tested”:

   Static analysis has found a few bugs in SQLite, but those are the exceptions. More bugs have been introduced into SQLite while trying to get it to compile without warnings than have been found by static analysis.

2. 
   DrahtBot commented at 11:49 am on February 1, 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, jarolrod
   Concept ACK	Sjors

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #25797 (build: Add CMake-based build system 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.

3. DrahtBot added the label Tests on Feb 1, 2023
4. 
   maflcko commented at 11:57 am on February 1, 2023: member

   Thanks, this is a lot more acceptable than applying wrong changes to the code (https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-1401810209)
5. maflcko approved
6. 
   maflcko commented at 11:57 am on February 1, 2023: member

   lgtm
7. 
   hebasto commented at 12:15 pm on February 1, 2023: member

   Concept ACK on:

   Don’t enable -Werror (in the CI) for compilers at least older than our current release compiler (GCC 10).

   Was thinking about the same approach in the context of #25972.

8. in ci/test/00_setup_env_i686_centos.sh:15 in ddc139dd48 outdated

   11@@ -12,6 +12,7 @@ export CI_IMAGE_NAME_TAG=quay.io/centos/centos:stream8
   12 export CI_BASE_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache libtool make git python38 python38-pip which patch lbzip2 xz procps-ng dash rsync coreutils bison"
   13 export PIP_PACKAGES="pyzmq"
   14 export GOAL="install"
   15+export NO_WERROR=1
   

   ---

   ---

   

   hebasto commented at 12:21 pm on February 1, 2023:

   While it is obvious how old a compiler is in other tasks with NO_WERROR=1, that is not the case in here. Maybe comment a compiler’s version in this task?

   ---

   

   maflcko commented at 12:58 pm on February 1, 2023:

   Yeah, I guess could just append a comment to explain # due to old compiler?

   ---

   

   fanquake commented at 3:22 pm on February 1, 2023:

   Added GCC 8 here.
9. 
   Sjors commented at 2:10 pm on February 1, 2023: member

   Concept ACK

   Can you add the specific release and CI compiler versions to the comment(s), so we remember to update it when either the CI compiler is upgraded or we bump the release compiler. Once reminded, the commit message provides enough context.

10. ci: avoid using -Werror for older compilers

    Don't enable `-Werror` (in the CI) for compilers at least older than
    our current release compiler (GCC 10). It provides little-to-no value,
    other than turning compiler bugs & false positives into build failures,
    and we aren't going to mutate perfectly fine/working code, for the sake
    of avoid a warning that shouldn't even exist.
    
    I also do not see the point of playing whack-a-mole and turning off various
    warnings/trying to further work around the broken compiler, just to
    acheive warningless builds for the sake of warningless builds.
    
    One anecdote from "How SQLite Is Tested":
    > Static analysis has found a few bugs in SQLite, but those are the
    > exceptions. More bugs have been introduced into SQLite while trying
    > to get it to compile without warnings than have been found by static
    > analysis.
    
    https://www.sqlite.org/testing.html.

    71383f2fad
11. fanquake force-pushed on Feb 1, 2023
12. 
    fanquake commented at 3:25 pm on February 1, 2023: member

    Can you add the specific release and CI compiler versions to the comment(s),

    It’s clear from the install lines what the compiler version is (except for the single case where a comment has been added), and the version specifics of GCC 8.x vs 8.y doesn’t matter. There’s no need to list the release compiler being used anywhere here in the CI config.

13. hebasto approved
14. 
    hebasto commented at 4:20 pm on February 1, 2023: member

    ACK 71383f2fad065378393ef55b6d65e14c656b7301
15. 
    Sjors commented at 6:32 pm on February 1, 2023: member

    There’s no need to list the release compiler being used anywhere here in the CI config.

    The problem I foresee is that we’ll forget to unset NO_WERROR=1 when updating the CI config. A comment serves as a reminder to reconsider that variable.

16. jarolrod approved
17. 
    jarolrod commented at 3:49 am on February 2, 2023: member

    ACK 71383f2fad065378393ef55b6d65e14c656b7301
18. maflcko merged this on Feb 2, 2023
19. maflcko closed this on Feb 2, 2023

    ---

20. fanquake deleted the branch on Feb 2, 2023
21. sidhujag referenced this in commit 6e979623be on Feb 3, 2023
22. bitcoin locked this on Feb 2, 2024

Contributors
fanquake DrahtBot maflcko hebasto Sjors jarolrod

Labels
Tests