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 <eskultet(a)redhat.com&gt; --- ci/build.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)

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.

Unless we run it as 'sh ci/build.sh' in .gitlab-ci.yml recipes and instead opt into doing 'chmod +x && ci/build.sh' it will cause meson dist build to fail with a fatal error about having uncommitted changes in the repo. Therefore, let's just make the script executable, it's the most straightforward solution. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/build.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 ci/build.sh diff --git a/ci/build.sh b/ci/build.sh old mode 100644 new mode 100755

This is a follow up to: https://listman.redhat.com/archives/libvir-list/2023-January/237201.html The effort here is to unify the way builds/tests are executed in GitLab CI vs local container executions and make another step forward in terms of reproducibility of (specifically) GitLab environments. Even though code to run all but one (coverity) jobs from GitLab via the build.sh script is added with this series, local behavior remains the same as before this series. The reason for that is that that will require more patches ridding of the Makefile which is currently used and instead integrate usage of lcitool with the ci/helper Python script which is currently the entry point for local container executions.

.gitlab-ci.yml | 56 ++++++++++------------- ci/Makefile | 16 ++++--- ci/build.sh | 121 +++++++++++++++++++++++++++++++++++++++++++------ ci/helper | 21 ++++++--- 4 files changed, 155 insertions(+), 59 deletions(-) mode change 100644 => 100755 ci/build.sh

On Mon, Feb 06, 2023 at 03:45:12PM +0000, Daniel P. Berrangé wrote: > On Mon, Feb 06, 2023 at 02:52:57PM +0100, Erik Skultety wrote: > > This is a follow up to: > > https://listman.redhat.com/archives/libvir-list/2023-January/237201.html > > > > The effort here is to unify the way builds/tests are executed in GitLab CI vs > > local container executions and make another step forward in terms of > > reproducibility of (specifically) GitLab environments. > > > > Even though code to run all but one (coverity) jobs from GitLab via the > > build.sh script is added with this series, local behavior remains the same as > > before this series. The reason for that is that that will require more patches > > ridding of the Makefile which is currently used and instead integrate usage of > > lcitool with the ci/helper Python script which is currently the entry point for > > local container executions. > > snip > > > .gitlab-ci.yml | 56 ++++++++++------------- > > ci/Makefile | 16 ++++--- > > ci/build.sh | 121 +++++++++++++++++++++++++++++++++++++++++++------ > > ci/helper | 21 ++++++--- > > 4 files changed, 155 insertions(+), 59 deletions(-) > > mode change 100644 => 100755 ci/build.sh > > I'm really super unenthusiastic about this change, and also the similar > change to add an ci/integration.sh. Shell scripting is something we > worked hard to eliminate from libvirt. It is an awful language > to do anything non-trivial with, error handling, lack of data > structures, variable handling, portability are all bug generators. > > I know the .gitlab-ci.yml 'script' commands are technically shell > script, but they are pretty trivial bits and don't have to worry > about portability for dash vs bash etc, or complex control logic. > The majority of it is just a simple list of commands to invoke, > with the occasional conditional. > > The build.sh script is by contrast significantly more complex. By > the very nature of taking "N" separate gitlab jobs and multiplexing > them onto the one shell script, we're now having todo command line > arg parsing in shell and de-multiplexing back to commands. The CI > logic is now split up across even more sources - the gitlab config, > the build.sh script and the meson.build files, which I think is > worse for maintaining this too. True, except that even despite that, and I know what I'm talking about since I wrote the integration jobs templates, GitLab is super picky; you can't debug the syntax problems properly; certain common syntactic sugars don't work and have to be replaced by less common counterparts; using YAML for bash formatting leads to many table-flipping moments; you have to wait for Nx10 minutes before it fails with some ridiculous error you would have caught early locally but you couldn't because of the stage and artifact dependencies. So, once you have it finally in place it's unarguably nice, but the road towards that isn't really rainbow and unicorns either, it's still shell after all. Since I'm mainly working on this part of libvirt, I myself would appreciate if I could easily just run a single script instead of copy pasting commands one-by-one to a local VM to catch stupid errors quickly rather than wait for GitLab to fail. > > I appreciate the goal of being able to run CI commands locally, but > I'm not feeling this approach is a net win. I'd much rather just > having developers invoke the meson command directly, which is not > that hard. Well, the big picture here is about making running integration tests locally less painful than it is now...plus increase the amount of automation involved. That's the major driver here - libvirt needs to be built locally in a safe environment before integration tests could be run against it without touching the host. So, with this script, while I agree with everything you said about Bash, was just one step towards getting the automation I mentioned in place. I also considered Python (yeah..why), but that would be super akward. TL;DR: if we don't do anything about how we currently maintain the build recipes (note we're maintaining 2 already), everybody will continue ignoring the integration test suite, unless we enable merge requests where the status quo would be somewhat (but not completely) eliminated. With integration tests you can't ignore the aspect of getting feedback early compared to waiting for GitLab CI. > > If we do really want to provide something that 100% mirrors the > gitlab CI job commands though, I'm even more convinced now that > we should be using the .gitlab-ci.yml as the canonical source. > > Since I last mentioned that idea, I found out that this should > be something we can achieve via the gitlab runner 'exec' command. Haven't heard of ^this one, but I wonder how we can get something meaningful out of it for the purposes I mentioned above. Erik

> So, how about I get rid of the multiplexing and CLI parsing by placing each job > recipe in a standalone script or even going one step further and extract the > commonalities to a separate file that would be source from within each job > script? Would you consider that meeting each other halfway? Lets consider the core CI (build + unit tests in .gitlab-ci.yml), separately from the integration test CI (ci/integration-template.yml). For the core CI, I'm just not convinced of the benefit of moving the commands out into a shell script, as the set of commands is small and straightforward. For the ingration CI though, I can see benefit because of all the command logic related to fetching and building dependancies and setup tasks. In retrospect this is a sign that we need to introduce a higher level frontend for invoking the libvirt-tck tests. We already have avocado as a frontend to replace the existing 'libvirt-tck' command line tool, but we never got rid of the latter. We should probably get rid of the existing 'libvirt-tck' cli and introduce a brand new pure python 'libvirt-tck' cli, with commands for running avocado, optionally installing deps, and all the other interesting things devs want help with. The ci/integration-template.yml could then invoke this newly re-invented 'libvirt-tck' command and eliminate most of the complexity in the integration-template.yml file.

Essentially we need to look at the gitlab CI yml files as being a very thin glue layer, and anything interesting should be in the end user facing tools developers already use. Introducing a new libvirt-tck python cli is likely not a quick fix though. So if we split the ci/integration-template.yml commands out into a set of bash scripts, that'll give us a good illustration of what commands we would want in a new libvirt-tck cli impl. Then as the new libvirt-tck cli arrives, we can eliminate the bash scripts from libvirt.git

I guess this would mean dropping much of this particular series, but keeping much of the corresponding integration test series.

> As for POSIX compliance, I guess this would be a soft-requirement based on > whether shellcheck was run during review, does gitlab do something better? IIUC > there's no pre-check and you'll only know after a job was executed that there > was an incompatible statement. I think its ok if we use bash scripts as a short term solution, with a plan to move to a more supportable python impl in future. 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 :|

On Mon, Feb 06, 2023 at 03:45:12PM +0000, Daniel P. Berrangé wrote: > On Mon, Feb 06, 2023 at 02:52:57PM +0100, Erik Skultety wrote: > I appreciate the goal of being able to run CI commands locally, but > I'm not feeling this approach is a net win. I'd much rather just > having developers invoke the meson command directly, which is not > that hard. > > If we do really want to provide something that 100% mirrors the > gitlab CI job commands though, I'm even more convinced now that > we should be using the .gitlab-ci.yml as the canonical source. > > Since I last mentioned that idea, I found out that this should > be something we can achieve via the gitlab runner 'exec' command. I wonder if we've been thinking about this from the wrong angle. We've been looking at the problem of "we have a gitlab-ci.yml" and we want to be able to locally run the jobs it defines. This creates us the problem of interpreting the gitlab-ci.yml file, which is very hard. 95% of what we define in the gitlab-ci.yml file comes in via the includes of ci/gitlab.yml which is entirely auto-generated from our ci/manifest.yml file. IOW, can we rephase the problem to "we have a ci/manifest.yml" and we want to locally run the jobs it implies. We already have the code for parsing the ci/manifest.yml file, and so (mostly) know what jobs we expect to have. What we're missing is the project specific build rules which still live in the hand crafted .gitlab-ci.yml. If we could get the project specific build rules to be expressed in the ci/manifest.yml file, then we have all the info we need to be able to run jobs locally. We could then make even the root .gitlab-ci.yml be auto-generated, which would be nice.

If we have the project build rules in ci/manifest.yml, we ahve lots of flexibility. We can easily trigger the builds in containers or VMs, or the local host, or whatever. 'lcitool' already has the command for runing builds in VMs, but that relies on build rules we defined separately in libvirt-ci.git and we've somewhat neglected the VM build logic since adopting containers. Essentially ci/manifest.yml would become a fully self-contained canonical representation of any info needed to run builds, and be independant of gitlab CI, or any other CI framework we might like to plug into in the future. So 'lcitool' can be the entrypoint for triggering builds that 100% match what gets run in gitlab Yes, this only addresses the core build/unittest side of things.