| Message ID | 09445d1715453b2eff4399da998717b967b829b3.1423739602.git.pmatilai@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 1F7BB5A99; Thu, 12 Feb 2015 12:13:30 +0100 (CET) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id EB8B65A97 for <dev@dpdk.org>; Thu, 12 Feb 2015 12:13:28 +0100 (CET) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t1CBDR7t007435 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for <dev@dpdk.org>; Thu, 12 Feb 2015 06:13:27 -0500 Received: from localhost.localdomain.com (vpn1-5-97.ams2.redhat.com [10.36.5.97]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t1CBDQ8w008798 for <dev@dpdk.org>; Thu, 12 Feb 2015 06:13:27 -0500 From: Panu Matilainen <pmatilai@redhat.com> To: dev@dpdk.org Date: Thu, 12 Feb 2015 13:13:22 +0200 Message-Id: <09445d1715453b2eff4399da998717b967b829b3.1423739602.git.pmatilai@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 Subject: [dpdk-dev] [PATCH] Make -Werror optional X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Panu Matilainen
Feb. 12, 2015, 11:13 a.m. UTC
This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable fail-on-warning compile behavior, defaulting to off. Failing build on warnings is a useful developer tool but its bad for release tarballs which can and do get built with newer compilers than what was used/available during development. Compilers routinely add new warnings so code which built silently with cc X might no longer do so with X+1. This doesn't make the existing code any more buggier and failing the build in this case does not help not help improve code quality of an already released version either. --- config/common_bsdapp | 1 + config/common_linuxapp | 1 + mk/toolchain/clang/rte.vars.mk | 5 ++++- mk/toolchain/gcc/rte.vars.mk | 5 ++++- mk/toolchain/icc/rte.vars.mk | 6 +++++- 5 files changed, 15 insertions(+), 3 deletions(-)
Comments
On Thu, Feb 12, 2015 at 01:13:22PM +0200, Panu Matilainen wrote: > This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable > fail-on-warning compile behavior, defaulting to off. > > Failing build on warnings is a useful developer tool but its bad > for release tarballs which can and do get built with newer > compilers than what was used/available during development. Compilers > routinely add new warnings so code which built silently with cc X > might no longer do so with X+1. This doesn't make the existing code > any more buggier and failing the build in this case does not help > not help improve code quality of an already released version either. This can already be achieve by passing EXTRA_CFLAGS='-Wno-error' into the build command. I don't like changing the default option here. Better to instead document how to disable the warning flags if necessary. /Bruce
On 02/12/2015 01:25 PM, Bruce Richardson wrote: > On Thu, Feb 12, 2015 at 01:13:22PM +0200, Panu Matilainen wrote: >> This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable >> fail-on-warning compile behavior, defaulting to off. >> >> Failing build on warnings is a useful developer tool but its bad >> for release tarballs which can and do get built with newer >> compilers than what was used/available during development. Compilers >> routinely add new warnings so code which built silently with cc X >> might no longer do so with X+1. This doesn't make the existing code >> any more buggier and failing the build in this case does not help >> not help improve code quality of an already released version either. > > This can already be achieve by passing EXTRA_CFLAGS='-Wno-error' into the > build command. I don't like changing the default option here. Better to > instead document how to disable the warning flags if necessary. Well, optimally it would only default to off in released versions, which is where the Werror behavior is just annoying without being useful. For a practical example of how silly this can be: just got a build failure with 1.8 due to "variable set but not used" warnings, because gcc 5 is "smarter" and finds couple of cases older versions did not. So everybody using gcc 5 to build the just-released 1.8 will be required to hunt down and pass in that extra disabler, for no good reason. - Panu -
On Thu, Feb 12, 2015 at 02:02:19PM +0200, Panu Matilainen wrote: > On 02/12/2015 01:25 PM, Bruce Richardson wrote: > >On Thu, Feb 12, 2015 at 01:13:22PM +0200, Panu Matilainen wrote: > >>This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable > >>fail-on-warning compile behavior, defaulting to off. > >> > >>Failing build on warnings is a useful developer tool but its bad > >>for release tarballs which can and do get built with newer > >>compilers than what was used/available during development. Compilers > >>routinely add new warnings so code which built silently with cc X > >>might no longer do so with X+1. This doesn't make the existing code > >>any more buggier and failing the build in this case does not help > >>not help improve code quality of an already released version either. > > > >This can already be achieve by passing EXTRA_CFLAGS='-Wno-error' into the > >build command. I don't like changing the default option here. Better to > >instead document how to disable the warning flags if necessary. > > Well, optimally it would only default to off in released versions, which is > where the Werror behavior is just annoying without being useful. This I can agree with. /Bruce > > For a practical example of how silly this can be: just got a build failure > with 1.8 due to "variable set but not used" warnings, because gcc 5 is > "smarter" and finds couple of cases older versions did not. > So everybody using gcc 5 to build the just-released 1.8 will be required to > hunt down and pass in that extra disabler, for no good reason. > > - Panu - >
On Thu, 12 Feb 2015 13:13:22 +0200 Panu Matilainen <pmatilai@redhat.com> wrote: > This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable > fail-on-warning compile behavior, defaulting to off. > > Failing build on warnings is a useful developer tool but its bad > for release tarballs which can and do get built with newer > compilers than what was used/available during development. Compilers > routinely add new warnings so code which built silently with cc X > might no longer do so with X+1. This doesn't make the existing code > any more buggier and failing the build in this case does not help > not help improve code quality of an already released version either. > --- > config/common_bsdapp | 1 + > config/common_linuxapp | 1 + > mk/toolchain/clang/rte.vars.mk | 5 ++++- > mk/toolchain/gcc/rte.vars.mk | 5 ++++- > mk/toolchain/icc/rte.vars.mk | 6 +++++- > 5 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/config/common_bsdapp b/config/common_bsdapp > index 57bacb8..a5687b3 100644 > --- a/config/common_bsdapp > +++ b/config/common_bsdapp > @@ -362,6 +362,7 @@ CONFIG_RTE_KNI_VHOST_DEBUG_TX=n > # Enable warning directives > # > CONFIG_RTE_INSECURE_FUNCTION_WARNING=n > +CONFIG_RTE_ERROR_ON_WARNING=n > > # > # Compile the test application > diff --git a/config/common_linuxapp b/config/common_linuxapp > index d428f84..0762f99 100644 > --- a/config/common_linuxapp > +++ b/config/common_linuxapp > @@ -383,6 +383,7 @@ CONFIG_RTE_LIBRTE_XEN_DOM0=n > # Enable warning directives > # > CONFIG_RTE_INSECURE_FUNCTION_WARNING=n > +CONFIG_RTE_ERROR_ON_WARNING=n > > # > # Compile the test application > diff --git a/mk/toolchain/clang/rte.vars.mk b/mk/toolchain/clang/rte.vars.mk > index 40cb389..12726e7 100644 > --- a/mk/toolchain/clang/rte.vars.mk > +++ b/mk/toolchain/clang/rte.vars.mk > @@ -63,11 +63,14 @@ TOOLCHAIN_ASFLAGS = > TOOLCHAIN_CFLAGS = > TOOLCHAIN_LDFLAGS = > > -WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes > +WERROR_FLAGS := -W -Wall -Wstrict-prototypes -Wmissing-prototypes > WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith > WERROR_FLAGS += -Wnested-externs -Wcast-qual > WERROR_FLAGS += -Wformat-nonliteral -Wformat-security > WERROR_FLAGS += -Wundef -Wwrite-strings > +ifeq ($(CONFIG_RTE_ERROR_ON_WARNING),y) > +WERROR_FLAGS += -Werror > +endif > > # process cpu flags > include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk > diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk > index 88f235c..bbd3c85 100644 > --- a/mk/toolchain/gcc/rte.vars.mk > +++ b/mk/toolchain/gcc/rte.vars.mk > @@ -71,11 +71,14 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS))) > endif > endif > > -WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes > +WERROR_FLAGS := -W -Wall -Wstrict-prototypes -Wmissing-prototypes > WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith > WERROR_FLAGS += -Wcast-align -Wnested-externs -Wcast-qual > WERROR_FLAGS += -Wformat-nonliteral -Wformat-security > WERROR_FLAGS += -Wundef -Wwrite-strings > +ifeq ($(CONFIG_RTE_ERROR_ON_WARNING),y) > +WERROR_FLAGS += -Werror > +endif > > # process cpu flags > include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk > diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk > index e39d710..652cca8 100644 > --- a/mk/toolchain/icc/rte.vars.mk > +++ b/mk/toolchain/icc/rte.vars.mk > @@ -69,8 +69,12 @@ TOOLCHAIN_ASFLAGS = > # error #13368: loop was not vectorized with "vector always assert" > # error #15527: loop was not vectorized: function call to fprintf cannot be vectorize > # was declared "deprecated" > -WERROR_FLAGS := -Wall -Werror-all -w2 -diag-disable 271 -diag-warning 1478 > +WERROR_FLAGS := -Wall -w2 -diag-disable 271 -diag-warning 1478 > WERROR_FLAGS += -diag-disable 13368 -diag-disable 15527 > +ifeq ($(CONFIG_RTE_ERROR_ON_WARNING),y) > +WERROR_FLAGS += -Werror-all > +endif > + > > # process cpu flags > include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk The -Werror is a real feature. Having dealt with legacy code where there are lots of bugs which were already warnings being ignored, having a big hard stop when errors are found is really good. The default should be on but I understand why you may want to turn it off, but it should require some effort to do.
On 02/12/2015 04:38 PM, Stephen Hemminger wrote: > On Thu, 12 Feb 2015 13:13:22 +0200 > Panu Matilainen <pmatilai@redhat.com> wrote: > >> This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable >> fail-on-warning compile behavior, defaulting to off. >> >> Failing build on warnings is a useful developer tool but its bad >> for release tarballs which can and do get built with newer >> compilers than what was used/available during development. Compilers >> routinely add new warnings so code which built silently with cc X >> might no longer do so with X+1. This doesn't make the existing code >> any more buggier and failing the build in this case does not help >> not help improve code quality of an already released version either. >> --- >> config/common_bsdapp | 1 + >> config/common_linuxapp | 1 + >> mk/toolchain/clang/rte.vars.mk | 5 ++++- >> mk/toolchain/gcc/rte.vars.mk | 5 ++++- >> mk/toolchain/icc/rte.vars.mk | 6 +++++- >> 5 files changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/config/common_bsdapp b/config/common_bsdapp >> index 57bacb8..a5687b3 100644 >> --- a/config/common_bsdapp >> +++ b/config/common_bsdapp >> @@ -362,6 +362,7 @@ CONFIG_RTE_KNI_VHOST_DEBUG_TX=n >> # Enable warning directives >> # >> CONFIG_RTE_INSECURE_FUNCTION_WARNING=n >> +CONFIG_RTE_ERROR_ON_WARNING=n >> >> # >> # Compile the test application >> diff --git a/config/common_linuxapp b/config/common_linuxapp >> index d428f84..0762f99 100644 >> --- a/config/common_linuxapp >> +++ b/config/common_linuxapp >> @@ -383,6 +383,7 @@ CONFIG_RTE_LIBRTE_XEN_DOM0=n >> # Enable warning directives >> # >> CONFIG_RTE_INSECURE_FUNCTION_WARNING=n >> +CONFIG_RTE_ERROR_ON_WARNING=n >> >> # >> # Compile the test application >> diff --git a/mk/toolchain/clang/rte.vars.mk b/mk/toolchain/clang/rte.vars.mk >> index 40cb389..12726e7 100644 >> --- a/mk/toolchain/clang/rte.vars.mk >> +++ b/mk/toolchain/clang/rte.vars.mk >> @@ -63,11 +63,14 @@ TOOLCHAIN_ASFLAGS = >> TOOLCHAIN_CFLAGS = >> TOOLCHAIN_LDFLAGS = >> >> -WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes >> +WERROR_FLAGS := -W -Wall -Wstrict-prototypes -Wmissing-prototypes >> WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith >> WERROR_FLAGS += -Wnested-externs -Wcast-qual >> WERROR_FLAGS += -Wformat-nonliteral -Wformat-security >> WERROR_FLAGS += -Wundef -Wwrite-strings >> +ifeq ($(CONFIG_RTE_ERROR_ON_WARNING),y) >> +WERROR_FLAGS += -Werror >> +endif >> >> # process cpu flags >> include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk >> diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk >> index 88f235c..bbd3c85 100644 >> --- a/mk/toolchain/gcc/rte.vars.mk >> +++ b/mk/toolchain/gcc/rte.vars.mk >> @@ -71,11 +71,14 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS))) >> endif >> endif >> >> -WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes >> +WERROR_FLAGS := -W -Wall -Wstrict-prototypes -Wmissing-prototypes >> WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith >> WERROR_FLAGS += -Wcast-align -Wnested-externs -Wcast-qual >> WERROR_FLAGS += -Wformat-nonliteral -Wformat-security >> WERROR_FLAGS += -Wundef -Wwrite-strings >> +ifeq ($(CONFIG_RTE_ERROR_ON_WARNING),y) >> +WERROR_FLAGS += -Werror >> +endif >> >> # process cpu flags >> include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk >> diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk >> index e39d710..652cca8 100644 >> --- a/mk/toolchain/icc/rte.vars.mk >> +++ b/mk/toolchain/icc/rte.vars.mk >> @@ -69,8 +69,12 @@ TOOLCHAIN_ASFLAGS = >> # error #13368: loop was not vectorized with "vector always assert" >> # error #15527: loop was not vectorized: function call to fprintf cannot be vectorize >> # was declared "deprecated" >> -WERROR_FLAGS := -Wall -Werror-all -w2 -diag-disable 271 -diag-warning 1478 >> +WERROR_FLAGS := -Wall -w2 -diag-disable 271 -diag-warning 1478 >> WERROR_FLAGS += -diag-disable 13368 -diag-disable 15527 >> +ifeq ($(CONFIG_RTE_ERROR_ON_WARNING),y) >> +WERROR_FLAGS += -Werror-all >> +endif >> + >> >> # process cpu flags >> include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk > > The -Werror is a real feature. Having dealt with legacy code where > there are lots of bugs which were already warnings being ignored, having > a big hard stop when errors are found is really good. > > The default should be on but I understand why you may want to turn it > off, but it should require some effort to do. I wholeheartedly agree with all that, when applied the developers of the codebase. I spent the last 7+ years of my life working on a piece of software that emitted well over six hundred "harmless" warnings on build when I started with it, trust me I have very little tolerance for introducing new compiler warnings. But placing that burden to users trying to compile a released version of the software does good to nobody. - Panu -
On Thu, 12 Feb 2015 16:54:44 +0200 Panu Matilainen <pmatilai@redhat.com> wrote: > On 02/12/2015 04:38 PM, Stephen Hemminger wrote: > > On Thu, 12 Feb 2015 13:13:22 +0200 > > Panu Matilainen <pmatilai@redhat.com> wrote: > > > >> This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable > >> fail-on-warning compile behavior, defaulting to off. > >> > >> Failing build on warnings is a useful developer tool but its bad > >> for release tarballs which can and do get built with newer > >> compilers than what was used/available during development. Compilers > >> routinely add new warnings so code which built silently with cc X > >> might no longer do so with X+1. This doesn't make the existing code > >> any more buggier and failing the build in this case does not help > >> not help improve code quality of an already released version either. Hopefully distro's like RHEL will build with -Werror enabled and not allow build to go through with errors.
On Fri, Feb 20, 2015 at 05:55:21PM -0800, Stephen Hemminger wrote: > On Thu, 12 Feb 2015 16:54:44 +0200 > Panu Matilainen <pmatilai@redhat.com> wrote: > > > On 02/12/2015 04:38 PM, Stephen Hemminger wrote: > > > On Thu, 12 Feb 2015 13:13:22 +0200 > > > Panu Matilainen <pmatilai@redhat.com> wrote: > > > > > >> This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable > > >> fail-on-warning compile behavior, defaulting to off. > > >> > > >> Failing build on warnings is a useful developer tool but its bad > > >> for release tarballs which can and do get built with newer > > >> compilers than what was used/available during development. Compilers > > >> routinely add new warnings so code which built silently with cc X > > >> might no longer do so with X+1. This doesn't make the existing code > > >> any more buggier and failing the build in this case does not help > > >> not help improve code quality of an already released version either. > > Hopefully distro's like RHEL will build with -Werror enabled > and not allow build to go through with errors. > Thats usually what we do, yes. Neil > >
On 02/21/2015 09:33 PM, Neil Horman wrote: > On Fri, Feb 20, 2015 at 05:55:21PM -0800, Stephen Hemminger wrote: >> On Thu, 12 Feb 2015 16:54:44 +0200 >> Panu Matilainen <pmatilai@redhat.com> wrote: >> >>> On 02/12/2015 04:38 PM, Stephen Hemminger wrote: >>>> On Thu, 12 Feb 2015 13:13:22 +0200 >>>> Panu Matilainen <pmatilai@redhat.com> wrote: >>>> >>>>> This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable >>>>> fail-on-warning compile behavior, defaulting to off. >>>>> >>>>> Failing build on warnings is a useful developer tool but its bad >>>>> for release tarballs which can and do get built with newer >>>>> compilers than what was used/available during development. Compilers >>>>> routinely add new warnings so code which built silently with cc X >>>>> might no longer do so with X+1. This doesn't make the existing code >>>>> any more buggier and failing the build in this case does not help >>>>> not help improve code quality of an already released version either. >> >> Hopefully distro's like RHEL will build with -Werror enabled >> and not allow build to go through with errors. >> > Thats usually what we do, yes. Um, nope. All Fedora and RHEL builds are done using a common base set of flags set centrally from rpm configuration, and that includes among other things -Wall but not -Werror, although since F21 -Werror=format-security is included since that there are relatively few false positives for that. The thing is, compiler warnings from compilers are just that: warnings, and often including hefty dose of false positives. A good package maintainer will look at the build logs of his/her packages, investigate warnings and send patches upstream to address them in oncoming versions where actually relevant, but generally a package maintainer in a distro is not responsible for achieving zero-warning build, nor should they. Take for example set-but-not-used warning introduced in gcc 4.6. Many of the warnings unearthed by that do indicate real potential issues in the software (a typical example being ignoring an allegedly unlikely error code from a syscall, sometimes dozens or even hundreds of them for larger projects) but its also typically code that's been in use for years and years without anybody noticing. That code does not suddenly become unusable just because its now being compiled by a compiler that detects this condition, and -Werror on distro level misplaces the burden of addressing years of upstream laziness on a packager who often has little to do with upstream. - Panu -
On Mon, Feb 23, 2015 at 10:19:23AM +0200, Panu Matilainen wrote: > On 02/21/2015 09:33 PM, Neil Horman wrote: > >On Fri, Feb 20, 2015 at 05:55:21PM -0800, Stephen Hemminger wrote: > >>On Thu, 12 Feb 2015 16:54:44 +0200 > >>Panu Matilainen <pmatilai@redhat.com> wrote: > >> > >>>On 02/12/2015 04:38 PM, Stephen Hemminger wrote: > >>>>On Thu, 12 Feb 2015 13:13:22 +0200 > >>>>Panu Matilainen <pmatilai@redhat.com> wrote: > >>>> > >>>>>This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable > >>>>>fail-on-warning compile behavior, defaulting to off. > >>>>> > >>>>>Failing build on warnings is a useful developer tool but its bad > >>>>>for release tarballs which can and do get built with newer > >>>>>compilers than what was used/available during development. Compilers > >>>>>routinely add new warnings so code which built silently with cc X > >>>>>might no longer do so with X+1. This doesn't make the existing code > >>>>>any more buggier and failing the build in this case does not help > >>>>>not help improve code quality of an already released version either. > >> > >>Hopefully distro's like RHEL will build with -Werror enabled > >>and not allow build to go through with errors. > >> > >Thats usually what we do, yes. > > Um, nope. All Fedora and RHEL builds are done using a common base set of > flags set centrally from rpm configuration, and that includes among other > things -Wall but not -Werror, although since F21 -Werror=format-security is > included since that there are relatively few false positives for that. > > The thing is, compiler warnings from compilers are just that: warnings, and > often including hefty dose of false positives. A good package maintainer > will look at the build logs of his/her packages, investigate warnings and > send patches upstream to address them in oncoming versions where actually > relevant, but generally a package maintainer in a distro is not responsible > for achieving zero-warning build, nor should they. > Um, I don't know what you've been doing, but most of my packages typically have zero warnings. Its true package maintainers have the option to disable warnings, and many do for pragmatic reasons as you note, but when its feasible, theres no reason not to make sure warning doesn't get raised when you expect there to be none. Neil
On 02/23/2015 03:55 PM, Neil Horman wrote: > On Mon, Feb 23, 2015 at 10:19:23AM +0200, Panu Matilainen wrote: >> On 02/21/2015 09:33 PM, Neil Horman wrote: >>> On Fri, Feb 20, 2015 at 05:55:21PM -0800, Stephen Hemminger wrote: >>>> On Thu, 12 Feb 2015 16:54:44 +0200 >>>> Panu Matilainen <pmatilai@redhat.com> wrote: >>>> >>>>> On 02/12/2015 04:38 PM, Stephen Hemminger wrote: >>>>>> On Thu, 12 Feb 2015 13:13:22 +0200 >>>>>> Panu Matilainen <pmatilai@redhat.com> wrote: >>>>>> >>>>>>> This adds new CONFIG_RTE_ERROR_ON_WARNING config option to enable >>>>>>> fail-on-warning compile behavior, defaulting to off. >>>>>>> >>>>>>> Failing build on warnings is a useful developer tool but its bad >>>>>>> for release tarballs which can and do get built with newer >>>>>>> compilers than what was used/available during development. Compilers >>>>>>> routinely add new warnings so code which built silently with cc X >>>>>>> might no longer do so with X+1. This doesn't make the existing code >>>>>>> any more buggier and failing the build in this case does not help >>>>>>> not help improve code quality of an already released version either. >>>> >>>> Hopefully distro's like RHEL will build with -Werror enabled >>>> and not allow build to go through with errors. >>>> >>> Thats usually what we do, yes. >> >> Um, nope. All Fedora and RHEL builds are done using a common base set of >> flags set centrally from rpm configuration, and that includes among other >> things -Wall but not -Werror, although since F21 -Werror=format-security is >> included since that there are relatively few false positives for that. >> >> The thing is, compiler warnings from compilers are just that: warnings, and >> often including hefty dose of false positives. A good package maintainer >> will look at the build logs of his/her packages, investigate warnings and >> send patches upstream to address them in oncoming versions where actually >> relevant, but generally a package maintainer in a distro is not responsible >> for achieving zero-warning build, nor should they. >> > Um, I don't know what you've been doing, but most of my packages typically have > zero warnings. Its true package maintainers have the option to disable > warnings, and many do for pragmatic reasons as you note, but when its feasible, > theres no reason not to make sure warning doesn't get raised when you expect > there to be none. The question wasn't about you or me or any other individual maintainer or package, it was whether distros build with -Werror, and the answer to that is generally no. Individual maintainers are free to do so of course, but for example with the ubiquitous autoconf-based packages you cant just stick -Werror into CFLAGS because it breaks a whole pile of the autoconf tests. But this is getting wildly off-topic for dpdk dev, I'll shut up now :) - Panu -
diff --git a/config/common_bsdapp b/config/common_bsdapp index 57bacb8..a5687b3 100644 --- a/config/common_bsdapp +++ b/config/common_bsdapp @@ -362,6 +362,7 @@ CONFIG_RTE_KNI_VHOST_DEBUG_TX=n # Enable warning directives # CONFIG_RTE_INSECURE_FUNCTION_WARNING=n +CONFIG_RTE_ERROR_ON_WARNING=n # # Compile the test application diff --git a/config/common_linuxapp b/config/common_linuxapp index d428f84..0762f99 100644 --- a/config/common_linuxapp +++ b/config/common_linuxapp @@ -383,6 +383,7 @@ CONFIG_RTE_LIBRTE_XEN_DOM0=n # Enable warning directives # CONFIG_RTE_INSECURE_FUNCTION_WARNING=n +CONFIG_RTE_ERROR_ON_WARNING=n # # Compile the test application diff --git a/mk/toolchain/clang/rte.vars.mk b/mk/toolchain/clang/rte.vars.mk index 40cb389..12726e7 100644 --- a/mk/toolchain/clang/rte.vars.mk +++ b/mk/toolchain/clang/rte.vars.mk @@ -63,11 +63,14 @@ TOOLCHAIN_ASFLAGS = TOOLCHAIN_CFLAGS = TOOLCHAIN_LDFLAGS = -WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes +WERROR_FLAGS := -W -Wall -Wstrict-prototypes -Wmissing-prototypes WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith WERROR_FLAGS += -Wnested-externs -Wcast-qual WERROR_FLAGS += -Wformat-nonliteral -Wformat-security WERROR_FLAGS += -Wundef -Wwrite-strings +ifeq ($(CONFIG_RTE_ERROR_ON_WARNING),y) +WERROR_FLAGS += -Werror +endif # process cpu flags include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk index 88f235c..bbd3c85 100644 --- a/mk/toolchain/gcc/rte.vars.mk +++ b/mk/toolchain/gcc/rte.vars.mk @@ -71,11 +71,14 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS))) endif endif -WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes +WERROR_FLAGS := -W -Wall -Wstrict-prototypes -Wmissing-prototypes WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith WERROR_FLAGS += -Wcast-align -Wnested-externs -Wcast-qual WERROR_FLAGS += -Wformat-nonliteral -Wformat-security WERROR_FLAGS += -Wundef -Wwrite-strings +ifeq ($(CONFIG_RTE_ERROR_ON_WARNING),y) +WERROR_FLAGS += -Werror +endif # process cpu flags include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk index e39d710..652cca8 100644 --- a/mk/toolchain/icc/rte.vars.mk +++ b/mk/toolchain/icc/rte.vars.mk @@ -69,8 +69,12 @@ TOOLCHAIN_ASFLAGS = # error #13368: loop was not vectorized with "vector always assert" # error #15527: loop was not vectorized: function call to fprintf cannot be vectorize # was declared "deprecated" -WERROR_FLAGS := -Wall -Werror-all -w2 -diag-disable 271 -diag-warning 1478 +WERROR_FLAGS := -Wall -w2 -diag-disable 271 -diag-warning 1478 WERROR_FLAGS += -diag-disable 13368 -diag-disable 15527 +ifeq ($(CONFIG_RTE_ERROR_ON_WARNING),y) +WERROR_FLAGS += -Werror-all +endif + # process cpu flags include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk