[MERGED] libasn1c[master]: configure: add --enable-werror
Neels Hofmeyr has submitted this change and it was merged. Change subject: configure: add --enable-werror .. configure: add --enable-werror Provide a sane means of adding the -Werror compiler flag. Currently, some of our jenkins.sh add -Werror by passing 'CFLAGS="-Werror"', but that actually *overwrites* all the other CFLAGS we might want to have set. Maintain these exceptions from -Werror: a) deprecation (allow upstream to mark deprecation without breaking builds); b) "#warning" pragmas (allow to remind ourselves of errors without breaking builds) As a last configure step before generating the output files, print the complete CFLAGS and CPPFLAGS by means of AC_MSG_RESULT. Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c --- M configure.ac 1 file changed, 21 insertions(+), 0 deletions(-) Approvals: Neels Hofmeyr: Looks good to me, approved Jenkins Builder: Verified diff --git a/configure.ac b/configure.ac index 4d65c2f..1610973 100644 --- a/configure.ac +++ b/configure.ac @@ -29,6 +29,24 @@ CPPFLAGS="$CPPFLAGS -fsanitize=address -fsanitize=undefined" fi +AC_ARG_ENABLE(werror, + [AS_HELP_STRING( + [--enable-werror], + [Turn all compiler warnings into errors, with exceptions: +a) deprecation (allow upstream to mark deprecation without breaking builds); +b) "#warning" pragmas (allow to remind ourselves of errors without breaking builds) + ] + )], + [werror=$enableval], [werror="no"]) +if test x"$werror" = x"yes" +then + WERROR_FLAGS="-Werror" + WERROR_FLAGS+=" -Wno-error=deprecated -Wno-error=deprecated-declarations" + WERROR_FLAGS+=" -Wno-error=cpp" # "#warning" + CFLAGS="$CFLAGS $WERROR_FLAGS" + CPPFLAGS="$CPPFLAGS $WERROR_FLAGS" +fi + # The following test is taken from WebKit's webkit.m4 saved_CFLAGS="$CFLAGS" CFLAGS="$CFLAGS -fvisibility=hidden " @@ -44,6 +62,9 @@ [build_debug="$enableval"], [build_debug="no"]) AM_CONDITIONAL(BUILD_DEBUG, test "x$build_debug" = "xyes") +AC_MSG_RESULT([CFLAGS="$CFLAGS"]) +AC_MSG_RESULT([CPPFLAGS="$CPPFLAGS"]) + AC_OUTPUT( libasn1c.pc src/Makefile -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol
libasn1c[master]: configure: add --enable-werror
Patch Set 1: Code-Review+2 marking +2 because I'm (barely) allowed to by the comment at https://gerrit.osmocom.org/7096 (03-09 13:04 CET) -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
libasn1c[master]: configure: add --enable-werror
Patch Set 1: well, as nobody is proposign any feasible alternative, I'm inclined to say "go ahead mark all of those as +2 and merge it'. I still don't like it, but I don't have time for a counter-proposal either. -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
libasn1c[master]: configure: add --enable-werror
Patch Set 1: > So clearly =pragma is wrong Ah, you meant, not suppress #pragma, but use #pragma instead of #warning. Ok, possible, but with -Wno-error=cpp we can also keep the #warnings :) -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
libasn1c[master]: configure: add --enable-werror
Patch Set 1: > Regarding deprecated, I think it's good that it fails when using a > deprecated symbol, this way it forces us to fix it. That part is good, yes, but the pattern that has emerged recently is: our jenkins builds use -Werror. That means as soon as we create some foo2() and deprecate foo(), all of the builds that still use foo() flare up red. It's still ok for that other project to use foo(), we haven't yet seen a need for it to move the newer API and use the new, safer features / the clarified signature / the more universal application. So what do I do? I *don't* mark foo() as deprecated. I make a note in my mind to first look at all the dependant projects and get rid of foo(), and *then* mark it deprecated. And then that never happens. The conclusion is we will refrain from marking anything deprecated if our builds fail because of deprecation warnings. So my opinion is quite strongly against failing for deprecation. We really need all those other warnings as errors, yes, deprecation as error breaks the workflow. > Regarding the warning pragmas, I'd bet -Wno-error=cpp also disabled > other C preprocessor warnings which we may not want to disable. If > you want to add explicit warning messages to show during the build, > use #pragma message instead. man gcc: -Wno-cpp (C, Objective-C, C++, Objective-C++ and Fortran only) Suppress warning messages emitted by "#warning" directives. and -Wno-pragmas Do not warn about misuses of pragmas, such as incorrect parameters, invalid syntax, or conflicts between pragmas. See also -Wunknown-pragmas. So clearly =pragma is wrong and =cpp has exactly the desired effect. (Unless the manpage is wrong.) -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
libasn1c[master]: configure: add --enable-werror
Patch Set 1: > > Why do you need each of this? > > -Wno-error=deprecated -Wno-error=deprecated-declarations > > -Wno-error=cpp > > it's all explained in the commit log as well as the ./configure > --help Regarding deprecated, I think it's good that it fails when using a deprecated symbol, this way it forces us to fix it. Regarding the warning pragmas, I'd bet -Wno-error=cpp also disabled other C preprocessor warnings which we may not want to disable. If you want to add explicit warning messages to show during the build, use #pragma message instead. -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
libasn1c[master]: configure: add --enable-werror
Patch Set 1: > Why do you need each of this? > -Wno-error=deprecated -Wno-error=deprecated-declarations > -Wno-error=cpp it's all explained in the commit log as well as the ./configure --help -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
libasn1c[master]: configure: add --enable-werror
Patch Set 1: -Code-Review Why do you need each of this? -Wno-error=deprecated -Wno-error=deprecated-declarations -Wno-error=cpp -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
libasn1c[master]: configure: add --enable-werror
Patch Set 1: > I would really hope there is another method than to add a > configure.ac flag for every compiler option / CFLAG we ever want to > sue from our continuous integration setup. It's not just one CFLAG here, there is a "fine-tuned" combination of warnings we don't want to see as errors: -Werror -Wno-error=deprecated -Wno-error=deprecated-declarations -Wno-error=cpp We can put it in all the jenkins.sh files, sure. But I also want to be able to conveniently test this locally. So what do I do, copy-paste from the jenkins.sh? For me what works best is the explicit ./configure argument, since (personally) I already have some infrastructure around to manage configure arguments (in osmo-dev.git). So I think yes, this is the right place. * It's about compilation = the realm of configure.ac * We already adjust a lot of CFLAGS in configure.ac. * I can conveniently run the exact -Werror definitions that we use to pass or reject a new patch, by just remembering --enable-werror. * ./configure --help provides "online doc" for -Werror and gives an explanation of why certain flags are set. * It's not harmful to have that option. (I'd actually prefer to have them defined only once, like in libosmocore, for all other projects, but we don't have a configure.ac sharing mechanism in place.) Do you guys have another suggestion that gets me similar convenience? -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
libasn1c[master]: configure: add --enable-werror
Patch Set 1: Code-Review-1 I think using: CFLAGS="..." ./configure instead of ./configure CFLAGS="..." should fix the overwritting issue. -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
libasn1c[master]: configure: add --enable-werror
Patch Set 1: I would really hope there is another method than to add a configure.ac flag for every compiler option / CFLAG we ever want to sue from our continuous integration setup. I know, it's easy to complain and if it helps, ok. But it just feels like the wrong approach to me, sorry. -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: No
[PATCH] libasn1c[master]: configure: add --enable-werror
Review at https://gerrit.osmocom.org/7096 configure: add --enable-werror Provide a sane means of adding the -Werror compiler flag. Currently, some of our jenkins.sh add -Werror by passing 'CFLAGS="-Werror"', but that actually *overwrites* all the other CFLAGS we might want to have set. Maintain these exceptions from -Werror: a) deprecation (allow upstream to mark deprecation without breaking builds); b) "#warning" pragmas (allow to remind ourselves of errors without breaking builds) As a last configure step before generating the output files, print the complete CFLAGS and CPPFLAGS by means of AC_MSG_RESULT. Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c --- M configure.ac 1 file changed, 21 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libasn1c refs/changes/96/7096/1 diff --git a/configure.ac b/configure.ac index 4d65c2f..1610973 100644 --- a/configure.ac +++ b/configure.ac @@ -29,6 +29,24 @@ CPPFLAGS="$CPPFLAGS -fsanitize=address -fsanitize=undefined" fi +AC_ARG_ENABLE(werror, + [AS_HELP_STRING( + [--enable-werror], + [Turn all compiler warnings into errors, with exceptions: +a) deprecation (allow upstream to mark deprecation without breaking builds); +b) "#warning" pragmas (allow to remind ourselves of errors without breaking builds) + ] + )], + [werror=$enableval], [werror="no"]) +if test x"$werror" = x"yes" +then + WERROR_FLAGS="-Werror" + WERROR_FLAGS+=" -Wno-error=deprecated -Wno-error=deprecated-declarations" + WERROR_FLAGS+=" -Wno-error=cpp" # "#warning" + CFLAGS="$CFLAGS $WERROR_FLAGS" + CPPFLAGS="$CPPFLAGS $WERROR_FLAGS" +fi + # The following test is taken from WebKit's webkit.m4 saved_CFLAGS="$CFLAGS" CFLAGS="$CFLAGS -fvisibility=hidden " @@ -44,6 +62,9 @@ [build_debug="$enableval"], [build_debug="no"]) AM_CONDITIONAL(BUILD_DEBUG, test "x$build_debug" = "xyes") +AC_MSG_RESULT([CFLAGS="$CFLAGS"]) +AC_MSG_RESULT([CPPFLAGS="$CPPFLAGS"]) + AC_OUTPUT( libasn1c.pc src/Makefile -- To view, visit https://gerrit.osmocom.org/7096 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ibcc8238584a585434b39a046cd2d7e18ddaf7f8c Gerrit-PatchSet: 1 Gerrit-Project: libasn1c Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr