1. Patchew
  2. Xen
  3. View series

[PATCH] xen/Makefile: drop -Werror

Fabrice Fontaine posted 1 patch 2 years, 9 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210702170602.890817-1-fontaine.fabrice@gmail.com
xen/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] xen/Makefile: drop -Werror
Posted by Fabrice Fontaine 2 years, 9 months ago 
Drop -Werror to avoid the following build failure with -DNDEBUG:

In file included from <command-line>:0:0:
/usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
 #define NDEBUG

<command-line>:0:0: note: this is the location of the previous definition

Fixes:
 - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
---
 xen/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/Makefile b/xen/Makefile
index 89879fad4c..cf9f83b1fb 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -210,7 +210,7 @@ CFLAGS += -fomit-frame-pointer
 endif
 
 CFLAGS += -nostdinc -fno-builtin -fno-common
-CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
+CFLAGS += -Wredundant-decls -Wno-pointer-arith
 $(call cc-option-add,CFLAGS,CC,-Wvla)
 CFLAGS += -pipe -D__XEN__ -include $(BASEDIR)/include/xen/config.h
 CFLAGS-$(CONFIG_DEBUG_INFO) += -g
-- 
2.30.2
Re: [PATCH] xen/Makefile: drop -Werror
Posted by Andrew Cooper 2 years, 9 months ago 
On 02/07/2021 18:06, Fabrice Fontaine wrote:
> Drop -Werror to avoid the following build failure with -DNDEBUG:
>
> In file included from <command-line>:0:0:
> /usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
>  #define NDEBUG
>
> <command-line>:0:0: note: this is the location of the previous definition
>
> Fixes:
>  - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d
>
> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>

For better or worse, It is Xen's policy that -Werror will remain.  95%
of the time, it is the right thing.  We will however build failures
whenever they crop up.

This one is weird though.  How is NDEBUG getting in twice?  What does
the rest of this build environment look like?

~Andrew
Re: [PATCH] xen/Makefile: drop -Werror
Posted by Fabrice Fontaine 2 years, 9 months ago 
Le ven. 2 juil. 2021 à 19:34, Andrew Cooper
<andrew.cooper3@citrix.com> a écrit :
>
> On 02/07/2021 18:06, Fabrice Fontaine wrote:
> > Drop -Werror to avoid the following build failure with -DNDEBUG:
> >
> > In file included from <command-line>:0:0:
> > /usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
> >  #define NDEBUG
> >
> > <command-line>:0:0: note: this is the location of the previous definition
> >
> > Fixes:
> >  - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d
> >
> > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
>
> For better or worse, It is Xen's policy that -Werror will remain.  95%
> of the time, it is the right thing.  We will however build failures
> whenever they crop up.
>
> This one is weird though.  How is NDEBUG getting in twice?  What does
> the rest of this build environment look like?
NDEBUG is added by buildroot in the command line if the user sets
BR2_ENABLE_RUNTIME_DEBUG to false since
https://git.buildroot.net/buildroot/commit/?id=5a8c50fe05afacc3cbe8e7347e238da9f242fab0

I do agree that setting -Werror is generally perfectly valid for upstream.
However, for downstream packager, it is generally seen as an issue as
it will always raise unexepected build failures with older, newer, or
exotic toolchains, see
https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend.
It would be good to, at least, have an option to disable -Werror for
example through a XEN_DISABLE_WERROR.
>
> ~Andrew
Best Regards,

Fabrice
Re: [PATCH] xen/Makefile: drop -Werror
Posted by Elliott Mitchell 2 years, 9 months ago 
On Fri, Jul 02, 2021 at 07:51:55PM +0200, Fabrice Fontaine wrote:
> 
> I do agree that setting -Werror is generally perfectly valid for upstream.
> However, for downstream packager, it is generally seen as an issue as
> it will always raise unexepected build failures with older, newer, or
> exotic toolchains, see
> https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend.
> It would be good to, at least, have an option to disable -Werror for
> example through a XEN_DISABLE_WERROR.

Two people don't make it a majority opinion, but if this was a meeting
this opinion would get a second.

I don't know where everyone is on the spectrum, but I also strongly
dislike -Werror yet do like -Wall and tend to get rid of warnings.
-Werror is good for continuous integration systems, not so great for
releases or active development.

-Werror kind of seems like Stack Ranking, good for use during brief
periods, but poor for long term continuous use.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
Re: [PATCH] xen/Makefile: drop -Werror
Posted by Jan Beulich 2 years, 9 months ago 
On 02.07.2021 20:52, Elliott Mitchell wrote:
> On Fri, Jul 02, 2021 at 07:51:55PM +0200, Fabrice Fontaine wrote:
>>
>> I do agree that setting -Werror is generally perfectly valid for upstream.
>> However, for downstream packager, it is generally seen as an issue as
>> it will always raise unexepected build failures with older, newer, or
>> exotic toolchains, see
>> https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend.
>> It would be good to, at least, have an option to disable -Werror for
>> example through a XEN_DISABLE_WERROR.
> 
> Two people don't make it a majority opinion, but if this was a meeting
> this opinion would get a second.
> 
> I don't know where everyone is on the spectrum, but I also strongly
> dislike -Werror yet do like -Wall and tend to get rid of warnings.
> -Werror is good for continuous integration systems, not so great for
> releases or active development.

Well, my experience with Linux (when I started working there alongside
working on Xen, many years ago) was that many people don't care at all
about compiler warnings their code changes introduce. While Linux has
improved some, I'm still carrying a fair size patch to silence all the
warnings that I observe on various build systems (i.e. with various
compiler versions). I do this because in a build with (perhaps many)
pre-existing warnings it is far easier to miss the one you accidentally
introduce with some code change. -Werror is an imo very appropriate
measure to get people to at least address the warnings they can easily
observe about everywhere. IOW I've always been appreciating Xen being
different from Linux in this regard.

Jan
Re: [PATCH] xen/Makefile: drop -Werror
Posted by Jan Beulich 2 years, 9 months ago 
On 02.07.2021 19:51, Fabrice Fontaine wrote:
> Le ven. 2 juil. 2021 à 19:34, Andrew Cooper
> <andrew.cooper3@citrix.com> a écrit :
>>
>> On 02/07/2021 18:06, Fabrice Fontaine wrote:
>>> Drop -Werror to avoid the following build failure with -DNDEBUG:
>>>
>>> In file included from <command-line>:0:0:
>>> /usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
>>>  #define NDEBUG
>>>
>>> <command-line>:0:0: note: this is the location of the previous definition
>>>
>>> Fixes:
>>>  - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d
>>>
>>> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
>>
>> For better or worse, It is Xen's policy that -Werror will remain.  95%
>> of the time, it is the right thing.  We will however build failures
>> whenever they crop up.
>>
>> This one is weird though.  How is NDEBUG getting in twice?  What does
>> the rest of this build environment look like?
> NDEBUG is added by buildroot in the command line if the user sets
> BR2_ENABLE_RUNTIME_DEBUG to false since
> https://git.buildroot.net/buildroot/commit/?id=5a8c50fe05afacc3cbe8e7347e238da9f242fab0

I suppose the build environment setting is really intended for user mode
code. I question its applicability to the building of kernels or
hypervisors, but I can see that opinions may vary here. If we wanted to
honor a pre-existing NDEBUG, how about simply making xen/config.h have

#if !defined(CONFIG_DEBUG) && !defined(NDEBUG)
#define NDEBUG
#endif

? This then raises the question though how an external environment could
achieve the opposite effect of suppressing NDEBUG's definition despite
CONFIG_DEBUG being set. (The main point - hence my view expressed above -
is that we switched to Kconfig to centralize where settings get
established, moving away from taking ones from environment or make
command line.)

Jan
Re: [PATCH] xen/Makefile: drop -Werror
Posted by Fabrice Fontaine 2 years, 9 months ago 
Dear all,

Le lun. 5 juil. 2021 à 10:16, Jan Beulich <jbeulich@suse.com> a écrit :
>
> On 02.07.2021 19:51, Fabrice Fontaine wrote:
> > Le ven. 2 juil. 2021 à 19:34, Andrew Cooper
> > <andrew.cooper3@citrix.com> a écrit :
> >>
> >> On 02/07/2021 18:06, Fabrice Fontaine wrote:
> >>> Drop -Werror to avoid the following build failure with -DNDEBUG:
> >>>
> >>> In file included from <command-line>:0:0:
> >>> /usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
> >>>  #define NDEBUG
> >>>
> >>> <command-line>:0:0: note: this is the location of the previous definition
> >>>
> >>> Fixes:
> >>>  - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d
> >>>
> >>> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> >>
> >> For better or worse, It is Xen's policy that -Werror will remain.  95%
> >> of the time, it is the right thing.  We will however build failures
> >> whenever they crop up.
> >>
> >> This one is weird though.  How is NDEBUG getting in twice?  What does
> >> the rest of this build environment look like?
> > NDEBUG is added by buildroot in the command line if the user sets
> > BR2_ENABLE_RUNTIME_DEBUG to false since
> > https://git.buildroot.net/buildroot/commit/?id=5a8c50fe05afacc3cbe8e7347e238da9f242fab0
>
> I suppose the build environment setting is really intended for user mode
> code. I question its applicability to the building of kernels or
> hypervisors, but I can see that opinions may vary here. If we wanted to
> honor a pre-existing NDEBUG, how about simply making xen/config.h have
>
> #if !defined(CONFIG_DEBUG) && !defined(NDEBUG)
> #define NDEBUG
> #endif
>
> ? This then raises the question though how an external environment could
> achieve the opposite effect of suppressing NDEBUG's definition despite
> CONFIG_DEBUG being set. (The main point - hence my view expressed above -
> is that we switched to Kconfig to centralize where settings get
> established, moving away from taking ones from environment or make
> command line.)
FYI, we have reverted the commit that allowed the user to set -DNDEBUG
as it was raising too many build failures:
https://git.buildroot.net/buildroot/commit/?id=a1c7cff1a081765c082c196bd9e6c1e72ceee797
So, this patch is not needed anymore on buildroot side.
>
> Jan
>
Best Regards,

Fabrice

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.