Re: [libvirt PATCH 09/20] ci: build.sh: Join MESON_ARGS and MESON_OPTS
On Wed, Aug 23, 2023 at 11:38:17AM +0200, Erik Skultety wrote: > On Mon, Aug 21, 2023 at 05:17:06AM -0700, Andrea Bolognani wrote: > > On Mon, Feb 06, 2023 at 02:53:06PM +0100, Erik Skultety wrote: > > > -meson setup build --werror -Dsystem=true $MESON_OPTS $MESON_ARGS || \ > > > +MESON_ARGS="$MESON_ARGS $MESON_OPTS" > > > + > > > +meson setup build --werror -Dsystem=true $MESON_ARGS || \ > > > > This has inverted the priority of the two lists of arguments. > > > > Before the change, an option (e.g. -Dfoo=enabled) could be added to > > $MESON_ARGS at the job level and it would override the same option > > (e.g. -Dfoo=disabled) defined as part of $MESON_OPTS in the container > > image. Now the option in the container image will always take > > precedence, which is undesirable. > > Good points. However, I'm already close to proposing something vastly > different > from this, dropping most of these variables as they are to make the local > executions pretty much >95% compatible to what happens in GitLab and > overriding > these shell variables simply isn't part of it because it only makes sense > right > now, but I think with the changes I have it'll only make sense in interactive > container shell sessions in which case it's left to the developer to set and > pass the right options IMO. This all sounds good, but in the meantime the feature is not working as intended. I've posted a quick fix here: https://listman.redhat.com/archives/libvir-list/2023-August/241402.html -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 09/20] ci: build.sh: Join MESON_ARGS and MESON_OPTS
On Mon, Aug 21, 2023 at 05:17:06AM -0700, Andrea Bolognani wrote: > Resurrecting this thread now what you've pushed some of the patches. > > On Mon, Feb 06, 2023 at 02:53:06PM +0100, Erik Skultety wrote: > > +# $MESON_ARGS correspond to meson's setup args, i.e. configure args. It's > > +# populated either from a GitLab's job configuration or from command line > > as > > +# `$ helper build --meson-configure-args=-Dopt1 -Dopt2` when run in a local > > +# containerized environment > > You need quotes around the value. As is, the shell will interpret > '--meson-configure-args=-Dopt1' and '-Dopt2' as separate arguments > and things will not work the way you expect them to. > > > -meson setup build --werror -Dsystem=true $MESON_OPTS $MESON_ARGS || \ > > +MESON_ARGS="$MESON_ARGS $MESON_OPTS" > > + > > +meson setup build --werror -Dsystem=true $MESON_ARGS || \ > > This has inverted the priority of the two lists of arguments. > > Before the change, an option (e.g. -Dfoo=enabled) could be added to > $MESON_ARGS at the job level and it would override the same option > (e.g. -Dfoo=disabled) defined as part of $MESON_OPTS in the container > image. Now the option in the container image will always take > precedence, which is undesirable. > Good points. However, I'm already close to proposing something vastly different from this, dropping most of these variables as they are to make the local executions pretty much >95% compatible to what happens in GitLab and overriding these shell variables simply isn't part of it because it only makes sense right now, but I think with the changes I have it'll only make sense in interactive container shell sessions in which case it's left to the developer to set and pass the right options IMO. Erik
Re: [libvirt PATCH 09/20] ci: build.sh: Join MESON_ARGS and MESON_OPTS
Resurrecting this thread now what you've pushed some of the patches. On Mon, Feb 06, 2023 at 02:53:06PM +0100, Erik Skultety wrote: > +# $MESON_ARGS correspond to meson's setup args, i.e. configure args. It's > +# populated either from a GitLab's job configuration or from command line as > +# `$ helper build --meson-configure-args=-Dopt1 -Dopt2` when run in a local > +# containerized environment You need quotes around the value. As is, the shell will interpret '--meson-configure-args=-Dopt1' and '-Dopt2' as separate arguments and things will not work the way you expect them to. > -meson setup build --werror -Dsystem=true $MESON_OPTS $MESON_ARGS || \ > +MESON_ARGS="$MESON_ARGS $MESON_OPTS" > + > +meson setup build --werror -Dsystem=true $MESON_ARGS || \ This has inverted the priority of the two lists of arguments. Before the change, an option (e.g. -Dfoo=enabled) could be added to $MESON_ARGS at the job level and it would override the same option (e.g. -Dfoo=disabled) defined as part of $MESON_OPTS in the container image. Now the option in the container image will always take precedence, which is undesirable. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 09/20] ci: build.sh: Join MESON_ARGS and MESON_OPTS
On Mon, Feb 06, 2023 at 02:53:06PM +0100, Erik Skultety wrote: > It is quite confusing seeing these two in a call like this one: > $ meson build $MESON_OPTS $MESON_ARGS > > One has to ask 'how are they different' and 'shouldn't these be > merged'. In fact, these variables hold very different things and we > should make it more obvious. The problem is that renaming MESON_OPTS to > something more meaningful, like 'MESON_CROSS_OPTS' which is what > MESON_OPTS really does would require changes to lcitool and would > impact Dockerfile generation which in turn might have an impact on > other projects which rely on this lcitool functionality which is risky. > > Instead, provide a docstring for the former tu supplement the latter s/tu/to/ > and join the two variables in a single one MESON_ARGS which is then > passed to meson's command line so it's a little less confusing. > > Signed-off-by: Erik Skultety > --- > ci/build.sh | 9 - > 1 file changed, 8 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[libvirt PATCH 09/20] ci: build.sh: Join MESON_ARGS and MESON_OPTS
It is quite confusing seeing these two in a call like this one: $ meson build $MESON_OPTS $MESON_ARGS One has to ask 'how are they different' and 'shouldn't these be merged'. In fact, these variables hold very different things and we should make it more obvious. The problem is that renaming MESON_OPTS to something more meaningful, like 'MESON_CROSS_OPTS' which is what MESON_OPTS really does would require changes to lcitool and would impact Dockerfile generation which in turn might have an impact on other projects which rely on this lcitool functionality which is risky. Instead, provide a docstring for the former tu supplement the latter and join the two variables in a single one MESON_ARGS which is then passed to meson's command line so it's a little less confusing. Signed-off-by: Erik Skultety --- ci/build.sh | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/ci/build.sh b/ci/build.sh index 2a83f756d5..322aff2632 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -7,8 +7,15 @@ export VIR_TEST_DEBUG=1 # $MESON_OPTS is an env that can optionally be set in the container, # populated at build time from the Dockerfile. A typical use case would # be to pass options to trigger cross-compilation +# +# $MESON_ARGS correspond to meson's setup args, i.e. configure args. It's +# populated either from a GitLab's job configuration or from command line as +# `$ helper build --meson-configure-args=-Dopt1 -Dopt2` when run in a local +# containerized environment -meson setup build --werror -Dsystem=true $MESON_OPTS $MESON_ARGS || \ +MESON_ARGS="$MESON_ARGS $MESON_OPTS" + +meson setup build --werror -Dsystem=true $MESON_ARGS || \ (cat build/meson-logs/meson-log.txt && exit 1) meson compile -C build $MESON_BUILD_ARGS -- 2.39.1
