Building with -Werror makes a lot of sense for development, but not a lot for distro deployments. Changes in default flag settings, versions of the compiler or C library, or 3rd party libraries can easily trigger harmless warnings that are a waste of time to track down. Especially when elfutils itself enables many extra -W flags. So add a configure flag for distros to utilize when building.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- ChangeLog | 5 +++++ config/eu.am | 2 +- configure.ac | 8 ++++++++ 3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog index a0ce570..3ccd2a9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2014-01-02 Mike Frysinger vapier@gentoo.org + + * configure.ac: Add --disable-werror flag. Export WERROR. + * config/eu.am (AM_CFLAGS): Change -Werror to $(WERROR). + 2013-12-20 Mark Wielaard mjw@redhat.com
* NEWS (libdwfl): Add dwfl_getthread_frames. diff --git a/config/eu.am b/config/eu.am index 38718c7..220c843 100644 --- a/config/eu.am +++ b/config/eu.am @@ -32,7 +32,7 @@ DEFS = -D_GNU_SOURCE -DHAVE_CONFIG_H -DLOCALEDIR='"${localedir}"' AM_CPPFLAGS = -I. -I$(srcdir) -I$(top_srcdir)/lib -I.. AM_CFLAGS = -std=gnu99 -Wall -Wshadow \ - $(if $($(*F)_no_Werror),,-Werror) \ + $(if $($(*F)_no_Werror),,$(WERROR)) \ $(if $($(*F)_no_Wunused),,-Wunused -Wextra) \ $(if $($(*F)_no_Wformat),-Wno-format,-Wformat=2) \ $($(*F)_CFLAGS) diff --git a/configure.ac b/configure.ac index 72fb3e8..f0fe890 100644 --- a/configure.ac +++ b/configure.ac @@ -48,6 +48,14 @@ AC_CONFIG_FILES([elfutils.spec:config/elfutils.spec.in])
AC_CANONICAL_HOST
+AC_ARG_ENABLE(werror, +[AS_HELP_STRING([--disable-werror], + [turn off -Werror @<:@default=enabled@:>@])]) +if test "x$enable_werror" != "xno"; then + WERROR="-Werror" +fi +AC_SUBST(WERROR) + AC_ARG_ENABLE(deterministic-archives, [AS_HELP_STRING([--enable-deterministic-archives], [ar and ranlib default to -D behavior])], [
On Thu, Jan 02, 2014 at 06:28:14AM -0500, Mike Frysinger wrote:
Building with -Werror makes a lot of sense for development, but not a lot for distro deployments. Changes in default flag settings, versions of the compiler or C library, or 3rd party libraries can easily trigger harmless warnings that are a waste of time to track down. Especially when elfutils itself enables many extra -W flags. So add a configure flag for distros to utilize when building.
Please do report any issues you find with -Werror. We explicitly enable specific -W flags to catch issues. With a modern GNU toolchain the codebase really should be warning/error free with the default flags. If not, that is certainly a bug.
For very old setups there is the portable branch and patch set that does include a --disable-werror option (which you contributed). But in general that should not be needed.
Cheers,
Mark
hmm, it seems i've written & sent this patch before, and it's been merged into the portability patchset already (although it's rotted and needs updating). so i'll update that branch.
i certainly agree with using -Werror in builds out of git, but imnsho elfutils is still being unreasonably unrealistic when it comes to releases. no distro ships elfutils w/out the portability/robustify patches applied which means no one in the world is testing the tarballs that the project is releasing. it's ridiculous that both patches continue to exist as separate branches.
but it seems that i'm the only one of this opinion, so the status quo isn't going to change. -mike
On Thu, Jan 02, 2014 at 07:44:48AM -0500, Mike Frysinger wrote:
no distro ships elfutils w/out the portability/robustify patches applied which means no one in the world is testing the tarballs that the project is releasing. it's ridiculous that both patches continue to exist as separate branches.
but it seems that i'm the only one of this opinion, so the status quo isn't going to change.
Actually I agree with at least half of what you are saying. For the next release (after 0.158 is out) I'll make an effort to merge the robustify patches to mainline. But the portability branch really is only needed for ancient toolchains and can contain stuff that is inefficient on a modern GNU/Linux setup. It isn't used in fedora >= 9 or rhel >= 6 for example.
Cheers,
Mark
On Thursday 02 January 2014 16:48:38 Mark Wielaard wrote:
On Thu, Jan 02, 2014 at 07:44:48AM -0500, Mike Frysinger wrote:
no distro ships elfutils w/out the portability/robustify patches applied which means no one in the world is testing the tarballs that the project is releasing. it's ridiculous that both patches continue to exist as separate branches.
but it seems that i'm the only one of this opinion, so the status quo isn't going to change.
Actually I agree with at least half of what you are saying. For the next release (after 0.158 is out) I'll make an effort to merge the robustify patches to mainline. But the portability branch really is only needed for ancient toolchains and can contain stuff that is inefficient on a modern GNU/Linux setup. It isn't used in fedora >= 9 or rhel >= 6 for example.
the majority of those changes are just for old toolchains (looks like gcc-4.3 is the oldest version that'll work, but that's what we've been requiring in glibc for a few releases and no one seems to mind, so shouldn't be an issue).
i think these belong in the main branch though:
35ae9cf964c2de52bb4d03412334fe83338393ee - adds an abort() after an assert() if you build with -DNDEBUG, the assert() is a NOP, and the code can continue running when it clearly shouldn't.
a3462c319770a035e8a1deed5b49f5a0d95c5a2b - support systems that lack tls probably not terribly important as every arch that is supported by current glibc has tls support now, but not every C library supports tls. plus, this change has like no overhead for the project (it's a minor tweak to configure.ac only).
5f96b96cba442bf0694c08489c3961f95fd69d7e - add support for --disable-werror (a refreshed version which started this thread) pretty much every other project (GNU or otherwise) which leverages -Werror during build has this type of flag. you can't control the default compile flags that users/distros use, and many of those can easily trigger warnings that you don't expect (or don't even want to handle because they're largely harmless). it also doesn't change the default behavior for anyone. -mike
On Thu, 2014-01-02 at 17:48 -0500, Mike Frysinger wrote:
the majority of those changes are just for old toolchains (looks like gcc-4.3 is the oldest version that'll work, but that's what we've been requiring in glibc for a few releases and no one seems to mind, so shouldn't be an issue).
i think these belong in the main branch though:
35ae9cf964c2de52bb4d03412334fe83338393ee
- adds an abort() after an assert()
if you build with -DNDEBUG, the assert() is a NOP, and the code can continue running when it clearly shouldn't.
More stuff breaks when you build with -DNDEBUG. Are you sure we want to support that setup? In general we seem to use assert and/or abort a lot for non-reachable code paths. It would be nice to have a better solution for that, especially in places where the compiler would be able to check that assertion. I am not sure what the correct way to express that is though.
a3462c319770a035e8a1deed5b49f5a0d95c5a2b
- support systems that lack tls
probably not terribly important as every arch that is supported by current glibc has tls support now, but not every C library supports tls. plus, this change has like no overhead for the project (it's a minor tweak to configure.ac only).
That is for the unsupported configure option --enable-thread-safety for which we already warn it should not be used.
Cheers,
Mark
On Thursday, January 23, 2014 14:54:53 Mark Wielaard wrote:
On Thu, 2014-01-02 at 17:48 -0500, Mike Frysinger wrote:
the majority of those changes are just for old toolchains (looks like gcc-4.3 is the oldest version that'll work, but that's what we've been requiring in glibc for a few releases and no one seems to mind, so shouldn't be an issue).
i think these belong in the main branch though:
35ae9cf964c2de52bb4d03412334fe83338393ee
- adds an abort() after an assert()
if you build with -DNDEBUG, the assert() is a NOP, and the code can continue running when it clearly shouldn't.
More stuff breaks when you build with -DNDEBUG. Are you sure we want to support that setup? In general we seem to use assert and/or abort a lot for non-reachable code paths. It would be nice to have a better solution for that, especially in places where the compiler would be able to check that assertion. I am not sure what the correct way to express that is though.
newer glibc/gcc versions have static_assert() which should work even w/NDEBUG. if there are no plans to support running with asserts turned off, then maybe add a #error when that is defined ?
a3462c319770a035e8a1deed5b49f5a0d95c5a2b
- support systems that lack tls
probably not terribly important as every arch that is supported by current glibc has tls support now, but not every C library supports tls. plus, this change has like no overhead for the project (it's a minor tweak to configure.ac only).
That is for the unsupported configure option --enable-thread-safety for which we already warn it should not be used.
then it should be easy to merge as you won't be breaking anything new :). as for warning, i think the code really only says "hey tests might break". that's not terribly strong if the project really doesn't want people using it. maybe change -mike
elfutils-devel@lists.fedorahosted.org