| Message ID | 20220913100328.27771-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libvirt: disable Werror for non-libvirt flights | expand |
Roger Pau Monne writes ("[PATCH] libvirt: disable Werror for non-libvirt flights"):
> 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.
..
> +build-i386-libvirt autogen_options --disable-werror
We have no way to specify -Wno-error-switch-enum specifically ?
(I'm not sure if that would be desirable.)
> I'm unsure whether we want o disable Werror even for libvirt flights,
> but this seems more conservative.
Probably disabling it only for Xen is right.
Ian.
On Tue, Sep 13, 2022 at 01:54:12PM +0100, Ian Jackson wrote: > Roger Pau Monne writes ("[PATCH] libvirt: disable Werror for non-libvirt flights"): > > 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. > .. > > +build-i386-libvirt autogen_options --disable-werror > > We have no way to specify -Wno-error-switch-enum specifically ? > (I'm not sure if that would be desirable.) Hm, maybe playing with CFLAGS, but not from the autogen/meson options AFAIK. Using the autogen/meson flags seems cleaner and less error prone (albeit the disabling of Werror is more wide than what we strictly require). > > I'm unsure whether we want o disable Werror even for libvirt flights, > > but this seems more conservative. > > Probably disabling it only for Xen is right. Thanks, let's try this first then. Roger.
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? > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > 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@xenproject.org> > Cc: Anthony PERARD <anthony.perard@citrix.com> > Cc: Julien Grall <julien@xen.org> > --- > 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. Thanks,
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@citrix.com> > > --- > > 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@xenproject.org> > > Cc: Anthony PERARD <anthony.perard@citrix.com> > > Cc: Julien Grall <julien@xen.org> > > --- > > 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.
Roger Pau Monné writes ("Re: [PATCH] libvirt: disable Werror for non-libvirt flights"):
> I don't mind using -Dwerror=false if that's considered better. Ian, do
> you have an opinion?
No, I don't think I do. I think it depends on what kinds of things
are likely to change, or go wrong, in libvirt.
- *) libvirt_build_runvars+=" make_njobs=1";;
+ *) libvirt_build_runvars+=" make_njobs=1 meson_options=-Dwerror=false autogen_options=--disable-werror";;
I wonder if it would be better to abstract this away and instead have
a runvar like "libvrit_build_werror=false". But maybe that is
gold-plating it.
If you choose to keep that the way it is, then for either version of
this patch:
Acked-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
On Fri, Sep 16, 2022 at 01:20:11AM +0100, Ian Jackson wrote: > Roger Pau Monné writes ("Re: [PATCH] libvirt: disable Werror for non-libvirt flights"): > > I don't mind using -Dwerror=false if that's considered better. Ian, do > > you have an opinion? > > No, I don't think I do. I think it depends on what kinds of things > are likely to change, or go wrong, in libvirt. > > - *) libvirt_build_runvars+=" make_njobs=1";; > + *) libvirt_build_runvars+=" make_njobs=1 meson_options=-Dwerror=false autogen_options=--disable-werror";; > > I wonder if it would be better to abstract this away and instead have > a runvar like "libvrit_build_werror=false". But maybe that is > gold-plating it. Did consider this initially, but decided to use specific options for mesa/autogen in case we need to add further switches to specific configuration systems (unlikely I guess, because autogen won't be used anymore). > If you choose to keep that the way it is, then for either version of > this patch: > > Acked-by: Ian Jackson <ijackson@chiark.greenend.org.uk> Thanks, Roger.
--- /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 Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- 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@xenproject.org> Cc: Anthony PERARD <anthony.perard@citrix.com> Cc: Julien Grall <julien@xen.org> --- 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";; esac job_create_build build-$arch-libvirt build-libvirt \ diff --git a/ts-libvirt-build b/ts-libvirt-build index 16b45cfd..e4faa1d7 100755 --- a/ts-libvirt-build +++ b/ts-libvirt-build @@ -73,7 +73,7 @@ sub config() { --with-libxl --without-xen --without-xenapi --without-selinux \\ --without-lxc --without-vbox --without-uml \\ --without-qemu --without-openvz --without-vmware \\ - --sysconfdir=/etc --localstatedir=/var #/ + --sysconfdir=/etc --localstatedir=/var $r{autogen_options} #/ END } else { target_cmd_build($ho, 3600, $builddir, <<END); @@ -87,6 +87,7 @@ END -Ddriver_libvirtd=enabled \\ -Ddriver_remote=enabled \\ --sysconfdir=/etc --localstatedir=/var \\ + $r{meson_options} \\ build END }