[libvirt PATCH 00/33] ci: Unify the GitLab CI jobs with local executions && adopt lcitool container executions
Technically a v2 of: https://listman.redhat.com/archives/libvir-list/2023-February/237552.html However, the approach here is slightly different and what that series said about migration to lcitool container executions as a replacement for ci/Makefile is actually done here. One of the core problems of the above pointed out in review was that more Shell logic was introduced including CLI parsing, conditional executions, etc. which we fought hard to get rid of in the past. I reworked the Shell functions quite a bit and dropped whatever extra Shell logic the original series added. Obviously we can't get rid of Shell completely because of .gitlab-ci.yml and so I merely extracted the recipes into functions which are then sourced as ci/build.sh and executed. Now, that on its own would hide the actual commands being run in the GitLab job log, so before any command is actually executed, it is formatted with a color sequence so we don't miss that information as that would be a regression to the status quo. Lastly, this series then takes the effort inside the ci/build.sh script and basically mirrors whatever GitLab would do to run a job inside a local container which is executed by lcitool (yes, we already have that capability). Please give this a try and I'm already looking forward to comments as I'd like to expand this effort to local VM executions running the TCK integration tests, so this series is quite important in that regard. Erik Skultety (33): ci: build.sh: Add variables from .gitlab-ci.yml ci: build.sh: Add GIT_ROOT env helper variable ci: build.sh: Don't mention that MESON_ARGS are available via CLI ci: build.sh: Add a wrapper function over meson's setup ci: build.sh: Add a wrapper function executing 'shell' commands ci: build.sh: Add a wrapper function over the 'build' job ci: build.sh: Add a helper function to create the dist tarball ci: build.sh: Add a wrapper function over the 'test' job ci: build.sh: Add a wrapper function over the 'codestyle' job ci: build.sh: Add a wrapper function over the 'potfile' job ci: build.sh: Add a wrapper function over the 'rpmbuild' job ci: build.sh: Add a wrapper function over the 'website' job ci: build.sh: Drop changing working directory to CI_CONT_DIR ci: build.sh: Drop direct invocation of meson/ninja commands ci: build.sh: Drop MESON_ARGS definition from global level gitlab-ci.yml: Add 'after_script' stage to prep for artifact collection .gitlab-ci.yml: Convert the native build job to the build.sh usage .gitlab-ci.yml: Convert the cross build job to the build.sh usage .gitlab-ci.yml: Convert the website build job to the build.sh usage .gitlab-ci.yml: Convert the codestyle job to the build.sh usage .gitlab-ci.yml: Convert the potfile job to the build.sh usage ci: helper: Drop _lcitool_get_targets method ci: helper: Don't make ':' literal a static part of the image tag ci: helper: Add --lcitool-path CLI option ci: helper: Add a job argparse subparser ci: helper: Add a required_deps higher order helper/decorator ci: helper: Add Python code hangling git clones ci: helper: Add a helper to create a local repo clone Pythonic way ci: helper: Rework _lcitool_run method logic ci: helper: Add an action to run the container workload via lcitool ci: helper: Drop original actions ci: helper: Drop the --meson-args/--ninja-args CLI options ci: helper: Drop the _make_run method .gitlab-ci.yml | 47 +++++++------ ci/build.sh | 105 +++++++++++++++++++++++++---- ci/helper | 176 ++++++++++++++++++++++++++++--------------------- 3 files changed, 218 insertions(+), 110 deletions(-) -- 2.41.0
These are common variables we wish to use in containerized environments both in GitLab and locally. Having these defined in a single place rather than twice is highly preferable. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ci/build.sh b/ci/build.sh index ed9b1f4473..d5586f457c 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -2,7 +2,12 @@ cd "$CI_CONT_SRCDIR" -export VIR_TEST_DEBUG=1 +export CCACHE_BASEDIR="$(pwd)" +export CCACHE_DIR="$CCACHE_BASEDIR/ccache" +export CCACHE_MAXSIZE="500M" +export PATH="$CCACHE_WRAPPERSDIR:$PATH" +export VIR_TEST_VERBOSE="1" +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 -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:09PM +0200, Erik Skultety wrote:
These are common variables we wish to use in containerized environments both in GitLab and locally. Having these defined in a single place rather than twice is highly preferable.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ci/build.sh b/ci/build.sh index ed9b1f4473..d5586f457c 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -2,7 +2,12 @@
cd "$CI_CONT_SRCDIR"
-export VIR_TEST_DEBUG=1 +export CCACHE_BASEDIR="$(pwd)" +export CCACHE_DIR="$CCACHE_BASEDIR/ccache" +export CCACHE_MAXSIZE="500M" +export PATH="$CCACHE_WRAPPERSDIR:$PATH" +export VIR_TEST_VERBOSE="1" +export VIR_TEST_DEBUG="1"
These last two vars are good for CI, because in a non-interactive env we need the verbose / debug output upfront to stand a chance of diagnosing problems from logs after the fact If I'm running a build locally with our ci/Makefile, I don't especially want it to be forced into verbose mode by default, as I have an interactive shell I can do that as & when needed myself. Perhaps we can do if ! test -t 1 then export VIR_TEST_VERBOSE="1" export VIR_TEST_DEBUG="1" fi such that we only set the verbose vars when STDOUT is NOT a TTY. 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 Thu, Aug 31, 2023 at 05:45:05PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:09PM +0200, Erik Skultety wrote:
These are common variables we wish to use in containerized environments both in GitLab and locally. Having these defined in a single place rather than twice is highly preferable.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ci/build.sh b/ci/build.sh index ed9b1f4473..d5586f457c 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -2,7 +2,12 @@
cd "$CI_CONT_SRCDIR"
-export VIR_TEST_DEBUG=1 +export CCACHE_BASEDIR="$(pwd)" +export CCACHE_DIR="$CCACHE_BASEDIR/ccache" +export CCACHE_MAXSIZE="500M" +export PATH="$CCACHE_WRAPPERSDIR:$PATH" +export VIR_TEST_VERBOSE="1" +export VIR_TEST_DEBUG="1"
These last two vars are good for CI, because in a non-interactive env we need the verbose / debug output upfront to stand a chance of diagnosing problems from logs after the fact
If I'm running a build locally with our ci/Makefile, I don't especially want it to be forced into verbose mode by default, as I have an interactive shell I can do that as & when needed myself.
Perhaps we can do
if ! test -t 1 then export VIR_TEST_VERBOSE="1" export VIR_TEST_DEBUG="1" fi
This uncovered an interesting bug in the ci/helper + lcitool combo. In lcitool we explicitly always ask for a TTY with --tty on podman/docker CLI. If I drop the argument and interrupt the execution I'll get an unhandled OSError exception from lcitool and a stacktrace from ci/helper which is nicely interlaced with meson's own stacktrace if you interrupt the setup phase. I need to look into lcitool whether there's an easy fix. The other temporary solution until we incorporate the ci/helper logic into lcitool would be to set another variable inside the temporary crafted script by ci/helper that would serve the same purpose. Erik
We'll use this one in many of the job functions future patches will introduce, it's a neat shortcut to avoid using relative paths. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index d5586f457c..152c222244 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -9,6 +9,8 @@ export PATH="$CCACHE_WRAPPERSDIR:$PATH" export VIR_TEST_VERBOSE="1" export VIR_TEST_DEBUG="1" +GIT_ROOT="$(git rev-parse --show-toplevel)" + # $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 -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:10PM +0200, Erik Skultety wrote:
We'll use this one in many of the job functions future patches will introduce, it's a neat shortcut to avoid using relative paths.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Previous patches have removed the code that allowed injecting arbitrary meson arguments, same for ninja args. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ci/build.sh b/ci/build.sh index 152c222244..82016ba29c 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -16,9 +16,7 @@ GIT_ROOT="$(git rev-parse --show-toplevel)" # 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 +# populated from a GitLab's job configuration MESON_ARGS="$MESON_ARGS $MESON_OPTS" -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:11PM +0200, Erik Skultety wrote:
Previous patches have removed the code that allowed injecting arbitrary meson arguments, same for ninja args.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
The reason for this wrapper is that all job functions introduced in future patches will refer to this one instead of open-coding the same 'meson setup' invocation N times. It also prevents 'setup' to be called multiple times as some future job functions might actually do just that in a transitive manner. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index 82016ba29c..02ff1a8388 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -24,3 +24,20 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \ (cat build/meson-logs/meson-log.txt && exit 1) ninja -C build $NINJA_ARGS + +run_meson_setup() { + if [ -d "${GIT_ROOT}/build/meson-private" ]; then + return + fi + + local DUMP_ERR_CMD="cat ${GIT_ROOT}/build/meson-logs/meson-log.txt" + local SETUP_CMD="meson \ + setup \ + build \ + --werror \ + -Dsystem=true $MESON_OPTS $MESON_ARGS" + + local CMD="$SETUP_CMD || ($DUMP_ERR_CMD && exit 1)" + + run_cmd "$CMD" +} -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:12PM +0200, Erik Skultety wrote:
The reason for this wrapper is that all job functions introduced in future patches will refer to this one instead of open-coding the same 'meson setup' invocation N times. It also prevents 'setup' to be called multiple times as some future job functions might actually do just that in a transitive manner.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 82016ba29c..02ff1a8388 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -24,3 +24,20 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \ (cat build/meson-logs/meson-log.txt && exit 1)
ninja -C build $NINJA_ARGS + +run_meson_setup() { + if [ -d "${GIT_ROOT}/build/meson-private" ]; then + return + fi
This isn't as accurate as meson's own check. I interrupted meson part way through, and this directry exists, but meson had written out sufficient files todo a build. Re-running 'meson setup build' directly happily runs, but this run_meson_setup command becomes a no-op which is kinda annoying.
+ + local DUMP_ERR_CMD="cat ${GIT_ROOT}/build/meson-logs/meson-log.txt" + local SETUP_CMD="meson \ + setup \ + build \ + --werror \ + -Dsystem=true $MESON_OPTS $MESON_ARGS" + + local CMD="$SETUP_CMD || ($DUMP_ERR_CMD && exit 1)" + + run_cmd "$CMD" +} -- 2.41.0
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 Thu, Aug 31, 2023 at 05:37:47PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:12PM +0200, Erik Skultety wrote:
The reason for this wrapper is that all job functions introduced in future patches will refer to this one instead of open-coding the same 'meson setup' invocation N times. It also prevents 'setup' to be called multiple times as some future job functions might actually do just that in a transitive manner.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 82016ba29c..02ff1a8388 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -24,3 +24,20 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \ (cat build/meson-logs/meson-log.txt && exit 1)
ninja -C build $NINJA_ARGS + +run_meson_setup() { + if [ -d "${GIT_ROOT}/build/meson-private" ]; then + return + fi
This isn't as accurate as meson's own check. I interrupted meson part way through, and this directry exists, but meson had written out sufficient files todo a build. Re-running 'meson setup build' directly happily runs, but this run_meson_setup command becomes a no-op which is kinda annoying.
Perhaps we can just push the test into the caller. eg test -d $GIT_ROOT/build || run_meson_setup so commands like 'run_build' "just work", but if someone explicitly invokes 'run_meson_setup' we just let meson detect a pre-created build dir using its native logic.
+ + local DUMP_ERR_CMD="cat ${GIT_ROOT}/build/meson-logs/meson-log.txt" + local SETUP_CMD="meson \ + setup \ + build \ + --werror \ + -Dsystem=true $MESON_OPTS $MESON_ARGS" + + local CMD="$SETUP_CMD || ($DUMP_ERR_CMD && exit 1)" + + run_cmd "$CMD" +} -- 2.41.0
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 :|
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 :|
This would normally be not needed at all, but the problem here is the Shell-in-YAML which GitLab interprets. It outputs every command that appears as a line in the 'script' segment in a color-coded fashion for easy identification of problems. Well, that useful feature is lost when there's indirection and one script calls into another in which case it would only output the respective script name which would make failure investigation harder. This simple helper tackles that by echoing the command to be run by any script/function with a color escape sequence so that we don't lose track of the *actual* shell commands being run as part of the GitLab job pipelines. An example of what the output then might look like: [RUN COMMAND]: 'meson compile -C build install-web' Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index 02ff1a8388..d4fbf0ab37 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -25,6 +25,12 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \ ninja -C build $NINJA_ARGS +run_cmd() { + local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces + + printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$CMD"; eval "$CMD" +} + run_meson_setup() { if [ -d "${GIT_ROOT}/build/meson-private" ]; then return -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
This would normally be not needed at all, but the problem here is the Shell-in-YAML which GitLab interprets. It outputs every command that appears as a line in the 'script' segment in a color-coded fashion for easy identification of problems. Well, that useful feature is lost when there's indirection and one script calls into another in which case it would only output the respective script name which would make failure investigation harder. This simple helper tackles that by echoing the command to be run by any script/function with a color escape sequence so that we don't lose track of the *actual* shell commands being run as part of the GitLab job pipelines. An example of what the output then might look like: [RUN COMMAND]: 'meson compile -C build install-web'
FWIW, I happened to come across this https://docs.gitlab.com/ee/ci/jobs/index.html#custom-collapsible-sections which lets scripts output messages in a special format that get turned into collapsible sections when browsing logs in gitlab UI. Not sure it is something we'd use in this specific patch, but might be conceptually useful somewhere.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 02ff1a8388..d4fbf0ab37 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -25,6 +25,12 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \
ninja -C build $NINJA_ARGS
+run_cmd() { + local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces + + printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$CMD"; eval "$CMD" +} + run_meson_setup() { if [ -d "${GIT_ROOT}/build/meson-private" ]; then return -- 2.41.0
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 Thu, Aug 31, 2023 at 03:30:23PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
This would normally be not needed at all, but the problem here is the Shell-in-YAML which GitLab interprets. It outputs every command that appears as a line in the 'script' segment in a color-coded fashion for easy identification of problems. Well, that useful feature is lost when there's indirection and one script calls into another in which case it would only output the respective script name which would make failure investigation harder. This simple helper tackles that by echoing the command to be run by any script/function with a color escape sequence so that we don't lose track of the *actual* shell commands being run as part of the GitLab job pipelines. An example of what the output then might look like: [RUN COMMAND]: 'meson compile -C build install-web'
FWIW, I happened to come across this
https://docs.gitlab.com/ee/ci/jobs/index.html#custom-collapsible-sections
which lets scripts output messages in a special format that get turned into collapsible sections when browsing logs in gitlab UI. Not sure it is something we'd use in this specific patch, but might be conceptually useful somewhere.
This actually works kinda nicely. I added this commit on top of yours: https://gitlab.com/berrange/libvirt/-/commit/29f123024c265262e70a409daa00c86... and an example pipeline is: https://gitlab.com/berrange/libvirt/-/jobs/4997176074 Notice there are three collapsible sections in the job logs "Running 'meson setup'" "Running 'meson build'" "Running 'meson test'" collapsing earlier sections makes it somewhat easier to find the phase you're interested in, though its doesn't work so well when gitlab truncates the earlier part of the log that's over 500k. Not a problem for pipelines on master, but bad for forks. 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 Fri, Sep 01, 2023 at 10:46:06AM +0100, Daniel P. Berrangé wrote:
On Thu, Aug 31, 2023 at 03:30:23PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
This would normally be not needed at all, but the problem here is the Shell-in-YAML which GitLab interprets. It outputs every command that appears as a line in the 'script' segment in a color-coded fashion for easy identification of problems. Well, that useful feature is lost when there's indirection and one script calls into another in which case it would only output the respective script name which would make failure investigation harder. This simple helper tackles that by echoing the command to be run by any script/function with a color escape sequence so that we don't lose track of the *actual* shell commands being run as part of the GitLab job pipelines. An example of what the output then might look like: [RUN COMMAND]: 'meson compile -C build install-web'
FWIW, I happened to come across this
https://docs.gitlab.com/ee/ci/jobs/index.html#custom-collapsible-sections
which lets scripts output messages in a special format that get turned into collapsible sections when browsing logs in gitlab UI. Not sure it is something we'd use in this specific patch, but might be conceptually useful somewhere.
This actually works kinda nicely. I added this commit on top of yours:
https://gitlab.com/berrange/libvirt/-/commit/29f123024c265262e70a409daa00c86...
and an example pipeline is:
https://gitlab.com/berrange/libvirt/-/jobs/4997176074
Notice there are three collapsible sections in the job logs
"Running 'meson setup'" "Running 'meson build'" "Running 'meson test'"
collapsing earlier sections makes it somewhat easier to find the phase you're interested in, though its doesn't work so well when gitlab truncates the earlier part of the log that's over 500k. Not a problem for pipelines on master, but bad for forks.
Not bad. We could improve on it and go as far as print the actual command in the section header and fallback to what I proposed in local environments. Let's finish this series first and then you can refine and post your patch as a follow up improvement. Erik
On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
This would normally be not needed at all, but the problem here is the Shell-in-YAML which GitLab interprets. It outputs every command that appears as a line in the 'script' segment in a color-coded fashion for easy identification of problems. Well, that useful feature is lost when there's indirection and one script calls into another in which case it would only output the respective script name which would make failure investigation harder. This simple helper tackles that by echoing the command to be run by any script/function with a color escape sequence so that we don't lose track of the *actual* shell commands being run as part of the GitLab job pipelines. An example of what the output then might look like: [RUN COMMAND]: 'meson compile -C build install-web'
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 02ff1a8388..d4fbf0ab37 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -25,6 +25,12 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \
ninja -C build $NINJA_ARGS
+run_cmd() { + local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces + + printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$CMD"; eval "$CMD" +}
Bright yellow is unfortunately almost unreadable when used on a terminal with white/light coloured background. Using green '32m' ought to be visible in both light/dark terminals reasonabley well.
+ run_meson_setup() { if [ -d "${GIT_ROOT}/build/meson-private" ]; then return -- 2.41.0
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 Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
This would normally be not needed at all, but the problem here is the Shell-in-YAML which GitLab interprets. It outputs every command that appears as a line in the 'script' segment in a color-coded fashion for easy identification of problems. Well, that useful feature is lost when there's indirection and one script calls into another in which case it would only output the respective script name which would make failure investigation harder. This simple helper tackles that by echoing the command to be run by any script/function with a color escape sequence so that we don't lose track of the *actual* shell commands being run as part of the GitLab job pipelines. An example of what the output then might look like: [RUN COMMAND]: 'meson compile -C build install-web'
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 02ff1a8388..d4fbf0ab37 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -25,6 +25,12 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \
ninja -C build $NINJA_ARGS
+run_cmd() { + local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces + + printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$CMD"; eval "$CMD" +}
I think we sould just get rid of the $CMD env variable in the caller entirely and pass in arguments individual. eg so this method becomes run_cmd() { printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$*" $@ } Then in the callers instead of local CMD="meson compile -C build $BUILD_ARGS" run_cmd "$CMD" We get run_cmd meson compile -C build "$BUILD_ARGS" this would have avoided the bug you just posted a fix for where we set 'local CMD' but forget the actual 'run_cmd' call. 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 Fri, Sep 01, 2023 at 10:10:55AM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
This would normally be not needed at all, but the problem here is the Shell-in-YAML which GitLab interprets. It outputs every command that appears as a line in the 'script' segment in a color-coded fashion for easy identification of problems. Well, that useful feature is lost when there's indirection and one script calls into another in which case it would only output the respective script name which would make failure investigation harder. This simple helper tackles that by echoing the command to be run by any script/function with a color escape sequence so that we don't lose track of the *actual* shell commands being run as part of the GitLab job pipelines. An example of what the output then might look like: [RUN COMMAND]: 'meson compile -C build install-web'
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 02ff1a8388..d4fbf0ab37 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -25,6 +25,12 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \
ninja -C build $NINJA_ARGS
+run_cmd() { + local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces + + printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$CMD"; eval "$CMD" +}
I think we sould just get rid of the $CMD env variable in the caller entirely and pass in arguments individual. eg so this method becomes
run_cmd() { printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$*" $@ }
Then in the callers instead of
local CMD="meson compile -C build $BUILD_ARGS" run_cmd "$CMD"
We get
run_cmd meson compile -C build "$BUILD_ARGS"
this would have avoided the bug you just posted a fix for where we set 'local CMD' but forget the actual 'run_cmd' call.
My only complaint is, again, readability - this particular example is fine, it would IMO become a mess with commands taking several arguments which would not fit onto a single line. I don't expect these functions to change much, so while you're absolutely right about preventing bugs like that, I think having some reasonable readability (shell--) would be a good tradeoff. Erik
On Fri, Sep 01, 2023 at 11:16:09AM +0200, Erik Skultety wrote:
On Fri, Sep 01, 2023 at 10:10:55AM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
This would normally be not needed at all, but the problem here is the Shell-in-YAML which GitLab interprets. It outputs every command that appears as a line in the 'script' segment in a color-coded fashion for easy identification of problems. Well, that useful feature is lost when there's indirection and one script calls into another in which case it would only output the respective script name which would make failure investigation harder. This simple helper tackles that by echoing the command to be run by any script/function with a color escape sequence so that we don't lose track of the *actual* shell commands being run as part of the GitLab job pipelines. An example of what the output then might look like: [RUN COMMAND]: 'meson compile -C build install-web'
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 02ff1a8388..d4fbf0ab37 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -25,6 +25,12 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \
ninja -C build $NINJA_ARGS
+run_cmd() { + local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces + + printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$CMD"; eval "$CMD" +}
I think we sould just get rid of the $CMD env variable in the caller entirely and pass in arguments individual. eg so this method becomes
run_cmd() { printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$*" $@ }
Then in the callers instead of
local CMD="meson compile -C build $BUILD_ARGS" run_cmd "$CMD"
We get
run_cmd meson compile -C build "$BUILD_ARGS"
this would have avoided the bug you just posted a fix for where we set 'local CMD' but forget the actual 'run_cmd' call.
My only complaint is, again, readability - this particular example is fine, it would IMO become a mess with commands taking several arguments which would not fit onto a single line. I don't expect these functions to change much, so while you're absolutely right about preventing bugs like that, I think having some reasonable readability (shell--) would be a good tradeoff.
A line continuation isn't that hard to add for comamnds which get too long. Avoiding the intermediate variable is also more robust by eliminating two levels of extra quoting. Quoting/expansion problems are one of the more painful aspects of working in shell, so I think its good to cull extra variables where practical. 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 :|
This helper is a shell function transcript of its original GitLab CI counterpart. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index d4fbf0ab37..c2f54707ce 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -47,3 +47,10 @@ run_meson_setup() { run_cmd "$CMD" } + +run_build() { + local CMD="meson compile -C build $BUILD_ARGS" + + run_meson_setup + run_cmd "$CMD" +} -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:14PM +0200, Erik Skultety wrote:
This helper is a shell function transcript of its original GitLab CI counterpart.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index d4fbf0ab37..c2f54707ce 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -47,3 +47,10 @@ run_meson_setup() {
run_cmd "$CMD" } + +run_build() { + local CMD="meson compile -C build $BUILD_ARGS" + + run_meson_setup
Per the previous patch, I think we should do test -d $GIT_ROOT/build/build.ninja || run_meson_setup so that 'run_meson_setup' can do its thing unconditionally if invoked explicitly
+ run_cmd "$CMD" +}
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 :|
This helper function does not correspond to a particular GitLab job, it just logically separates the necessary step of creating a dist tarball from the RPM build job that takes over. One notable change here is the need to update git's file index which causes issues in local container executions which rely on a shallow copy of the libvirt repo created as: $ git clone --local Even if all changes have been committed, git often complained otherwise. Updating the index in a GitLab environment is a NOP. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index c2f54707ce..c56e45f51f 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -54,3 +54,15 @@ run_build() { run_meson_setup run_cmd "$CMD" } + +run_dist() { + local CMD="meson dist -C build --no-tests" + + run_meson_setup + + # dist is unhappy in local container environment complaining about + # uncommitted changes in the repo which is often not the case - refreshing + # git's index solves the problem + git update-index --refresh + run_cmd "$CMD" +} -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:15PM +0200, Erik Skultety wrote:
This helper function does not correspond to a particular GitLab job, it just logically separates the necessary step of creating a dist tarball from the RPM build job that takes over. One notable change here is the need to update git's file index which causes issues in local container executions which rely on a shallow copy of the libvirt repo created as:
$ git clone --local
Even if all changes have been committed, git often complained otherwise. Updating the index in a GitLab environment is a NOP.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index c2f54707ce..c56e45f51f 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -54,3 +54,15 @@ run_build() { run_meson_setup run_cmd "$CMD" } + +run_dist() { + local CMD="meson dist -C build --no-tests" + + run_meson_setup
Same comment as prev patch about conditionally invoking this
+ + # dist is unhappy in local container environment complaining about + # uncommitted changes in the repo which is often not the case - refreshing + # git's index solves the problem + git update-index --refresh
This is all very peculiar but I concur, this does appear to "solve" it for our needs.
+ run_cmd "$CMD" +} -- 2.41.0
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 :|
This helper is a shell function transcript of its original GitLab CI counterpart. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index c56e45f51f..ab471f2741 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -66,3 +66,10 @@ run_dist() { git update-index --refresh run_cmd "$CMD" } + +run_test() { + local CMD="meson test -C build $TEST_ARGS" + + run_meson_setup + run_cmd "$CMD" +} -- 2.41.0
This helper is a shell function transcript of its original GitLab CI counterpart. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index ab471f2741..b2891a0c0f 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -73,3 +73,12 @@ run_test() { run_meson_setup run_cmd "$CMD" } + +run_codestyle() { + BUILD_ARGS="libvirt-pot-dep" + TEST_ARGS="--suite syntax-check --no-rebuild --print-errorlogs" + + run_meson_setup + run_build + run_test +} -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:17PM +0200, Erik Skultety wrote:
This helper is a shell function transcript of its original GitLab CI counterpart.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index ab471f2741..b2891a0c0f 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -73,3 +73,12 @@ run_test() { run_meson_setup run_cmd "$CMD" } + +run_codestyle() { + BUILD_ARGS="libvirt-pot-dep" + TEST_ARGS="--suite syntax-check --no-rebuild --print-errorlogs" + + run_meson_setup + run_build
run_build already implies run_meson_setup so we can drop it in this case.
+ run_test +}
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 :|
This helper is a shell function transcript of its original GitLab CI counterpart. There's one notable difference such that we pass '-j1' to the meson compile command otherwise we'd have to execute the 'run_build' function twice, passing 'libvirt-pot-dep' and 'libvirt-pot' targets in a serial manner. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index b2891a0c0f..6990f2d171 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -82,3 +82,15 @@ run_codestyle() { run_build run_test } + +run_potfile() { + # since meson would run jobs for each of the following target in parallel, + # we'd have dependency issues such that one target might depend on a + # generated file which hasn't been generated yet by the other target, hence + # we limit potfile job to a single build job (luckily potfile build has + # negligible performance impact) + BUILD_ARGS="-j1 libvirt-pot-dep libvirt-pot" + + run_meson_setup + run_build +} -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:18PM +0200, Erik Skultety wrote:
This helper is a shell function transcript of its original GitLab CI counterpart. There's one notable difference such that we pass '-j1' to the meson compile command otherwise we'd have to execute the 'run_build' function twice, passing 'libvirt-pot-dep' and 'libvirt-pot' targets in a serial manner.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index b2891a0c0f..6990f2d171 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -82,3 +82,15 @@ run_codestyle() { run_build run_test } + +run_potfile() { + # since meson would run jobs for each of the following target in parallel, + # we'd have dependency issues such that one target might depend on a + # generated file which hasn't been generated yet by the other target, hence + # we limit potfile job to a single build job (luckily potfile build has + # negligible performance impact) + BUILD_ARGS="-j1 libvirt-pot-dep libvirt-pot" + + run_meson_setup + run_build
run_meson_setup is redundant here too as implied by run_build Rather than -j1 could we instead invoke 'run_build' twice eg: BUILD_ARGS=libvirt-pot-dep run_build BUILD_ARGS=libvirt-pot run_build
+}
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 Thu, Aug 31, 2023 at 05:55:21PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:18PM +0200, Erik Skultety wrote:
This helper is a shell function transcript of its original GitLab CI counterpart. There's one notable difference such that we pass '-j1' to the meson compile command otherwise we'd have to execute the 'run_build' function twice, passing 'libvirt-pot-dep' and 'libvirt-pot' targets in a serial manner.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index b2891a0c0f..6990f2d171 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -82,3 +82,15 @@ run_codestyle() { run_build run_test } + +run_potfile() { + # since meson would run jobs for each of the following target in parallel, + # we'd have dependency issues such that one target might depend on a + # generated file which hasn't been generated yet by the other target, hence + # we limit potfile job to a single build job (luckily potfile build has + # negligible performance impact) + BUILD_ARGS="-j1 libvirt-pot-dep libvirt-pot" + + run_meson_setup + run_build
run_meson_setup is redundant here too as implied by run_build
Rather than -j1 could we instead invoke 'run_build' twice eg:
BUILD_ARGS=libvirt-pot-dep run_build BUILD_ARGS=libvirt-pot run_build
There was a reason why I did it that way, but I can't remember what it was. Unless I manage to remember, I'll revert back to what you suggest. Erik
This helper is a shell function transcript of its original GitLab CI counterpart. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index 6990f2d171..30f4712e4b 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -94,3 +94,14 @@ run_potfile() { run_meson_setup run_build } + +run_rpmbuild() { + local CMD="rpmbuild \ + --clean \ + --nodeps \ + --define "_without_mingw 1" \ + -ta build/meson-dist/libvirt-*.tar.xz" + + run_meson_setup + run_dist +} -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:19PM +0200, Erik Skultety wrote:
This helper is a shell function transcript of its original GitLab CI counterpart.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 6990f2d171..30f4712e4b 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -94,3 +94,14 @@ run_potfile() { run_meson_setup run_build } + +run_rpmbuild() { + local CMD="rpmbuild \ + --clean \ + --nodeps \ + --define "_without_mingw 1" \ + -ta build/meson-dist/libvirt-*.tar.xz" + + run_meson_setup
Redundant here as implied by run_dist
+ run_dist +}
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 Thu, Aug 31, 2023 at 05:55:54PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:19PM +0200, Erik Skultety wrote:
This helper is a shell function transcript of its original GitLab CI counterpart.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 6990f2d171..30f4712e4b 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -94,3 +94,14 @@ run_potfile() { run_meson_setup run_build } + +run_rpmbuild() { + local CMD="rpmbuild \ + --clean \ + --nodeps \ + --define "_without_mingw 1" \ + -ta build/meson-dist/libvirt-*.tar.xz" + + run_meson_setup
Redundant here as implied by run_dist
While I can drop all these redundant setup calls, the reason why I put them there was simply readability. Let's face it Shell isn't a particularly nice structured language to look at especially when it comes to functions, so rather than having everyone go and look what run_dist/run_build, etc. do, I instead opted for transparent naming and explicit function calls, IOW so that when you look at run_rpmbuild you immediately know we run meson setup followed by a project build, etc., but then if you call run_dist alone in a local interactive environment then run_dist would also call meson_setup because it can't go without it, so hence the redundancy in all the functions. If you still feel like it's undesirable even in ^this case, I will drop the calls. Erik
On Fri, Sep 01, 2023 at 09:29:13AM +0200, Erik Skultety wrote:
On Thu, Aug 31, 2023 at 05:55:54PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:19PM +0200, Erik Skultety wrote:
This helper is a shell function transcript of its original GitLab CI counterpart.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 6990f2d171..30f4712e4b 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -94,3 +94,14 @@ run_potfile() { run_meson_setup run_build } + +run_rpmbuild() { + local CMD="rpmbuild \ + --clean \ + --nodeps \ + --define "_without_mingw 1" \ + -ta build/meson-dist/libvirt-*.tar.xz" + + run_meson_setup
Redundant here as implied by run_dist
While I can drop all these redundant setup calls, the reason why I put them there was simply readability. Let's face it Shell isn't a particularly nice structured language to look at especially when it comes to functions, so rather than having everyone go and look what run_dist/run_build, etc. do, I instead opted for transparent naming and explicit function calls, IOW so that when you look at run_rpmbuild you immediately know we run meson setup followed by a project build, etc., but then if you call run_dist alone in a local interactive environment then run_dist would also call meson_setup because it can't go without it, so hence the redundancy in all the functions. If you still feel like it's undesirable even in ^this case, I will drop the calls.
In practice I don't think users need to worry about what each functions runs, because your implementation has ensured that every single cmd is "self contained" and will run any dependancies is has. Given that I think it is overkill to have the transitive expansion of every dependancy, just the immediate dependency is sufficient. 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 Fri, Aug 25, 2023 at 07:55:19PM +0200, Erik Skultety wrote:
This helper is a shell function transcript of its original GitLab CI counterpart.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 6990f2d171..30f4712e4b 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -94,3 +94,14 @@ run_potfile() { run_meson_setup run_build } + +run_rpmbuild() { + local CMD="rpmbuild \ + --clean \ + --nodeps \ + --define "_without_mingw 1" \ + -ta build/meson-dist/libvirt-*.tar.xz" + + run_meson_setup + run_dist +} -- 2.41.0
Consider the following squashed in: diff --git a/ci/build.sh b/ci/build.sh index 5eef53f8d1..b990f5eeac 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -87,6 +87,7 @@ run_rpmbuild() { run_meson_setup run_dist + run_cmd "$CMD" } AND diff --git a/ci/build.sh b/ci/build.sh index b990f5eeac..a45bc2b110 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -82,7 +82,7 @@ run_rpmbuild() { local CMD="rpmbuild \ --clean \ --nodeps \ - --define "_without_mingw 1" \ + --define '_without_mingw 1' \ -ta build/meson-dist/libvirt-*.tar.xz" run_meson_setup Regards, Erik
This helper is a shell function transcript of its original GitLab CI counterpart. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index 30f4712e4b..6673ec6b18 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -105,3 +105,11 @@ run_rpmbuild() { run_meson_setup run_dist } + +run_website_build() { + export DESTDIR="$(pwd)/install" + BUILD_ARGS="install-web" + + run_meson_setup + run_build +} -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:20PM +0200, Erik Skultety wrote:
This helper is a shell function transcript of its original GitLab CI counterpart.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh index 30f4712e4b..6673ec6b18 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -105,3 +105,11 @@ run_rpmbuild() { run_meson_setup run_dist } + +run_website_build() { + export DESTDIR="$(pwd)/install"
Perhaps use ${GIT_ROOT} instead of $(pwd) to make this stable regardless of pwd
+ BUILD_ARGS="install-web" + + run_meson_setup
Redundant call
+ run_build +} --
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 :|
Firstly, this would mangle with "sourcing" this file in either execution environment later down the road. Secondly, we won't need this as future ci/helper patches will generate a throwaway script that will take care of a correct execution of a build job in a similar fashion as if the job ran in a GitLab environment. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/ci/build.sh b/ci/build.sh index 6673ec6b18..133952f706 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -1,7 +1,5 @@ #!/bin/sh -cd "$CI_CONT_SRCDIR" - export CCACHE_BASEDIR="$(pwd)" export CCACHE_DIR="$CCACHE_BASEDIR/ccache" export CCACHE_MAXSIZE="500M" -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:21PM +0200, Erik Skultety wrote:
Firstly, this would mangle with "sourcing" this file in either execution environment later down the road. Secondly, we won't need this as future ci/helper patches will generate a throwaway script that will take care of a correct execution of a build job in a similar fashion as if the job ran in a GitLab environment.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
We've moved all invocations to the respective helper function which we'll execute both from gitlab CI jobs and local environments so we don't need to have them on the global level as it would also not work with "sourcing" this file to populate the environment with function definitions. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 5 ----- 1 file changed, 5 deletions(-) diff --git a/ci/build.sh b/ci/build.sh index 133952f706..b075c49af3 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -18,11 +18,6 @@ GIT_ROOT="$(git rev-parse --show-toplevel)" MESON_ARGS="$MESON_ARGS $MESON_OPTS" -meson setup build --werror -Dsystem=true $MESON_ARGS || \ -(cat build/meson-logs/meson-log.txt && exit 1) - -ninja -C build $NINJA_ARGS - run_cmd() { local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:22PM +0200, Erik Skultety wrote:
We've moved all invocations to the respective helper function which we'll execute both from gitlab CI jobs and local environments so we don't need to have them on the global level as it would also not work with "sourcing" this file to populate the environment with function definitions.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/ci/build.sh b/ci/build.sh index 133952f706..b075c49af3 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -18,11 +18,6 @@ GIT_ROOT="$(git rev-parse --show-toplevel)"
MESON_ARGS="$MESON_ARGS $MESON_OPTS"
-meson setup build --werror -Dsystem=true $MESON_ARGS || \ -(cat build/meson-logs/meson-log.txt && exit 1) - -ninja -C build $NINJA_ARGS - run_cmd() { local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces
Now we drop immediate invokation at time of execution, I wonder if the build.sh name is a little mis-leading. Might be better renamed to 'functions.sh' or 'commands.sh' perhaps, so the name doesn't suggest that it actually builds stuff, merely that it supplies some shell logic ? 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 Thu, Aug 31, 2023 at 05:59:26PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:22PM +0200, Erik Skultety wrote:
We've moved all invocations to the respective helper function which we'll execute both from gitlab CI jobs and local environments so we don't need to have them on the global level as it would also not work with "sourcing" this file to populate the environment with function definitions.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/ci/build.sh b/ci/build.sh index 133952f706..b075c49af3 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -18,11 +18,6 @@ GIT_ROOT="$(git rev-parse --show-toplevel)"
MESON_ARGS="$MESON_ARGS $MESON_OPTS"
-meson setup build --werror -Dsystem=true $MESON_ARGS || \ -(cat build/meson-logs/meson-log.txt && exit 1) - -ninja -C build $NINJA_ARGS - run_cmd() { local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces
Now we drop immediate invokation at time of execution, I wonder if the build.sh name is a little mis-leading.
Might be better renamed to 'functions.sh' or 'commands.sh' perhaps, so the name doesn't suggest that it actually builds stuff, merely that it supplies some shell logic ?
Fair enough, didn't like the name either, but I wanted to keep at least something we had around :). Erik
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/ci/build.sh b/ci/build.sh index b075c49af3..5eef53f8d1 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -8,16 +8,6 @@ export VIR_TEST_VERBOSE="1" export VIR_TEST_DEBUG="1" GIT_ROOT="$(git rev-parse --show-toplevel)" - -# $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 from a GitLab's job configuration - -MESON_ARGS="$MESON_ARGS $MESON_OPTS" - run_cmd() { local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:23PM +0200, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/build.sh | 10 ---------- 1 file changed, 10 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
This is one of the preparation steps that if not done would otherwise collide with local container executions where we: 1) don't collect artifacts 2) are not limited by GitLab's environment and hence moving build artifacts to unusual places would only cause confusion when doing local build inspection in an interactive container shell session Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 944a7b7817..1c6af8f8b3 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -31,11 +31,16 @@ include: - if test -x /usr/bin/rpmbuild && test "$RPM" != "skip"; then rpmbuild --clean --nodeps --define "_without_mingw 1" -ta build/meson-dist/libvirt-*.tar.xz; - mv "$HOME"/rpmbuild/RPMS/x86_64/ libvirt-rpms/; else meson compile -C build; meson test -C build --no-suite syntax-check --print-errorlogs; fi + after_script: + - test "$CI_JOB_STATUS" != "success" && exit 1; + - if test -x /usr/bin/rpmbuild && test "$RPM" != "skip"; + then + mv "$HOME"/rpmbuild/RPMS/x86_64/ libvirt-rpms/; + fi .native_build_job_prebuilt_env: extends: @@ -77,6 +82,8 @@ include: - *script_variables - meson setup build --werror -Dsystem=true || (cat build/meson-logs/meson-log.txt && exit 1) - DESTDIR=$(pwd)/install meson compile -C build install-web + after_script: + - test "$CI_JOB_STATUS" != "success" && exit 1; - mv install/usr/share/doc/libvirt/html/ website artifacts: expose_as: 'Website' @@ -155,6 +162,8 @@ potfile: - meson setup build --werror || (cat build/meson-logs/meson-log.txt && exit 1) - meson compile -C build libvirt-pot-dep - meson compile -C build libvirt-pot + after_script: + - test "$CI_JOB_STATUS" != "success" && exit 1; - cp po/libvirt.pot libvirt.pot artifacts: expose_as: 'Potfile' -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:24PM +0200, Erik Skultety wrote:
This is one of the preparation steps that if not done would otherwise collide with local container executions where we: 1) don't collect artifacts 2) are not limited by GitLab's environment and hence moving build artifacts to unusual places would only cause confusion when doing local build inspection in an interactive container shell session
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 1c6af8f8b3..6a8115037d 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -25,15 +25,13 @@ include: - ccache/ key: "$CI_JOB_NAME" script: - - *script_variables - - meson setup build --werror $MESON_ARGS || (cat build/meson-logs/meson-log.txt && exit 1) - - meson dist -C build --no-tests + - source ci/build.sh - if test -x /usr/bin/rpmbuild && test "$RPM" != "skip"; then - rpmbuild --clean --nodeps --define "_without_mingw 1" -ta build/meson-dist/libvirt-*.tar.xz; + run_rpmbuild; else - meson compile -C build; - meson test -C build --no-suite syntax-check --print-errorlogs; + run_build; + run_test; fi after_script: - test "$CI_JOB_STATUS" != "success" && exit 1; -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:25PM +0200, Erik Skultety wrote:
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 6a8115037d..b59e56941c 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -56,10 +56,12 @@ include: - ccache/ key: "$CI_JOB_NAME" script: - - *script_variables - - meson setup build --werror $MESON_OPTS || (cat build/meson-logs/meson-log.txt && exit 1) - - meson compile -C build - - if test "$CROSS" = "i686" ; then meson test -C build --no-suite syntax-check --print-errorlogs ; fi + - source ci/build.sh + - run_build + - if test "$CROSS" = "i686" ; + then + run_test; + fi .cross_build_job_prebuilt_env: extends: -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:26PM +0200, Erik Skultety wrote:
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b59e56941c..0450c1f6ad 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -79,9 +79,8 @@ include: # https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=webs... .website_job: script: - - *script_variables - - meson setup build --werror -Dsystem=true || (cat build/meson-logs/meson-log.txt && exit 1) - - DESTDIR=$(pwd)/install meson compile -C build install-web + - source ci/build.sh + - run_website_build after_script: - test "$CI_JOB_STATUS" != "success" && exit 1; - mv install/usr/share/doc/libvirt/html/ website -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:27PM +0200, Erik Skultety wrote:
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 0450c1f6ad..0ed6504dc6 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -114,10 +114,8 @@ website_local_env: .codestyle_job: stage: sanity_checks script: - - *script_variables - - meson setup build --werror || (cat build/meson-logs/meson-log.txt && exit 1) - - meson compile -C build libvirt-pot-dep - - meson test -C build --suite syntax-check --no-rebuild --print-errorlogs + - source ci/build.sh + - run_codestyle codestyle_prebuilt_env: extends: -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:28PM +0200, Erik Skultety wrote:
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 0ed6504dc6..3efde7098a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -156,9 +156,8 @@ potfile: before_script: - *script_variables script: - - meson setup build --werror || (cat build/meson-logs/meson-log.txt && exit 1) - - meson compile -C build libvirt-pot-dep - - meson compile -C build libvirt-pot + - source ci/build.sh + - run_potfile after_script: - test "$CI_JOB_STATUS" != "success" && exit 1; - cp po/libvirt.pot libvirt.pot -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:29PM +0200, Erik Skultety wrote:
Individual shell command executions are replaced by respective functions in the ci/build.sh base script. This will make sure we use the same recipes in GitLab jobs as well as in local executions.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitlab-ci.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
This method unused anywhere, so drop it. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 4 ---- 1 file changed, 4 deletions(-) diff --git a/ci/helper b/ci/helper index fb562d55e1..8986772153 100755 --- a/ci/helper +++ b/ci/helper @@ -163,10 +163,6 @@ class Application: output = subprocess.check_output([self._args.lcitool] + args) return output.decode("utf-8") - def _lcitool_get_targets(self): - output = self._lcitool_run(["targets"]) - return output.splitlines() - def _check_stale_images(self): namespace = self._args.namespace gitlab_uri = self._args.gitlab_uri -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:30PM +0200, Erik Skultety wrote:
This method unused anywhere, so drop it.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 4 ---- 1 file changed, 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
':' is just a connecting character, we can add it to the appropriate place later in the Python script later, but it doesn't make sense to be part of the image 'tag' string. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/helper b/ci/helper index 8986772153..d3de6d96cb 100755 --- a/ci/helper +++ b/ci/helper @@ -40,7 +40,7 @@ class Parser: ) containerparser.add_argument( "--image-tag", - default=":latest", + default="latest", help="use container images with non-default tags", ) -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:31PM +0200, Erik Skultety wrote:
':' is just a connecting character, we can add it to the appropriate place later in the Python script later, but it doesn't make sense to be part of the image 'tag' string.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
We'll soon be relying solely on lcitool so we need to be able to run it from a user-provided location if it's not installed in a known location. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ci/helper b/ci/helper index d3de6d96cb..75552774f6 100755 --- a/ci/helper +++ b/ci/helper @@ -43,6 +43,12 @@ class Parser: default="latest", help="use container images with non-default tags", ) + containerparser.add_argument( + "--lcitool-path", + dest="lcitool", + default="lcitool", + help="path to lcitool (default: $PATH)", + ) # Options that are common to all actions that call the # project's build system -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:32PM +0200, Erik Skultety wrote:
We'll soon be relying solely on lcitool so we need to be able to run it from a user-provided location if it's not installed in a known location.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 6 ++++++ 1 file changed, 6 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
The idea behind this subcommand is to follow whatever build job we have defined in the GitLab CI pipeline, so that we only have a single source of truth for the recipes. Adds 'shell' as an extra option for interactive container build debugging. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/ci/helper b/ci/helper index 75552774f6..60fdaed5f4 100755 --- a/ci/helper +++ b/ci/helper @@ -114,6 +114,21 @@ class Parser: ) shellparser.set_defaults(func=Application._action_shell) + jobparser = subparsers.add_parser( + "run", + help="Run a GitLab CI job or 'shell' in a local environment", + parents=[containerparser], + formatter_class=argparse.ArgumentDefaultsHelpFormatter, + ) + jobparser.add_argument( + "--job", + choices=["build", "codestyle", "potfile", "rpmbuild", + "shell", "test", "website"], + default="build", + help="Run a GitLab CI job or 'shell' in a local environment", + ) + jobparser.set_defaults(func=Application._action_run) + # list-images action listimagesparser = subparsers.add_parser( "list-images", -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:33PM +0200, Erik Skultety wrote:
The idea behind this subcommand is to follow whatever build job we have defined in the GitLab CI pipeline, so that we only have a single source of truth for the recipes. Adds 'shell' as an extra option for interactive container build debugging.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Since we'll depend on GitPython for repo cloning, we need to make sure to emit a user friendly error if the module is not installed. This patch introduces a helper which future patches will use as a decorator. Inspiration for this helper has been taken out of lcitool where we use an identical helper for this purpose. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/ci/helper b/ci/helper index 60fdaed5f4..780f4161f5 100755 --- a/ci/helper +++ b/ci/helper @@ -14,6 +14,28 @@ import textwrap import util +def required_deps(*deps): + module2pkg = { + "git": "GitPython" + } + + def inner_decorator(func): + def wrapped(*args, **kwargs): + cmd = func.__name__[len('_action_'):] + for dep in deps: + try: + import importlib + importlib.import_module(dep) + except ImportError: + pkg = module2pkg[dep] + msg = f"'{pkg}' not found (required by the '{cmd}' command)" + print(msg, file=sys.stderr) + sys.exit(1) + func(*args, **kwargs) + return wrapped + return inner_decorator + + class Parser: def __init__(self): # Options that are common to all actions that use containers -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:34PM +0200, Erik Skultety wrote:
Since we'll depend on GitPython for repo cloning, we need to make sure to emit a user friendly error if the module is not installed. This patch introduces a helper which future patches will use as a decorator. Inspiration for this helper has been taken out of lcitool where we use an identical helper for this purpose.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
This helper will be utilized by a future patch which will add the lcitool container execution logic. The reason why the required_deps decorator isn't being used here is because this is a property. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ci/helper b/ci/helper index 780f4161f5..2a51ce1786 100755 --- a/ci/helper +++ b/ci/helper @@ -174,9 +174,18 @@ class Parser: class Application: + @property + def repo(self): + if self._repo is None: + from git import Repo + + self._repo = Repo(search_parent_directories=True) + return self._repo + def __init__(self): self._basedir = pathlib.Path(__file__).resolve().parent self._args = Parser().parse() + self._repo = None def _make_run(self, target): args = [ -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:35PM +0200, Erik Skultety wrote:
This helper will be utilized by a future patch which will add the lcitool container execution logic. The reason why the required_deps decorator isn't being used here is because this is a property.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 9 +++++++++ 1 file changed, 9 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
A proper Python equivalent of 'git clone --local'. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ci/helper b/ci/helper index 2a51ce1786..d704d1f711 100755 --- a/ci/helper +++ b/ci/helper @@ -211,6 +211,10 @@ class Application: if pty.spawn(["make"] + args) != 0: sys.exit("error: 'make' failed") + @staticmethod + def _prepare_repo_copy(repo, dest): + return repo.clone(dest, local=True) + def _lcitool_run(self, args): output = subprocess.check_output([self._args.lcitool] + args) return output.decode("utf-8") -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:36PM +0200, Erik Skultety wrote:
A proper Python equivalent of 'git clone --local'.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
This method wasn't even utilized before this patch. This patch adds all the necessary logic to successfully execute a container workload via lcitool (which will later allow us to ditch ci/Makefile). Because container executions via lcitool creates the following inside the container: $ ls script datadir where 'datadir' is the workload directory (in this case a local git repo clone) and 'script' is the code that runs whatever the workload is over 'datadir'. In order to satisfy the ^above, our helper generates a trivial temporary 'script' that will source ci/build.sh and run whatever was specified as --job essentially to simulate the exact steps a GitLab pipeline job would go through. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/ci/helper b/ci/helper index d704d1f711..887e64ece1 100755 --- a/ci/helper +++ b/ci/helper @@ -11,6 +11,9 @@ import subprocess import sys import textwrap +from pathlib import Path +from tempfile import TemporaryDirectory + import util @@ -216,8 +219,51 @@ class Application: return repo.clone(dest, local=True) def _lcitool_run(self, args): - output = subprocess.check_output([self._args.lcitool] + args) - return output.decode("utf-8") + positional_args = ["container"] + opts = ["--user", self._args.login] + tmpdir = TemporaryDirectory(prefix="scratch", + dir=Path(self.repo.working_dir, "ci")) + + repo_dest_path = Path(tmpdir.name, "libvirt.git").as_posix() + repo_clone = self._prepare_repo_copy(self.repo, repo_dest_path) + opts.extend(["--workload-dir", repo_clone.working_dir]) + + if self._args.job == "shell": + positional_args.append("shell") + else: + job2func = { + "test": "run_test", + "build": "run_build", + "codestyle": "run_codestyle", + "potfile": "run_potfile", + "rpmbuild": "run_rpmbuild", + "website": "run_website_build", + } + + if self._args.engine != "auto": + positional_args.extend(["--engine", self._args.engine]) + + with open(Path(tmpdir.name, "script"), "w") as f: + script_path = f.name + contents = textwrap.dedent(f"""\ + #!/bin/sh + + cd datadir + . ci/build.sh + + {job2func[self._args.job]} + """) + + f.write(contents) + + positional_args.append("run") + opts.extend(["--script", script_path]) + + opts.append(f"{self._args.image_prefix}{self._args.target}:{self._args.image_tag}") + subprocess.run([self._args.lcitool] + positional_args + opts) + + # this will take care of the generated script file above as well + tmpdir.cleanup() def _check_stale_images(self): namespace = self._args.namespace -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:37PM +0200, Erik Skultety wrote:
This method wasn't even utilized before this patch. This patch adds all the necessary logic to successfully execute a container workload via lcitool (which will later allow us to ditch ci/Makefile). Because container executions via lcitool creates the following inside the container:
$ ls script datadir
where 'datadir' is the workload directory (in this case a local git repo clone) and 'script' is the code that runs whatever the workload is over 'datadir'.
In order to satisfy the ^above, our helper generates a trivial temporary 'script' that will source ci/build.sh and run whatever was specified as --job essentially to simulate the exact steps a GitLab pipeline job would go through.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Just like with the other CLI sub-commands, add an action to run a GitLab spec job in a local container via lcitool. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ci/helper b/ci/helper index 887e64ece1..7385d3b95c 100755 --- a/ci/helper +++ b/ci/helper @@ -305,6 +305,10 @@ class Application: def _action_shell(self): self._make_run(f"ci-shell@{self._args.target}") + @required_deps("git") + def _action_run(self): + self._lcitool_run(self._args.job) + def _action_list_images(self): registry_uri = util.get_registry_uri(self._args.namespace, self._args.gitlab_uri) -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:38PM +0200, Erik Skultety wrote:
Just like with the other CLI sub-commands, add an action to run a GitLab spec job in a local container via lcitool.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Previous patches added a single 'run' command parametrized with GitLab job specs via '--job' that cover all of these original actions, adding some more in the process. Drop the original actions as we don't need them anymore. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 36 ------------------------------------ 1 file changed, 36 deletions(-) diff --git a/ci/helper b/ci/helper index 7385d3b95c..07e449e59f 100755 --- a/ci/helper +++ b/ci/helper @@ -112,33 +112,6 @@ class Parser: ) subparsers.required = True - # build action - buildparser = subparsers.add_parser( - "build", - help="run a build in a container", - parents=[containerparser, mesonparser], - formatter_class=argparse.ArgumentDefaultsHelpFormatter, - ) - buildparser.set_defaults(func=Application._action_build) - - # test action - testparser = subparsers.add_parser( - "test", - help="run a build in a container (including tests)", - parents=[containerparser, mesonparser], - formatter_class=argparse.ArgumentDefaultsHelpFormatter, - ) - testparser.set_defaults(func=Application._action_test) - - # shell action - shellparser = subparsers.add_parser( - "shell", - help="start a shell in a container", - parents=[containerparser], - formatter_class=argparse.ArgumentDefaultsHelpFormatter, - ) - shellparser.set_defaults(func=Application._action_shell) - jobparser = subparsers.add_parser( "run", help="Run a GitLab CI job or 'shell' in a local environment", @@ -296,15 +269,6 @@ class Application: """) print(msg.replace("STALE_DETAILS", stale_details)) - def _action_build(self): - self._make_run(f"ci-build@{self._args.target}") - - def _action_test(self): - self._make_run(f"ci-test@{self._args.target}") - - def _action_shell(self): - self._make_run(f"ci-shell@{self._args.target}") - @required_deps("git") def _action_run(self): self._lcitool_run(self._args.job) -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:39PM +0200, Erik Skultety wrote:
Previous patches added a single 'run' command parametrized with GitLab job specs via '--job' that cover all of these original actions, adding some more in the process. Drop the original actions as we don't need them anymore.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 36 ------------------------------------ 1 file changed, 36 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
These originally allowed customizing the ci/Makefile script which was the core of the local container executions. The problem was that however flexible this may have been, it never mirrored what was being done as part of the GitLab jobs. Motivated by the effort of mirroring GitLab jobs locally, these would only ever make sense to be set/used in interactive shell container sessions where the developer is perfectly capable of using the right meson/ninja CLI options directly without going through another shell variable indirection as it was the case with these ci/helper options. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/ci/helper b/ci/helper index 07e449e59f..0053fce40e 100755 --- a/ci/helper +++ b/ci/helper @@ -75,21 +75,6 @@ class Parser: help="path to lcitool (default: $PATH)", ) - # Options that are common to all actions that call the - # project's build system - mesonparser = argparse.ArgumentParser(add_help=False) - mesonparser.add_argument( - "--meson-args", - default="", - help="additional arguments passed to meson " - "(eg --meson-args='-Dopt1=enabled -Dopt2=disabled')", - ) - mesonparser.add_argument( - "--ninja-args", - default="", - help="additional arguments passed to ninja", - ) - # Options that are common to actions communicating with a GitLab # instance gitlabparser = argparse.ArgumentParser(add_help=False) -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:40PM +0200, Erik Skultety wrote:
These originally allowed customizing the ci/Makefile script which was the core of the local container executions. The problem was that however flexible this may have been, it never mirrored what was being done as part of the GitLab jobs. Motivated by the effort of mirroring GitLab jobs locally, these would only ever make sense to be set/used in interactive shell container sessions where the developer is perfectly capable of using the right meson/ninja CLI options directly without going through another shell variable indirection as it was the case with these ci/helper options.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 15 --------------- 1 file changed, 15 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
We've successfully migrated over to lcitool to take care of the container workload execution, so dropping this 'make' prep code is a prerequisite of finally getting rid of the ci/Makefile script. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/ci/helper b/ci/helper index 0053fce40e..e45eb63016 100755 --- a/ci/helper +++ b/ci/helper @@ -6,7 +6,6 @@ import argparse import os import pathlib -import pty import subprocess import sys import textwrap @@ -148,30 +147,6 @@ class Application: self._args = Parser().parse() self._repo = None - def _make_run(self, target): - args = [ - "-C", - self._basedir, - target, - ] - - if self._args.action in ["build", "test", "shell"]: - args.extend([ - f"CI_ENGINE={self._args.engine}", - f"CI_USER_LOGIN={self._args.login}", - f"CI_IMAGE_PREFIX={self._args.image_prefix}", - f"CI_IMAGE_TAG={self._args.image_tag}", - ]) - - if self._args.action in ["build", "test"]: - args.extend([ - f"MESON_ARGS={self._args.meson_args}", - f"NINJA_ARGS={self._args.ninja_args}", - ]) - - if pty.spawn(["make"] + args) != 0: - sys.exit("error: 'make' failed") - @staticmethod def _prepare_repo_copy(repo, dest): return repo.clone(dest, local=True) -- 2.41.0
On Fri, Aug 25, 2023 at 07:55:41PM +0200, Erik Skultety wrote:
We've successfully migrated over to lcitool to take care of the container workload execution, so dropping this 'make' prep code is a prerequisite of finally getting rid of the ci/Makefile script.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 25 ------------------------- 1 file changed, 25 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 Fri, Aug 25, 2023 at 07:55:08PM +0200, Erik Skultety wrote:
Technically a v2 of: https://listman.redhat.com/archives/libvir-list/2023-February/237552.html
However, the approach here is slightly different and what that series said about migration to lcitool container executions as a replacement for ci/Makefile is actually done here. One of the core problems of the above pointed out in review was that more Shell logic was introduced including CLI parsing, conditional executions, etc. which we fought hard to get rid of in the past. I reworked the Shell functions quite a bit and dropped whatever extra Shell logic the original series added. Obviously we can't get rid of Shell completely because of .gitlab-ci.yml and so I merely extracted the recipes into functions which are then sourced as ci/build.sh and executed. Now, that on its own would hide the actual commands being run in the GitLab job log, so before any command is actually executed, it is formatted with a color sequence so we don't miss that information as that would be a regression to the status quo.
Lastly, this series then takes the effort inside the ci/build.sh script and basically mirrors whatever GitLab would do to run a job inside a local container which is executed by lcitool (yes, we already have that capability).
Please give this a try and I'm already looking forward to comments as I'd like to expand this effort to local VM executions running the TCK integration tests, so this series is quite important in that regard.
Do you have a gitlab branch with this contnt somewhere. When i tried to apply the patches to current git, it was unhappy on the 3rd patch $ git am -3 ~/cibuild Applying: ci: build.sh: Add variables from .gitlab-ci.yml Applying: ci: build.sh: Add GIT_ROOT env helper variable Applying: ci: build.sh: Don't mention that MESON_ARGS are available via CLI error: sha1 information is lacking or useless (ci/build.sh). error: could not build fake ancestor Patch failed at 0003 ci: build.sh: Don't mention that MESON_ARGS are available via CLI hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". 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 Thu, Aug 31, 2023 at 05:17:13PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:08PM +0200, Erik Skultety wrote:
Technically a v2 of: https://listman.redhat.com/archives/libvir-list/2023-February/237552.html
However, the approach here is slightly different and what that series said about migration to lcitool container executions as a replacement for ci/Makefile is actually done here. One of the core problems of the above pointed out in review was that more Shell logic was introduced including CLI parsing, conditional executions, etc. which we fought hard to get rid of in the past. I reworked the Shell functions quite a bit and dropped whatever extra Shell logic the original series added. Obviously we can't get rid of Shell completely because of .gitlab-ci.yml and so I merely extracted the recipes into functions which are then sourced as ci/build.sh and executed. Now, that on its own would hide the actual commands being run in the GitLab job log, so before any command is actually executed, it is formatted with a color sequence so we don't miss that information as that would be a regression to the status quo.
Lastly, this series then takes the effort inside the ci/build.sh script and basically mirrors whatever GitLab would do to run a job inside a local container which is executed by lcitool (yes, we already have that capability).
Please give this a try and I'm already looking forward to comments as I'd like to expand this effort to local VM executions running the TCK integration tests, so this series is quite important in that regard.
Do you have a gitlab branch with this contnt somewhere. When i tried to apply the patches to current git, it was unhappy on the 3rd patch
$ git am -3 ~/cibuild Applying: ci: build.sh: Add variables from .gitlab-ci.yml Applying: ci: build.sh: Add GIT_ROOT env helper variable Applying: ci: build.sh: Don't mention that MESON_ARGS are available via CLI error: sha1 information is lacking or useless (ci/build.sh). error: could not build fake ancestor Patch failed at 0003 ci: build.sh: Don't mention that MESON_ARGS are available via CLI hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Yeah, I had a different merge conflict locally, but I guess both related to b68d253c4603a041cb993fa133ba25e610a78929 not having been pushed yet and the time I posted the series. Here's the fixed branch: https://gitlab.com/eskultety/libvirt/-/commits/helper-lcitool And here's a pipeline (with the fix posted for patch 11): https://gitlab.com/eskultety/libvirt/-/pipelines/988871113 Erik
On Fri, Sep 01, 2023 at 11:21:04AM +0200, Erik Skultety wrote:
On Thu, Aug 31, 2023 at 05:17:13PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:08PM +0200, Erik Skultety wrote:
Technically a v2 of: https://listman.redhat.com/archives/libvir-list/2023-February/237552.html
However, the approach here is slightly different and what that series said about migration to lcitool container executions as a replacement for ci/Makefile is actually done here. One of the core problems of the above pointed out in review was that more Shell logic was introduced including CLI parsing, conditional executions, etc. which we fought hard to get rid of in the past. I reworked the Shell functions quite a bit and dropped whatever extra Shell logic the original series added. Obviously we can't get rid of Shell completely because of .gitlab-ci.yml and so I merely extracted the recipes into functions which are then sourced as ci/build.sh and executed. Now, that on its own would hide the actual commands being run in the GitLab job log, so before any command is actually executed, it is formatted with a color sequence so we don't miss that information as that would be a regression to the status quo.
Lastly, this series then takes the effort inside the ci/build.sh script and basically mirrors whatever GitLab would do to run a job inside a local container which is executed by lcitool (yes, we already have that capability).
Please give this a try and I'm already looking forward to comments as I'd like to expand this effort to local VM executions running the TCK integration tests, so this series is quite important in that regard.
Do you have a gitlab branch with this contnt somewhere. When i tried to apply the patches to current git, it was unhappy on the 3rd patch
$ git am -3 ~/cibuild Applying: ci: build.sh: Add variables from .gitlab-ci.yml Applying: ci: build.sh: Add GIT_ROOT env helper variable Applying: ci: build.sh: Don't mention that MESON_ARGS are available via CLI error: sha1 information is lacking or useless (ci/build.sh). error: could not build fake ancestor Patch failed at 0003 ci: build.sh: Don't mention that MESON_ARGS are available via CLI hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Yeah, I had a different merge conflict locally, but I guess both related to b68d253c4603a041cb993fa133ba25e610a78929 not having been pushed yet and the time I posted the series.
Yeah, I eventually figured out b68d253c4603a041cb993fa133ba25e610a78929 was the problem and applied your series before that commit to test it. 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 Fri, Aug 25, 2023 at 07:55:08PM +0200, Erik Skultety wrote:
Technically a v2 of: https://listman.redhat.com/archives/libvir-list/2023-February/237552.html
However, the approach here is slightly different and what that series said about migration to lcitool container executions as a replacement for ci/Makefile is actually done here. One of the core problems of the above pointed out in review was that more Shell logic was introduced including CLI parsing, conditional executions, etc. which we fought hard to get rid of in the past. I reworked the Shell functions quite a bit and dropped whatever extra Shell logic the original series added. Obviously we can't get rid of Shell completely because of .gitlab-ci.yml and so I merely extracted the recipes into functions which are then sourced as ci/build.sh and executed. Now, that on its own would hide the actual commands being run in the GitLab job log, so before any command is actually executed, it is formatted with a color sequence so we don't miss that information as that would be a regression to the status quo.
Lastly, this series then takes the effort inside the ci/build.sh script and basically mirrors whatever GitLab would do to run a job inside a local container which is executed by lcitool (yes, we already have that capability).
Please give this a try and I'm already looking forward to comments as I'd like to expand this effort to local VM executions running the TCK integration tests, so this series is quite important in that regard.
Overall I'm fine with what's proposed here. Two general thoughts * ci/Makefile appears pretty much redundant - ci/helper can provide the same level of functionality AFAICT, and it'd be nice to kill an outstanding usage of 'make' given our goal to standardize on meson + python * ci/helper looks almost entirely independent of libvirt, aside from the list of 'choices' for the --job arg, and the --namespace arg default value, it would work with any virt project we have if the project created its own ci/build.sh file Can we fold all its logic into lcitool, and just have that as the entrypoint ? In ci/manifest.yml we can get the project namespace, and we could possibly just extra the commands by crudely regex matching 'ci/build.sh' content against the pattern '^run_.*\(\)$ {' The removal of ci/Makefil feels like it could be done in this series, but its fine if the ci/helper suggestion is left as separate future work. 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 Thu, Aug 31, 2023 at 06:28:16PM +0100, Daniel P. Berrangé wrote:
On Fri, Aug 25, 2023 at 07:55:08PM +0200, Erik Skultety wrote:
Technically a v2 of: https://listman.redhat.com/archives/libvir-list/2023-February/237552.html
However, the approach here is slightly different and what that series said about migration to lcitool container executions as a replacement for ci/Makefile is actually done here. One of the core problems of the above pointed out in review was that more Shell logic was introduced including CLI parsing, conditional executions, etc. which we fought hard to get rid of in the past. I reworked the Shell functions quite a bit and dropped whatever extra Shell logic the original series added. Obviously we can't get rid of Shell completely because of .gitlab-ci.yml and so I merely extracted the recipes into functions which are then sourced as ci/build.sh and executed. Now, that on its own would hide the actual commands being run in the GitLab job log, so before any command is actually executed, it is formatted with a color sequence so we don't miss that information as that would be a regression to the status quo.
Lastly, this series then takes the effort inside the ci/build.sh script and basically mirrors whatever GitLab would do to run a job inside a local container which is executed by lcitool (yes, we already have that capability).
Please give this a try and I'm already looking forward to comments as I'd like to expand this effort to local VM executions running the TCK integration tests, so this series is quite important in that regard.
Overall I'm fine with what's proposed here.
Two general thoughts
* ci/Makefile appears pretty much redundant - ci/helper can provide the same level of functionality AFAICT, and it'd be nice to kill an outstanding usage of 'make' given our goal to standardize on meson + python
Huh, the fact that removal of Makefile isn't part of this series is a mistake on my side - I worked on this on 2 parallel branches trying out 2 slightly different approaches. I did drop it on one branch but not the other which I ultimately decided to go with. Since I'll be sending a v2, I'll add that patch.
* ci/helper looks almost entirely independent of libvirt, aside from the list of 'choices' for the --job arg, and the --namespace arg default value, it would work with any virt project we have if the project created its own ci/build.sh file
Can we fold all its logic into lcitool, and just have that as the entrypoint ? In ci/manifest.yml we can get the project namespace, and we could possibly just extra the commands by crudely regex matching 'ci/build.sh' content against the pattern '^run_.*\(\)$ {'
Technically we could. Extracting the code and injecting it into lcitool is not a problem, in fact, it would be quite straight forward. The problem is designing a CLI interface that would make sense for the use case without breaking the existing one too much. Ideally by introducing just a bunch of optional args which I don't think is very realistic. Since that will require thorough thinking and designing I did not want to dive right into that as I wasn't even sure whether I'd be able to push this conversion through upstream.
The removal of ci/Makefil feels like it could be done in this series, but its fine if the ci/helper suggestion is left as separate future work.
Yeah, like I said above, incorporating ci/helper into lcitool is likely going to be again one of the bigger overhauls so that will be a project on its own. Thanks for the comments, much appreciated. Erik
participants (2)
-
Daniel P. Berrangé -
Erik Skultety