[MERGED] libasn1c[master]: configure: add --enable-werror

[Neels Hofmeyr]

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

[Harald Welte]

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

[Neels Hofmeyr]

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

[Neels Hofmeyr]

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

[Pau Espin Pedrol]

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

[Neels Hofmeyr]

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

[Pau Espin Pedrol]

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

[Neels Hofmeyr]

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

[Pau Espin Pedrol]

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

[Harald Welte]

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

[Neels Hofmeyr]

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