Apparently we've only had it because the -[ao] options weren't portable at the time, but according to https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html both are defined in POSIX.1-2017 revision which is old enough for all our supported platforms to have adopted it already. Therefore, drop the rule. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- build-aux/syntax-check.mk | 9 --------- 1 file changed, 9 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index d7cf109fbd..5718768193 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1201,15 +1201,6 @@ sc_prohibit_double_semicolon: halt="Double semicolon detected" \ $(_sc_search_regexp) -_ptm1 = use "test C1 && test C2", not "test C1 -''a C2" -_ptm2 = use "test C1 || test C2", not "test C1 -''o C2" -# Using test's -a and -o operators is not portable. -# We prefer test over [, since the latter is spelled [[ in configure.ac. -sc_prohibit_test_minus_ao: - @prohibit='(\<test| \[+) .+ -[ao] ' \ - halt='$(_ptm1); $(_ptm2)' \ - $(_sc_search_regexp) - # Avoid a test bashism. sc_prohibit_test_double_equal: @prohibit='(\<test| \[+) .+ == ' \ -- 2.41.0

Follow what's been done to other jobs in .gitlab-ci.yml and extract the shell logic from YAML to a function in ci/jobs.sh Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/integration-template.yml | 36 ++---------------------------------- ci/jobs.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/ci/integration-template.yml b/ci/integration-template.yml index 111a115286..e87ffaf9aa 100644 --- a/ci/integration-template.yml +++ b/ci/integration-template.yml @@ -27,33 +27,6 @@ .install-deps: &install-deps - sudo dnf install -y libvirt-rpms/* libvirt-perl-rpms/* libvirt-python-rpms/* - # Avocado >98.0 fails with the nwfilter TCK tests, so stick with 98.0 for now - - sudo pip3 install --prefix=/usr avocado-framework - - -.enable-core-dumps: &enable-core-dumps - - sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explicitly allow storing cores globally - - sudo systemctl daemon-reexec # need to reexec systemd after changing config - - -.enable-libvirt-debugging: &enable-libvirt-debugging - - source /etc/os-release # in order to query the vendor-provided variables - - if test "$ID" = "centos" && test "$VERSION_ID" -lt 9; - then - DAEMONS="libvirtd virtlockd virtlogd"; - else - DAEMONS="virtinterfaced virtlockd virtlogd virtnetworkd virtnodedevd virtnwfilterd virtproxyd virtqemud virtsecretd virtstoraged"; - fi - - for daemon in $DAEMONS; - do - LOG_OUTPUTS="1:file:/var/log/libvirt/${daemon}.log"; - LOG_FILTERS="3:remote 4:event 3:util.json 3:util.object 3:util.dbus 3:util.netlink 3:node_device 3:rpc 3:access 1:*"; - sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" &>/dev/null; - sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" &>/dev/null; - sudo systemctl --quiet stop ${daemon}.service; - sudo systemctl restart ${daemon}.socket; - done - .collect-logs: &collect-logs - set +e @@ -78,14 +51,9 @@ before_script: - mkdir "$SCRATCH_DIR" - *install-deps - - *enable-core-dumps - - *enable-libvirt-debugging - - sudo virsh net-start default &>/dev/null || true; script: - - cd "$SCRATCH_DIR" - - git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git - - cd libvirt-tck - - sudo avocado --config avocado.config run --job-results-dir "$SCRATCH_DIR"/avocado + - source ci/jobs.sh + - run_integration after_script: - test "$CI_JOB_STATUS" = "success" && exit 0; - *collect-logs diff --git a/ci/jobs.sh b/ci/jobs.sh index eb4a4e29cf..2bc6f86e45 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -80,3 +80,35 @@ run_website_build() { run_build } + +run_integration() { + # Avocado >98.0 fails with the nwfilter TCK tests, so stick with 98.0 for now + sudo pip3 install --prefix=/usr avocado-framework + + sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explicitly allow storing cores globally + sudo systemctl daemon-reexec # need to reexec systemd after changing config + + source /etc/os-release # in order to query the vendor-provided variables + if test "$ID" = "centos" && test "$VERSION_ID" -lt 9 + then + DAEMONS="libvirtd virtlockd virtlogd" + else + DAEMONS="virtinterfaced virtlockd virtlogd virtnetworkd virtnodedevd virtnwfilterd virtproxyd virtqemud virtsecretd virtstoraged" + fi + for daemon in $DAEMONS + do + LOG_OUTPUTS="1:file:/var/log/libvirt/${daemon}.log" + LOG_FILTERS="3:remote 4:event 3:util.json 3:util.object 3:util.dbus 3:util.netlink 3:node_device 3:rpc 3:access 1:*" + sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" &>/dev/null + sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" &>/dev/null + sudo systemctl --quiet stop ${daemon}.service + sudo systemctl restart ${daemon}.socket + done + + sudo virsh net-start default &>/dev/null || true + + cd "$SCRATCH_DIR" + git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git + cd libvirt-tck + sudo avocado --config avocado.config run --job-results-dir "$SCRATCH_DIR"/avocado +} -- 2.41.0

All supported versions of Fedora and CentOS Stream 9 default to modular setup, it's probably better if we cosmetically adjust the CentOS Stream version check to make it explicit that monolithic daemon services ought to be started only on Stream 8. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/jobs.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index 2bc6f86e45..adb798d127 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -89,7 +89,7 @@ run_integration() { sudo systemctl daemon-reexec # need to reexec systemd after changing config source /etc/os-release # in order to query the vendor-provided variables - if test "$ID" = "centos" && test "$VERSION_ID" -lt 9 + if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 then DAEMONS="libvirtd virtlockd virtlogd" else -- 2.41.0

Since the section now only consists of a single command, we can happily move the command to the main integration template job body. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/integration-template.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ci/integration-template.yml b/ci/integration-template.yml index e87ffaf9aa..544264c3d5 100644 --- a/ci/integration-template.yml +++ b/ci/integration-template.yml @@ -24,10 +24,6 @@ - sudo make install -.install-deps: &install-deps - - sudo dnf install -y libvirt-rpms/* libvirt-perl-rpms/* libvirt-python-rpms/* - - .collect-logs: &collect-logs - set +e - shopt -s nullglob @@ -50,7 +46,7 @@ stage: integration_tests before_script: - mkdir "$SCRATCH_DIR" - - *install-deps + - sudo dnf install -y libvirt-rpms/* libvirt-perl-rpms/* libvirt-python-rpms/* script: - source ci/jobs.sh - run_integration -- 2.41.0

We needed v98.0 due to a bug in the past and have been installing the latest Avocado for a while, yet we kept the comment by a mistake. Besides, looks like v98.0 ignores the avocado.config file in the TCK repo instructing it to run the test suite sequentially leading to test stability issues, so abandoning the v98.0 was a good thing in the end. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/jobs.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index adb798d127..04e776e9be 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -82,7 +82,6 @@ run_website_build() { } run_integration() { - # Avocado >98.0 fails with the nwfilter TCK tests, so stick with 98.0 for now sudo pip3 install --prefix=/usr avocado-framework sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explicitly allow storing cores globally -- 2.41.0

We've not been interested in any extra output from the command at all since we always redirected both stdout and stderr to /dev/null. Future patch will change that slightly, so --quiet will start making sense. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/jobs.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index 04e776e9be..75ea638580 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -104,7 +104,7 @@ run_integration() { sudo systemctl restart ${daemon}.socket done - sudo virsh net-start default &>/dev/null || true + sudo virsh --quiet net-start default &>/dev/null || true cd "$SCRATCH_DIR" git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git -- 2.41.0

Because of the nature of writing inline shell commands to YAML, most of the commentaries where inlined with the command not to hinder YAML readability any further. Since we moved the logic to a standalone script, we can now do whatever formatting & readability adjustments we want. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/jobs.sh | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index 75ea638580..37bca452fa 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -84,10 +84,14 @@ run_website_build() { run_integration() { sudo pip3 install --prefix=/usr avocado-framework - sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explicitly allow storing cores globally - sudo systemctl daemon-reexec # need to reexec systemd after changing config + # Explicitly allow storing cores globally + sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" - source /etc/os-release # in order to query the vendor-provided variables + # Need to reexec systemd after changing config + sudo systemctl daemon-reexec + + # Source the os-release file to query the vendor-provided variables + source /etc/os-release if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 then DAEMONS="libvirtd virtlockd virtlogd" @@ -104,6 +108,11 @@ run_integration() { sudo systemctl restart ${daemon}.socket done + # Make sure the default network is started on all platforms + # The reason for the '|| true' here is solely that GitLab executes all + # Shell scripts with -e by default and virsh returns an error if one tries + # to start a machine/network that is already active which is both fine and + # should also be a non-fatal error sudo virsh --quiet net-start default &>/dev/null || true cd "$SCRATCH_DIR" -- 2.41.0

Neither '&>' nor 'source' are defined in POSIX. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/jobs.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index 37bca452fa..f4e83dda2e 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -91,7 +91,7 @@ run_integration() { sudo systemctl daemon-reexec # Source the os-release file to query the vendor-provided variables - source /etc/os-release + . /etc/os-release if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 then DAEMONS="libvirtd virtlockd virtlogd" @@ -102,8 +102,8 @@ run_integration() { do LOG_OUTPUTS="1:file:/var/log/libvirt/${daemon}.log" LOG_FILTERS="3:remote 4:event 3:util.json 3:util.object 3:util.dbus 3:util.netlink 3:node_device 3:rpc 3:access 1:*" - sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" &>/dev/null - sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" &>/dev/null + sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" 2>/dev/null 1>&2 + sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" 2>/dev/null 1>&2 sudo systemctl --quiet stop ${daemon}.service sudo systemctl restart ${daemon}.socket done @@ -113,7 +113,7 @@ run_integration() { # Shell scripts with -e by default and virsh returns an error if one tries # to start a machine/network that is already active which is both fine and # should also be a non-fatal error - sudo virsh --quiet net-start default &>/dev/null || true + sudo virsh --quiet net-start default 2>/dev/null 1>&2 || true cd "$SCRATCH_DIR" git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git -- 2.41.0

We tried to evade usage of eval in commit 6214ae55f6a, but trying to use I/O redirections with a command doesn't have the desired effect, because when Shell eats the redirection it is applied to anything inside the run_cmd function, even the print command we use for debugging purposes. In order to print all commands and use the redirection only on the actual execution of a given command, let's adopt eval on "$@" and allow passing redirections as strings later on. Future patches will demonstrate this. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/jobs.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index f4e83dda2e..3a89cb1a69 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -15,7 +15,7 @@ fi GIT_ROOT="$(git rev-parse --show-toplevel)" run_cmd() { printf "\e[32m[RUN COMMAND]: '%s'\e[0m\n" "$*" - "$@" + eval "$@" } run_meson_setup() { @@ -70,7 +70,7 @@ run_rpmbuild() { run_cmd rpmbuild \ --clean \ --nodeps \ - --define "_without_mingw 1" \ + --define "'_without_mingw 1'" \ -ta build/meson-dist/libvirt-*.tar.xz } -- 2.41.0

On Mon, Sep 18, 2023 at 11:31:53AM +0100, Daniel P. Berrangé wrote: I don't have a problem with ^this in principle, but eval is more flexible (and hence more dangerous) in what we can pass as parameters in the future. Erik

Unfortunately, once we go down the line of running our own scripts as part of GitLab CI jobs rather than open coding Shell in YAML, we lose the benefit of seeing each line the script executes. The downside of the default YAML however is that we have to maintain the same piece of code on 2 places in that case. Let's adopt what we use with other container jobs and prefix each shell command with 'run_cmd' which will dump it in the logs before executing. Flow control expressions and structures are a problem though in this regard, so let's just print some important values for debugging purposes. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/jobs.sh | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index 3a89cb1a69..27add3d105 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -82,16 +82,16 @@ run_website_build() { } run_integration() { - sudo pip3 install --prefix=/usr avocado-framework + run_cmd sudo pip3 install --prefix=/usr avocado-framework # Explicitly allow storing cores globally - sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" + run_cmd sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explicitly allow storing cores globally # Need to reexec systemd after changing config - sudo systemctl daemon-reexec + run_cmd sudo systemctl daemon-reexec # need to reexec systemd after changing config # Source the os-release file to query the vendor-provided variables - . /etc/os-release + run_cmd . /etc/os-release if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 then DAEMONS="libvirtd virtlockd virtlogd" @@ -102,10 +102,10 @@ run_integration() { do LOG_OUTPUTS="1:file:/var/log/libvirt/${daemon}.log" LOG_FILTERS="3:remote 4:event 3:util.json 3:util.object 3:util.dbus 3:util.netlink 3:node_device 3:rpc 3:access 1:*" - sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" 2>/dev/null 1>&2 - sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" 2>/dev/null 1>&2 - sudo systemctl --quiet stop ${daemon}.service - sudo systemctl restart ${daemon}.socket + run_cmd sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" "2>/dev/null 1>&2" + run_cmd sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" "2>/dev/null 1>&2" + run_cmd sudo systemctl --quiet stop ${daemon}.service + run_cmd sudo systemctl restart ${daemon}.socket done # Make sure the default network is started on all platforms @@ -113,10 +113,10 @@ run_integration() { # Shell scripts with -e by default and virsh returns an error if one tries # to start a machine/network that is already active which is both fine and # should also be a non-fatal error - sudo virsh --quiet net-start default 2>/dev/null 1>&2 || true + run_cmd sudo virsh --quiet net-start default "2>/dev/null 1>&2" || true - cd "$SCRATCH_DIR" - git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git - cd libvirt-tck - sudo avocado --config avocado.config run --job-results-dir "$SCRATCH_DIR"/avocado + run_cmd cd "$SCRATCH_DIR" + run_cmd git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git + run_cmd cd libvirt-tck + run_cmd sudo avocado --config avocado.config run --job-results-dir "$SCRATCH_DIR"/avocado } -- 2.41.0

One advantage that GitLab's YAML has with Shell commands is that every single line is printed out as is, including control structures. In order to see whether the logic did the same thing and the tests are going to operate on the right set of daemons (monolithic vs modular), lets print the DAEMONS variable that we set depending on the distro we execute the tests on. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/jobs.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/jobs.sh b/ci/jobs.sh index 27add3d105..e10769527a 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -98,6 +98,8 @@ run_integration() { else DAEMONS="virtinterfaced virtlockd virtlogd virtnetworkd virtnodedevd virtnwfilterd virtproxyd virtqemud virtsecretd virtstoraged" fi + + echo "DAEMONS=$DAEMONS" for daemon in $DAEMONS do LOG_OUTPUTS="1:file:/var/log/libvirt/${daemon}.log" -- 2.41.0

Running outside of GitLab will likely not have the variable set and hence the execution would fail. To make sure we always start with a clean scratch dir (which may or may not be the best thing), create it with 'mktemp'. The main reason for a temporary directory is to ensure a clean environment for the job every time run_integration function is run. For repeated interactive use case, it is imperative that the developer takes care of their environment. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- ci/jobs.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ci/jobs.sh b/ci/jobs.sh index e10769527a..6e3d681af1 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -117,6 +117,13 @@ run_integration() { # should also be a non-fatal error run_cmd sudo virsh --quiet net-start default "2>/dev/null 1>&2" || true + # SCRATCH_DIR is normally set inside the GitLab CI job to /tmp/scratch. + # However, for local executions inside a VM we need to make sure some + # scratch directory exists and also that it is created outside of /tmp for + # storage space reasons (if multiple project repos are to be cloned). + SCRATCH_DIR="${SCRATCH_DIR:=$GIT_ROOT/ci/scratch)}" + + test ! -d "$SCRATCH_DIR" && run_cmd mkdir "$SCRATCH_DIR" run_cmd cd "$SCRATCH_DIR" run_cmd git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git run_cmd cd libvirt-tck -- 2.41.0