[PATCH] Make -Werror optional

[PATCH] Make -Werror optional

[Panu Matilainen]

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
-- 
2.1.0

Re: [PATCH] Make -Werror optional

[Bruce Richardson]

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

Re: [PATCH] Make -Werror optional

[Panu Matilainen]

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 -

Re: [PATCH] Make -Werror optional

[Bruce Richardson]

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 -
> 

Re: [PATCH] Make -Werror optional

[Panu Matilainen]

On 02/12/2015 02:08 PM, Bruce Richardson wrote:
> 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.

Ok, would something to this tune be more acceptable?

diff --git a/mk/rte.vars.mk b/mk/rte.vars.mk
index d5b36be..2ad969b 100644
--- a/mk/rte.vars.mk
+++ b/mk/rte.vars.mk
@@ -71,6 +71,10 @@ ifneq ($(BUILDING_RTE_SDK),)
    ifeq ($(RTE_BUILD_COMBINE_LIBS),)
      RTE_BUILD_COMBINE_LIBS := n
    endif
+  # see if we're building from git
+  ifneq ($(wildcard $(RTE_SDK)/.git),)
+    DEVEL_BUILD := y
+  endif
  endif

  RTE_LIBNAME := $(CONFIG_RTE_LIBNAME:"%"=%)
diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
index 88f235c..a14ae44 100644
--- a/mk/toolchain/gcc/rte.vars.mk
+++ b/mk/toolchain/gcc/rte.vars.mk
@@ -71,12 +71,16 @@ 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 ($(DEVEL_BUILD),y)
+WERROR_FLAGS += -Werror
+endif
+
  # process cpu flags
  include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk

(non-gcc toolchains are missing and whitespace probably broken due to 
copy-paste, this is not a suggested patch but just an RFC for the approach)


	- Panu -

Re: [PATCH] Make -Werror optional

[Bruce Richardson]

On Thu, Feb 12, 2015 at 03:58:07PM +0200, Panu Matilainen wrote:
> On 02/12/2015 02:08 PM, Bruce Richardson wrote:
> >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.
> 
> Ok, would something to this tune be more acceptable?
> 
> diff --git a/mk/rte.vars.mk b/mk/rte.vars.mk
> index d5b36be..2ad969b 100644
> --- a/mk/rte.vars.mk
> +++ b/mk/rte.vars.mk
> @@ -71,6 +71,10 @@ ifneq ($(BUILDING_RTE_SDK),)
>    ifeq ($(RTE_BUILD_COMBINE_LIBS),)
>      RTE_BUILD_COMBINE_LIBS := n
>    endif
> +  # see if we're building from git
> +  ifneq ($(wildcard $(RTE_SDK)/.git),)
> +    DEVEL_BUILD := y
> +  endif
>  endif
> 
>  RTE_LIBNAME := $(CONFIG_RTE_LIBNAME:"%"=%)
> diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
> index 88f235c..a14ae44 100644
> --- a/mk/toolchain/gcc/rte.vars.mk
> +++ b/mk/toolchain/gcc/rte.vars.mk
> @@ -71,12 +71,16 @@ 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 ($(DEVEL_BUILD),y)
> +WERROR_FLAGS += -Werror
> +endif
> +
>  # process cpu flags
>  include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk
> 
> (non-gcc toolchains are missing and whitespace probably broken due to
> copy-paste, this is not a suggested patch but just an RFC for the approach)
> 
> 
> 	- Panu -
> 
Looks ok to me.
/Bruce

Re: [PATCH] Make -Werror optional

[Thomas Monjalon]

2015-02-12 15:58, Panu Matilainen:
> +  # see if we're building from git
> +  ifneq ($(wildcard $(RTE_SDK)/.git),)
> +    DEVEL_BUILD := y
> +  endif

Yes it allows to force DEVEL_BUILD to any value on command line.
But please use RTE_ prefix.

[PATCH v2] mk: Only default to -Werror when building from git checkout

[Panu Matilainen]

Add RTE_DEVEL_BUILD make-variable which can be used to do things
differently when doing development vs building a release,
autodetected from source root .git presence and overridable via
commandline. Use it to only enable -Werror compiler flag when
building a git checkout:

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.
---
 mk/rte.vars.mk                 | 4 ++++
 mk/toolchain/clang/rte.vars.mk | 6 +++++-
 mk/toolchain/gcc/rte.vars.mk   | 6 +++++-
 mk/toolchain/icc/rte.vars.mk   | 6 +++++-
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/mk/rte.vars.mk b/mk/rte.vars.mk
index d5b36be..a4ad367 100644
--- a/mk/rte.vars.mk
+++ b/mk/rte.vars.mk
@@ -71,6 +71,10 @@ ifneq ($(BUILDING_RTE_SDK),)
   ifeq ($(RTE_BUILD_COMBINE_LIBS),)
     RTE_BUILD_COMBINE_LIBS := n
   endif
+  # see if we're building from git
+  ifneq ($(wildcard $(RTE_SDK)/.git),)
+    RTE_DEVEL_BUILD := y
+  endif
 endif
 
 RTE_LIBNAME := $(CONFIG_RTE_LIBNAME:"%"=%)
diff --git a/mk/toolchain/clang/rte.vars.mk b/mk/toolchain/clang/rte.vars.mk
index 40cb389..01b1860 100644
--- a/mk/toolchain/clang/rte.vars.mk
+++ b/mk/toolchain/clang/rte.vars.mk
@@ -63,12 +63,16 @@ 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 ($(RTE_DEVEL_BUILD),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..fc3a642 100644
--- a/mk/toolchain/gcc/rte.vars.mk
+++ b/mk/toolchain/gcc/rte.vars.mk
@@ -71,12 +71,16 @@ 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 ($(RTE_DEVEL_BUILD),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..0850719 100644
--- a/mk/toolchain/icc/rte.vars.mk
+++ b/mk/toolchain/icc/rte.vars.mk
@@ -69,9 +69,13 @@ 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 ($(RTE_DEVEL_BUILD),y)
+WERROR_FLAGS += -Werror-all
+endif
+
 # process cpu flags
 include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk
 # disable max-inline params boundaries for ICC 15 compiler
-- 
2.1.0

Re: [PATCH v2] mk: Only default to -Werror when building from git checkout

[Thomas Monjalon]

2015-02-12 17:18, Panu Matilainen:
> Add RTE_DEVEL_BUILD make-variable which can be used to do things
> differently when doing development vs building a release,
> autodetected from source root .git presence and overridable via
> commandline. Use it to only enable -Werror compiler flag when
> building a git checkout:
> 
> 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.

Please, could you update documentation to explain RTE_DEVEL_BUILD option?
Some users could try to build from git, so we should advise to disable RTE_DEVEL_BUILD.
These files might be updated:
	http://dpdk.org/browse/dpdk/tree/doc/build-sdk-quick.txt
	http://dpdk.org/browse/dpdk/tree/doc/guides/linux_gsg/build_dpdk.rst
	http://dpdk.org/browse/dpdk/tree/doc/guides/freebsd_gsg/build_dpdk.rst
	http://dpdk.org/browse/dpdk/tree/doc/guides/prog_guide/dev_kit_build_system.rst

Thanks

Re: [PATCH v2] mk: Only default to -Werror when building from git checkout

[Stephen Hemminger]

On Fri, 20 Feb 2015 13:15:38 +0100
Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> wrote:

> 2015-02-12 17:18, Panu Matilainen:
> > Add RTE_DEVEL_BUILD make-variable which can be used to do things
> > differently when doing development vs building a release,
> > autodetected from source root .git presence and overridable via
> > commandline. Use it to only enable -Werror compiler flag when
> > building a git checkout:
> > 
> > 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.
> 
> Please, could you update documentation to explain RTE_DEVEL_BUILD option?
> Some users could try to build from git, so we should advise to disable RTE_DEVEL_BUILD.
> These files might be updated:
> 	http://dpdk.org/browse/dpdk/tree/doc/build-sdk-quick.txt
> 	http://dpdk.org/browse/dpdk/tree/doc/guides/linux_gsg/build_dpdk.rst
> 	http://dpdk.org/browse/dpdk/tree/doc/guides/freebsd_gsg/build_dpdk.rst
> 	http://dpdk.org/browse/dpdk/tree/doc/guides/prog_guide/dev_kit_build_system.rst
> 
> Thanks

Also do not allow any patches to into upstream that cause
new warnings with latest stable version of Gcc or Clang?
I don't want a clean project to get littered with warning graffiti.
Maybe a build bot?

Re: [PATCH v2] mk: Only default to -Werror when building from git checkout

[Thomas Monjalon]

2015-02-20 18:15, Stephen Hemminger:
> On Fri, 20 Feb 2015 13:15:38 +0100
> Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> wrote:
> 
> > 2015-02-12 17:18, Panu Matilainen:
> > > Add RTE_DEVEL_BUILD make-variable which can be used to do things
> > > differently when doing development vs building a release,
> > > autodetected from source root .git presence and overridable via
> > > commandline. Use it to only enable -Werror compiler flag when
> > > building a git checkout:
> > > 
> > > 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.
> > 
> > Please, could you update documentation to explain RTE_DEVEL_BUILD option?
> > Some users could try to build from git, so we should advise to disable RTE_DEVEL_BUILD.
> > These files might be updated:
> > 	http://dpdk.org/browse/dpdk/tree/doc/build-sdk-quick.txt
> > 	http://dpdk.org/browse/dpdk/tree/doc/guides/linux_gsg/build_dpdk.rst
> > 	http://dpdk.org/browse/dpdk/tree/doc/guides/freebsd_gsg/build_dpdk.rst
> > 	http://dpdk.org/browse/dpdk/tree/doc/guides/prog_guide/dev_kit_build_system.rst
> > 
> > Thanks
> 
> Also do not allow any patches to into upstream that cause
> new warnings with latest stable version of Gcc or Clang?
> I don't want a clean project to get littered with warning graffiti.
> Maybe a build bot?

Yes there are some daily builds with many compilers/distros.
We'll have to be sure that they are done in devel mode (with -Werror).
Anyway devel build is a sanity check which should be done by every developers
and reviewers.

[PATCH v3] mk: stop on warning only in developer build

[Thomas Monjalon]

From: Panu Matilainen <pmatilai@redhat.com>

Add RTE_DEVEL_BUILD make-variable which can be used to do things
differently when doing development vs building a release,
autodetected from source root .git presence and overridable via
commandline. It is used it to enable -Werror compiler flag and may
be extended to other checks.

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
to improve the quality of an already released version either.

This change the default flags which can be tuned with EXTRA_CFLAGS.

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 doc/build-sdk-quick.txt                        | 1 +
 doc/guides/prog_guide/dev_kit_build_system.rst | 2 ++
 mk/rte.vars.mk                                 | 5 +++++
 mk/toolchain/clang/rte.vars.mk                 | 6 +++++-
 mk/toolchain/gcc/rte.vars.mk                   | 6 +++++-
 mk/toolchain/icc/rte.vars.mk                   | 6 +++++-
 6 files changed, 23 insertions(+), 3 deletions(-)

v3:
- not only for SDK build (-Werror for examples, apps)
- update doc

diff --git a/doc/build-sdk-quick.txt b/doc/build-sdk-quick.txt
index acd1bfe..967ff09 100644
--- a/doc/build-sdk-quick.txt
+++ b/doc/build-sdk-quick.txt
@@ -15,6 +15,7 @@ Build variables
 	EXTRA_LDFLAGS    linker options
 	EXTRA_LDLIBS     linker library options
 	RTE_KERNELDIR    linux headers path
+	RTE_DEVEL_BUILD  stricter options (default: y in git tree)
 	CROSS     toolchain prefix
 	V         verbose
 	D         debug dependencies
diff --git a/doc/guides/prog_guide/dev_kit_build_system.rst b/doc/guides/prog_guide/dev_kit_build_system.rst
index dd3e3d0..3e89eae 100644
--- a/doc/guides/prog_guide/dev_kit_build_system.rst
+++ b/doc/guides/prog_guide/dev_kit_build_system.rst
@@ -343,6 +343,8 @@ Useful Variables Provided by the Build System
     By default, the variable is set to /lib/modules/$(shell uname -r)/build,
     which is correct when the target machine is also the build machine.
 
+*   RTE_DEVEL_BUILD: Stricter options (stop on warning). It defaults to y in a git tree.
+
 Variables that Can be Set/Overridden in a Makefile Only
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/mk/rte.vars.mk b/mk/rte.vars.mk
index 2d734bd..28982a5 100644
--- a/mk/rte.vars.mk
+++ b/mk/rte.vars.mk
@@ -102,6 +102,11 @@ export RTE_MACHINE
 export RTE_EXEC_ENV
 export RTE_TOOLCHAIN
 
+# developer build automatically enabled in a git tree
+ifneq ($(wildcard $(RTE_SDK)/.git),)
+RTE_DEVEL_BUILD := y
+endif
+
 # SRCDIR is the current source directory
 ifdef S
 SRCDIR := $(abspath $(RTE_SRCDIR)/$(S))
diff --git a/mk/toolchain/clang/rte.vars.mk b/mk/toolchain/clang/rte.vars.mk
index 245ea7e..7749b99 100644
--- a/mk/toolchain/clang/rte.vars.mk
+++ b/mk/toolchain/clang/rte.vars.mk
@@ -63,12 +63,16 @@ 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 ($(RTE_DEVEL_BUILD),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 c2c5255..ff70f3d 100644
--- a/mk/toolchain/gcc/rte.vars.mk
+++ b/mk/toolchain/gcc/rte.vars.mk
@@ -71,12 +71,16 @@ 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 ($(RTE_DEVEL_BUILD),y)
+WERROR_FLAGS += -Werror
+endif
+
 # There are many issues reported for ARMv7 architecture
 # which are not necessarily fatal. Report as warnings.
 ifeq ($(CONFIG_RTE_ARCH_ARMv7),y)
diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk
index 9b6b34b..ba69f1f 100644
--- a/mk/toolchain/icc/rte.vars.mk
+++ b/mk/toolchain/icc/rte.vars.mk
@@ -69,9 +69,13 @@ 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 ($(RTE_DEVEL_BUILD),y)
+WERROR_FLAGS += -Werror-all
+endif
+
 # process cpu flags
 include $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk
 # disable max-inline params boundaries for ICC compiler for version 15 and greater
-- 
2.7.0

Re: [PATCH v3] mk: stop on warning only in developer build

[Bruce Richardson]

On Wed, Mar 02, 2016 at 03:22:23PM +0100, Thomas Monjalon wrote:
> From: Panu Matilainen <pmatilai@redhat.com>
> 
> Add RTE_DEVEL_BUILD make-variable which can be used to do things
> differently when doing development vs building a release,
> autodetected from source root .git presence and overridable via
> commandline. It is used it to enable -Werror compiler flag and may
> be extended to other checks.
> 
> 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
> to improve the quality of an already released version either.
> 
> This change the default flags which can be tuned with EXTRA_CFLAGS.
> 
> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH v3] mk: stop on warning only in developer build

[Thomas Monjalon]

2016-03-02 22:04, Bruce Richardson:
> On Wed, Mar 02, 2016 at 03:22:23PM +0100, Thomas Monjalon wrote:
> > From: Panu Matilainen <pmatilai@redhat.com>
> > 
> > Add RTE_DEVEL_BUILD make-variable which can be used to do things
> > differently when doing development vs building a release,
> > autodetected from source root .git presence and overridable via
> > commandline. It is used it to enable -Werror compiler flag and may
> > be extended to other checks.
> > 
> > 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
> > to improve the quality of an already released version either.
> > 
> > This change the default flags which can be tuned with EXTRA_CFLAGS.
> > 
> > Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks

If people have problems compiling previous releases,
please remind to use EXTRA_CFLAGS=-Wno-error

Re: [PATCH v3] mk: stop on warning only in developer build

[Panu Matilainen]

On 03/03/2016 12:36 PM, Thomas Monjalon wrote:
> 2016-03-02 22:04, Bruce Richardson:
>> On Wed, Mar 02, 2016 at 03:22:23PM +0100, Thomas Monjalon wrote:
>>> From: Panu Matilainen <pmatilai@redhat.com>
>>>
>>> Add RTE_DEVEL_BUILD make-variable which can be used to do things
>>> differently when doing development vs building a release,
>>> autodetected from source root .git presence and overridable via
>>> commandline. It is used it to enable -Werror compiler flag and may
>>> be extended to other checks.
>>>
>>> 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
>>> to improve the quality of an already released version either.
>>>
>>> This change the default flags which can be tuned with EXTRA_CFLAGS.
>>>
>>> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
>>> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
>>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
> Applied, thanks

Thanks for dusting this up, I'd pretty much forgotten the whole thing.

	- Panu -

Re: [PATCH] Make -Werror optional

[Stephen Hemminger]

On Thu, 12 Feb 2015 13:13:22 +0200
Panu Matilainen <pmatilai-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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.

Re: [PATCH] Make -Werror optional

[Panu Matilainen]

On 02/12/2015 04:38 PM, Stephen Hemminger wrote:
> On Thu, 12 Feb 2015 13:13:22 +0200
> Panu Matilainen <pmatilai-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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 -

Re: [PATCH] Make -Werror optional

[Stephen Hemminger]

On Thu, 12 Feb 2015 16:54:44 +0200
Panu Matilainen <pmatilai-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On 02/12/2015 04:38 PM, Stephen Hemminger wrote:
> > On Thu, 12 Feb 2015 13:13:22 +0200
> > Panu Matilainen <pmatilai-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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.

Re: [PATCH] Make -Werror optional

[Neil Horman]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On 02/12/2015 04:38 PM, Stephen Hemminger wrote:
> > > On Thu, 12 Feb 2015 13:13:22 +0200
> > > Panu Matilainen <pmatilai-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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

> 
> 

Re: [PATCH] Make -Werror optional

[Panu Matilainen]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>
>>> On 02/12/2015 04:38 PM, Stephen Hemminger wrote:
>>>> On Thu, 12 Feb 2015 13:13:22 +0200
>>>> Panu Matilainen <pmatilai-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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 -

Re: [PATCH] Make -Werror optional

[Neil Horman]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>
> >>>On 02/12/2015 04:38 PM, Stephen Hemminger wrote:
> >>>>On Thu, 12 Feb 2015 13:13:22 +0200
> >>>>Panu Matilainen <pmatilai-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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

Re: [PATCH] Make -Werror optional

[Panu Matilainen]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>
>>>>> On 02/12/2015 04:38 PM, Stephen Hemminger wrote:
>>>>>> On Thu, 12 Feb 2015 13:13:22 +0200
>>>>>> Panu Matilainen <pmatilai-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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 -