Re: [PATCH] libvirt: disable Werror for non-libvirt flights

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 15, 2022 at 03:10:59PM +0100, Anthony PERARD wrote:
> On Tue, Sep 13, 2022 at 12:03:28PM +0200, Roger Pau Monne wrote:
> > Current usage of Werror=switch-enum by default for libvirt builds out
> > of the git tree causes issues when new items are added to libxl public
> > API enums if those are used in a switch statement in libvirt code.
> > This leads to libvirt build failures for seemingly unrelated libxl
> > changes.
> > 
> > In order to prevent those errors from blocking the push gate, disable
> > Werror for libvirt builds when not in a libvirt specific flight.
> > 
> > The errors will be reported on the libvirt flight, and block the
> > pushes there.  So the author of the changes in libxl is still expected
> > to send a fix to libvirt code.  This is no ideal, but the other option
> > is to just disable Werror for all libvirt builds and let libvirt
> > developers fix the breakage when they notice it.
> > 
> > runvar differences for a xen-unstable flight are:
> > 
> > --- /dev/fd/63  2022-09-13 09:53:58.044441678 +0000
> > +++ /dev/fd/62  2022-09-13 09:53:58.044441678 +0000
> > @@ -574,6 +574,10 @@
> >  test-xtf-amd64-amd64-3                                arch                 
> >            amd64
> >  test-xtf-amd64-amd64-4                                arch                 
> >            amd64
> >  test-xtf-amd64-amd64-5                                arch                 
> >            amd64
> > +build-amd64-libvirt                                   autogen_options      
> >            --disable-werror
> > +build-arm64-libvirt                                   autogen_options      
> >            --disable-werror
> > +build-armhf-libvirt                                   autogen_options      
> >            --disable-werror
> > +build-i386-libvirt                                    autogen_options      
> >            --disable-werror
> >  test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm    bios                 
> >            seabios
> >  test-amd64-amd64-qemuu-nested-amd                     bios                 
> >            seabios
> >  test-amd64-amd64-qemuu-nested-intel                   bios                 
> >            seabios
> > @@ -1217,6 +1221,10 @@
> >  build-arm64-libvirt                                   make_njobs           
> >            1
> >  build-armhf-libvirt                                   make_njobs           
> >            1
> >  build-i386-libvirt                                    make_njobs           
> >            1
> > +build-amd64-libvirt                                   meson_options        
> >            -Dgit_werror=disabled
> > +build-arm64-libvirt                                   meson_options        
> >            -Dgit_werror=disabled
> > +build-armhf-libvirt                                   meson_options        
> >            -Dgit_werror=disabled
> > +build-i386-libvirt                                    meson_options        
> >            -Dgit_werror=disabled
> >  test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict recipe_dmrestrict    
> >            true
> >  test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict  recipe_dmrestrict    
> >            true
> >  test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict recipe_nomigrate     
> >            true
> 
> For "osstest" flight or "xen-unstable-smoke" flight, we would have the
> same difference, right?
> 
> The only branch with no change would be libvirt, right?

Indeed, that's the intention.

> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > I'm unsure whether we want o disable Werror even for libvirt flights,
> > but this seems more conservative.
> > 
> > This does at least unblock the libvirt builds for both the
> > xen-unstable and the libvirt flights.
> > ---
> > Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> > Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > Cc: Julien Grall <julien@xxxxxxx>
> > ---
> >  mfi-common       | 2 +-
> >  ts-libvirt-build | 3 ++-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mfi-common b/mfi-common
> > index 59e712f4..450229e9 100644
> > --- a/mfi-common
> > +++ b/mfi-common
> > @@ -459,7 +459,7 @@ create_build_jobs () {
> >      libvirt_build_runvars=''
> >      case "$branch" in
> >      libvirt*) ;;
> > -    *)        libvirt_build_runvars+=" make_njobs=1";;
> > +    *)        libvirt_build_runvars+=" make_njobs=1 
> > meson_options=-Dgit_werror=disabled autogen_options=--disable-werror";;
> 
> For meson, I think '-Dwerror=false' would be enough, instead of the
> unusual 'git_werror' configuration.
> 
> But, we might not need to disable all errors, for meson we can have:
>     -Dc_args='-Wno-error=switch -Wno-error=switch-enum'
> 
> But disabling werror is fine too, as less likely to be an issue later.
> 
> Both 'werror' and 'c_args' seems to be meson built-in options rather
> than options implemented for only libvirt.
>     https://mesonbuild.com/Builtin-options.html
> While 'git_werror' is libvirt only.

I don't have a strong opinion really, I've used git_werror because
that's the first thing that I found in:

https://libvirt.org/git/?p=libvirt.git;a=blob;f=meson_options.txt

I don't mind using -Dwerror=false if that's considered better. Ian, do
you have an opinion?

Thanks, Roger.