ci: Set -Werror for all CI builds

real-or-random commented at 12:03 pm on May 6, 2021: contributor

Let’s see if that passes…

real-or-random commented at 1:33 pm on May 6, 2021: contributor

Uff ok, this is failing for multiple reasons:

setting CFLAGS=-Werror during configure is a bad idea because then all the compilation checks run by configure use this and produce wrong results (these are the DISTCHECK and x86 asm failures)
the mingw libtool build produces warnings that are not our responsibility, see #923…

Fixing the first item is a little bit annoying. We could simply patch the generated Makefile but that’s pretty ugly. So we’ll better add some logic.

It would be nice to call ./configure normally but then make CFLAGS=-Werror but this would override all the other settings in CFLAGS. According to https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html, it would anyway be cleaner to set our -Wall etc in AM_CFLAGS but even then CFLAGS=-Werror would override -O2 for example.

I believe a better way would be to have some maintainer variable with additional flags that can be set from the outside like make EXTRACFLAGS=-Werror. Or have some --enable-werror or --enable-maintainer-mode for configure. I’ll probably try one of those options. Happy to hear opinions.

gmaxwell commented at 5:07 am on May 9, 2021: contributor

An enable-werror is the sort of thing I’d expect (maintainer mode sounds like it would get confused with AM_MAINTAINER_MODE).

The only concern I have with EXTRACFLAGS is just that it gets into my environment and then messes up my tests but is weird enough that I don’t think to look for it. :P Is there any particular reason it needs to rerun both ways such that rerunning configure is a concern?

However its setup it should be done in a way that makes it relatively easy to drop on some CI builds. Because if some build starts throwing false positives it may be best to ignore it (e.g. if it’s S390 or some other weird config and silencing it requires hurting performance or crapping up the codebase), or maybe not, but the decision shouldn’t be driven by having to all-or-nothing the Werror or dropping the build.

real-or-random force-pushed on May 12, 2021

real-or-random commented at 9:43 am on May 12, 2021: contributor

The only concern I have with EXTRACFLAGS is just that it gets into my environment and then messes up my tests but is weird enough that I don’t think to look for it.

I ended up doing this (naming it WERROR_CFLAGS) because we anyway need some free text variable for specifying fine-grained -Wno-error flags, and having a user-facing configure option is not tremendously helpful anyway. If users want -Werror they can always set CFLAGS.

Let’s see what Cirrus says.

build: Use own variable SECP_CFLAGS instead of touching user CFLAGS

Fixes one of the items in #923, namely the warnings of the form
    '_putenv' redeclared without dllimport attribute:
    previous dllimport ignored [-Wattributes]

This also cleans up the way we add CFLAGS, in particular flags enabling
warnings. Now we perform some more fine-grained checking for flag
support, which is not strictly necessary but the changes also help to
document autoconf.ac.

721928361d

build: Enable -Wcast-align=strict warning 98158d4fc7

build: List *CPPFLAGS before *CFLAGS like on the compiler command line 303a28188f

build: Ensure that configure's compile checks default to -O2

Fixes #896.

595efa0104

build: Add hidden WERROR_CFLAGS variable for testing and CI 1b6bef4006

ci: Make compiler warning into errors on CI

This also tidies the list of environment variables in .cirrus.yml.

8d8cd482e1

real-or-random force-pushed on May 13, 2021

real-or-random closed this on May 13, 2021

real-or-random commented at 7:38 pm on May 13, 2021: contributor

Closed in favor of https://github.com/bitcoin-core/secp256k1/pull/944

real-or-random cross-referenced this on May 13, 2021 from issue Various improvements related to CFLAGS by real-or-random

ci: Set -Werror for all CI builds #938