# [PATCH 0/4] Random improvements to GitLab CI 20 messages from 2024-12-06 to 2024-12-12. Participants: Patrick Steinhardt, karthik nayak, Oswald Buddenhagen, Toon Claes. Thread: https://gitlist.dev/t/62609 ## Patrick Steinhardt, 2024-12-06 11:10 Subject: [PATCH 0/4] Random improvements to GitLab CI Message-ID: <20241206-pks-ci-section-fixes-v1-0-7ab1b69e3648@pks.im> URL: https://gitlist.dev/e/20241206-pks-ci-section-fixes-v1-0-7ab1b69e3648%40pks.im ``` Hi, this small patch series includes a couple of more-or-less random improvements to GitLab CI. Thanks! Patrick --- Patrick Steinhardt (4): gitlab-ci: update macOS images to Sonoma ci/lib: remove duplicate trap to end "CI setup" group ci/lib: use echo instead of printf to set up sections ci/lib: fix "CI setup" sections with GitLab CI .gitlab-ci.yml | 4 ++-- ci/lib.sh | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) --- base-commit: e66fd72e972df760a53c3d6da023c17adfc426d6 change-id: 20241206-pks-ci-section-fixes-1bb91ceb50b8 ``` ## Patrick Steinhardt, 2024-12-06 11:10 Subject: [PATCH 2/4] ci/lib: remove duplicate trap to end "CI setup" group Message-ID: <20241206-pks-ci-section-fixes-v1-2-7ab1b69e3648@pks.im> URL: https://gitlist.dev/e/20241206-pks-ci-section-fixes-v1-2-7ab1b69e3648%40pks.im In-Reply-To: <20241206-pks-ci-section-fixes-v1-0-7ab1b69e3648@pks.im> ``` We exlicitly trap on EXIT in order to end the "CI setup" group. This isn't necessary though given that `begin_group ()` already sets up the trap for us. Remove the duplicate trap. Signed-off-by: Patrick Steinhardt --- ci/lib.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/ci/lib.sh b/ci/lib.sh index 930f98d7228166c37c236beb062b14675fb68ef3..a54601be923bf475ba1a9cafd98bb1cb71a10255 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -56,7 +56,6 @@ group () { } begin_group "CI setup" -trap "end_group 'CI setup'" EXIT # Set 'exit on error' for all CI scripts to let the caller know that # something went wrong. -- 2.47.0.366.g5daf58cba8.dirty ``` ## Patrick Steinhardt, 2024-12-06 11:10 Subject: [PATCH 1/4] gitlab-ci: update macOS images to Sonoma Message-ID: <20241206-pks-ci-section-fixes-v1-1-7ab1b69e3648@pks.im> URL: https://gitlist.dev/e/20241206-pks-ci-section-fixes-v1-1-7ab1b69e3648%40pks.im In-Reply-To: <20241206-pks-ci-section-fixes-v1-0-7ab1b69e3648@pks.im> ``` The macOS Ventura images we use for GitLab CI runners have been deprecated. Update them to macOS 14, aka Sonoma. Signed-off-by: Patrick Steinhardt --- .gitlab-ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 526ecfe030a43e0a5a83ddd35cb7c96d46ab2485..61c56ccac8fdc940075d91dd4cb0b54ee33d5199 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -99,10 +99,10 @@ test:osx: parallel: matrix: - jobname: osx-clang - image: macos-13-xcode-14 + image: macos-14-xcode-15 CC: clang - jobname: osx-reftable - image: macos-13-xcode-14 + image: macos-14-xcode-15 CC: clang artifacts: paths: -- 2.47.0.366.g5daf58cba8.dirty ``` ## Patrick Steinhardt, 2024-12-06 11:10 Subject: [PATCH 3/4] ci/lib: use echo instead of printf to set up sections Message-ID: <20241206-pks-ci-section-fixes-v1-3-7ab1b69e3648@pks.im> URL: https://gitlist.dev/e/20241206-pks-ci-section-fixes-v1-3-7ab1b69e3648%40pks.im In-Reply-To: <20241206-pks-ci-section-fixes-v1-0-7ab1b69e3648@pks.im> ``` We use printf to set up sections with GitLab CI even though we could trivially use echo. This may cause problems in case the argument passed to `begin_group ()` or `end_group ()` contains formatting directives as we use them as part of the formatting string. Simplify the code to instead use echo. Signed-off-by: Patrick Steinhardt --- ci/lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/lib.sh b/ci/lib.sh index a54601be923bf475ba1a9cafd98bb1cb71a10255..ba8f4da39caf29db5edaffde160bc81a7c58c329 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -18,7 +18,7 @@ elif test true = "$GITLAB_CI" then begin_group () { need_to_end_group=t - printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n" + echo "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1" trap "end_group '$1'" EXIT set -x } @@ -27,7 +27,7 @@ then test -n "$need_to_end_group" || return 0 set +x need_to_end_group= - printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n" + echo "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K" trap - EXIT } else -- 2.47.0.366.g5daf58cba8.dirty ``` ## Patrick Steinhardt, 2024-12-06 11:10 Subject: [PATCH 4/4] ci/lib: fix "CI setup" sections with GitLab CI Message-ID: <20241206-pks-ci-section-fixes-v1-4-7ab1b69e3648@pks.im> URL: https://gitlist.dev/e/20241206-pks-ci-section-fixes-v1-4-7ab1b69e3648%40pks.im In-Reply-To: <20241206-pks-ci-section-fixes-v1-0-7ab1b69e3648@pks.im> ``` Whenever we source "ci/lib.sh" we wrap the directives in a separate group so that they can easily be collapsed in the web UI. And as we source the script multiple times during a single CI run we thus end up with the same section name reused multiple times, as well. This is broken on GitLab CI though, where reusing the same group name is not supported. The consequence is that only the last of these sections can be collapsed. Fix this issue by including the name of the sourcing script in the group's name. Signed-off-by: Patrick Steinhardt --- ci/lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/lib.sh b/ci/lib.sh index ba8f4da39caf29db5edaffde160bc81a7c58c329..2cdc99e7fd05650ef80715b621b42d15d6b13a12 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -55,7 +55,7 @@ group () { return $res } -begin_group "CI setup" +begin_group "CI setup via $(basename $0)" # Set 'exit on error' for all CI scripts to let the caller know that # something went wrong. @@ -393,5 +393,5 @@ esac MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}" -end_group "CI setup" +end_group "CI setup via $(basename $0)" set -x -- 2.47.0.366.g5daf58cba8.dirty ``` ## karthik nayak, 2024-12-06 11:39 Subject: Re: [PATCH 1/4] gitlab-ci: update macOS images to Sonoma Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRUZC9KDj9F6iLWWU%2BCBHz_DWKA_8znM%3D7RVSCVGSeD9w%40mail.gmail.com In-Reply-To: <20241206-pks-ci-section-fixes-v1-1-7ab1b69e3648@pks.im> ``` Patrick Steinhardt writes: > The macOS Ventura images we use for GitLab CI runners have been > deprecated. Update them to macOS 14, aka Sonoma. > > Signed-off-by: Patrick Steinhardt > --- > .gitlab-ci.yml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 526ecfe030a43e0a5a83ddd35cb7c96d46ab2485..61c56ccac8fdc940075d91dd4cb0b54ee33d5199 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -99,10 +99,10 @@ test:osx: > parallel: > matrix: > - jobname: osx-clang > - image: macos-13-xcode-14 > + image: macos-14-xcode-15 > CC: clang > - jobname: osx-reftable > - image: macos-13-xcode-14 > + image: macos-14-xcode-15 > CC: clang > artifacts: > paths: The changes look good, the documentation also states that if no image is mentioned, it'll use `macos-14-xcode-15` [1]. I wonder if this means that if the image is unspecified, it will always use the first non-deprecated version. That'd allow us to not have to keep updating this. [1]: https://docs.gitlab.com/ee/ci/runners/hosted_runners/macos.html#supported-macos-images ``` ## karthik nayak, 2024-12-06 11:46 Subject: Re: [PATCH 4/4] ci/lib: fix "CI setup" sections with GitLab CI Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZTdpmfKfuLj9PcXuO6_EqqR-2V4y8s2mpR3iXAS0sT32A%40mail.gmail.com In-Reply-To: <20241206-pks-ci-section-fixes-v1-4-7ab1b69e3648@pks.im> ``` Patrick Steinhardt writes: > Whenever we source "ci/lib.sh" we wrap the directives in a separate > group so that they can easily be collapsed in the web UI. And as we > source the script multiple times during a single CI run we thus end up > with the same section name reused multiple times, as well. > > This is broken on GitLab CI though, where reusing the same group name is > not supported. The consequence is that only the last of these sections > can be collapsed. > > Fix this issue by including the name of the sourcing script in the > group's name. > > Signed-off-by: Patrick Steinhardt > --- > ci/lib.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/ci/lib.sh b/ci/lib.sh > index ba8f4da39caf29db5edaffde160bc81a7c58c329..2cdc99e7fd05650ef80715b621b42d15d6b13a12 100755 > --- a/ci/lib.sh > +++ b/ci/lib.sh > @@ -55,7 +55,7 @@ group () { > return $res > } > > -begin_group "CI setup" > +begin_group "CI setup via $(basename $0)" > > # Set 'exit on error' for all CI scripts to let the caller know that > # something went wrong. > @@ -393,5 +393,5 @@ esac > > MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}" > > -end_group "CI setup" > +end_group "CI setup via $(basename $0)" > set -x Nice. Before, only the last instance of "CI setup" would be collapsible. Now that should be fixed. Here are the links for without and with this series. Without: https://gitlab.com/gitlab-org/git/-/jobs/8565481567#L20 With: https://gitlab.com/gitlab-org/git/-/jobs/8567031049 ``` ## karthik nayak, 2024-12-06 11:47 Subject: Re: [PATCH 0/4] Random improvements to GitLab CI Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZQeRWHog4Fjkb4MgK_34kV5JVEp4yAQ7Obgx6cnDJgyAA%40mail.gmail.com In-Reply-To: <20241206-pks-ci-section-fixes-v1-0-7ab1b69e3648@pks.im> ``` Patrick Steinhardt writes: > Hi, > > this small patch series includes a couple of more-or-less random > improvements to GitLab CI. > Apart from one suggestion(?) in the first commit, I think the series looks great! Thanks > Thanks! > > Patrick > > --- > Patrick Steinhardt (4): > gitlab-ci: update macOS images to Sonoma > ci/lib: remove duplicate trap to end "CI setup" group > ci/lib: use echo instead of printf to set up sections > ci/lib: fix "CI setup" sections with GitLab CI > > .gitlab-ci.yml | 4 ++-- > ci/lib.sh | 9 ++++----- > 2 files changed, 6 insertions(+), 7 deletions(-) > > > --- > base-commit: e66fd72e972df760a53c3d6da023c17adfc426d6 > change-id: 20241206-pks-ci-section-fixes-1bb91ceb50b8 ``` ## Patrick Steinhardt, 2024-12-06 12:02 Subject: Re: [PATCH 0/4] Random improvements to GitLab CI Message-ID: URL: https://gitlist.dev/e/Z1Ln2ObkKB3ZnoGD%40pks.im In-Reply-To: ``` On Fri, Dec 06, 2024 at 03:47:25AM -0800, karthik nayak wrote: > Patrick Steinhardt writes: > > > Hi, > > > > this small patch series includes a couple of more-or-less random > > improvements to GitLab CI. > > > > Apart from one suggestion(?) in the first commit, I think the series > looks great! Thanks Thanks for your review! Patrick ``` ## Patrick Steinhardt, 2024-12-06 12:02 Subject: Re: [PATCH 1/4] gitlab-ci: update macOS images to Sonoma Message-ID: URL: https://gitlist.dev/e/Z1Ln2492OCTwbII1%40pks.im In-Reply-To: ``` On Fri, Dec 06, 2024 at 06:39:59AM -0500, karthik nayak wrote: > Patrick Steinhardt writes: > > > The macOS Ventura images we use for GitLab CI runners have been > > deprecated. Update them to macOS 14, aka Sonoma. > > > > Signed-off-by: Patrick Steinhardt > > --- > > .gitlab-ci.yml | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > > index 526ecfe030a43e0a5a83ddd35cb7c96d46ab2485..61c56ccac8fdc940075d91dd4cb0b54ee33d5199 100644 > > --- a/.gitlab-ci.yml > > +++ b/.gitlab-ci.yml > > @@ -99,10 +99,10 @@ test:osx: > > parallel: > > matrix: > > - jobname: osx-clang > > - image: macos-13-xcode-14 > > + image: macos-14-xcode-15 > > CC: clang > > - jobname: osx-reftable > > - image: macos-13-xcode-14 > > + image: macos-14-xcode-15 > > CC: clang > > artifacts: > > paths: > > The changes look good, the documentation also states that if no image is > mentioned, it'll use `macos-14-xcode-15` [1]. I wonder if this means > that if the image is unspecified, it will always use the first > non-deprecated version. That'd allow us to not have to keep updating > this. > > [1]: https://docs.gitlab.com/ee/ci/runners/hosted_runners/macos.html#supported-macos-images It does, but at the cost of potential breakage whenever GitLab decides to update these images. It shouldn't happen all that frequently, but when we notice that it does become annoying we can iterate in the past and experiment with setting no image at all. Patrick ``` ## Oswald Buddenhagen, 2024-12-07 09:13 Subject: Re: [PATCH 3/4] ci/lib: use echo instead of printf to set up sections Message-ID: URL: https://gitlist.dev/e/Z1QRp8jFJPYdNmQk%40ugly In-Reply-To: <20241206-pks-ci-section-fixes-v1-3-7ab1b69e3648@pks.im> ``` On Fri, Dec 06, 2024 at 12:10:15PM +0100, Patrick Steinhardt wrote: >We use printf to set up sections with GitLab CI even though we could >trivially use echo. > probably not a good idea, because the file is also included from plain sh scripts, which may invoke an `echo` that cannot deal with \escapes. at least potentially. >This may cause problems in case the argument passed to `begin_group ()` >or `end_group ()` contains formatting directives as we use them as part >of the formatting string. > that can be fixed properly by using the %s expando. >+++ b/ci/lib.sh >- printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n" >+ echo "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1" ``` ## karthik nayak, 2024-12-07 09:49 Subject: Re: [PATCH 1/4] gitlab-ci: update macOS images to Sonoma Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZSExxSOna9Py%2B5Gx_dpLgom-fX48TGvcsi71f4xBXyoTA%40mail.gmail.com In-Reply-To: ``` Patrick Steinhardt writes: > On Fri, Dec 06, 2024 at 06:39:59AM -0500, karthik nayak wrote: >> Patrick Steinhardt writes: >> >> > The macOS Ventura images we use for GitLab CI runners have been >> > deprecated. Update them to macOS 14, aka Sonoma. >> > >> > Signed-off-by: Patrick Steinhardt >> > --- >> > .gitlab-ci.yml | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml >> > index 526ecfe030a43e0a5a83ddd35cb7c96d46ab2485..61c56ccac8fdc940075d91dd4cb0b54ee33d5199 100644 >> > --- a/.gitlab-ci.yml >> > +++ b/.gitlab-ci.yml >> > @@ -99,10 +99,10 @@ test:osx: >> > parallel: >> > matrix: >> > - jobname: osx-clang >> > - image: macos-13-xcode-14 >> > + image: macos-14-xcode-15 >> > CC: clang >> > - jobname: osx-reftable >> > - image: macos-13-xcode-14 >> > + image: macos-14-xcode-15 >> > CC: clang >> > artifacts: >> > paths: >> >> The changes look good, the documentation also states that if no image is >> mentioned, it'll use `macos-14-xcode-15` [1]. I wonder if this means >> that if the image is unspecified, it will always use the first >> non-deprecated version. That'd allow us to not have to keep updating >> this. >> >> [1]: https://docs.gitlab.com/ee/ci/runners/hosted_runners/macos.html#supported-macos-images > > It does, but at the cost of potential breakage whenever GitLab decides > to update these images. It shouldn't happen all that frequently, but > when we notice that it does become annoying we can iterate in the past > and experiment with setting no image at all. > Yeah, I get that. All good then! Karthik ``` ## Patrick Steinhardt, 2024-12-10 11:56 Subject: Re: [PATCH 3/4] ci/lib: use echo instead of printf to set up sections Message-ID: URL: https://gitlist.dev/e/Z1gscPivAADEvMGI%40pks.im In-Reply-To: ``` On Sat, Dec 07, 2024 at 10:13:11AM +0100, Oswald Buddenhagen wrote: > On Fri, Dec 06, 2024 at 12:10:15PM +0100, Patrick Steinhardt wrote: > > We use printf to set up sections with GitLab CI even though we could > > trivially use echo. > > > probably not a good idea, because the file is also included from plain > sh scripts, which may invoke an `echo` that cannot deal with \escapes. > at least potentially. Ah, true. > > This may cause problems in case the argument passed to `begin_group ()` > > or `end_group ()` contains formatting directives as we use them as part > > of the formatting string. > > > that can be fixed properly by using the %s expando. Yup, will do. Thanks! Patrick ``` ## Patrick Steinhardt, 2024-12-10 12:01 Subject: [PATCH v2 0/4] Random improvements to GitLab CI Message-ID: <20241210-pks-ci-section-fixes-v2-0-e087cfd174f4@pks.im> URL: https://gitlist.dev/e/20241210-pks-ci-section-fixes-v2-0-e087cfd174f4%40pks.im In-Reply-To: <20241206-pks-ci-section-fixes-v1-0-7ab1b69e3648@pks.im> ``` Hi, this small patch series includes a couple of more-or-less random improvements to GitLab CI. Changes in v2: - Use "%s" to inject variable data into section headers instead of converting to echo. - Link to v1: https://lore.kernel.org/r/20241206-pks-ci-section-fixes-v1-0-7ab1b69e3648@pks.im Thanks! Patrick --- Patrick Steinhardt (4): gitlab-ci: update macOS images to Sonoma ci/lib: remove duplicate trap to end "CI setup" group ci/lib: do not interpret escape sequences in `group ()` arguments ci/lib: fix "CI setup" sections with GitLab CI .gitlab-ci.yml | 4 ++-- ci/lib.sh | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) Range-diff versus v1: 1: c52dfd96a6 = 1: c98da734b2 gitlab-ci: update macOS images to Sonoma 2: 6214c8b6d5 = 2: d36bafc387 ci/lib: remove duplicate trap to end "CI setup" group 3: b56c6ec0a2 ! 3: 16882b0033 ci/lib: use echo instead of printf to set up sections @@ Metadata Author: Patrick Steinhardt ## Commit message ## - ci/lib: use echo instead of printf to set up sections + ci/lib: do not interpret escape sequences in `group ()` arguments - We use printf to set up sections with GitLab CI even though we could - trivially use echo. This may cause problems in case the argument passed - to `begin_group ()` or `end_group ()` contains formatting directives as - we use them as part of the formatting string. + We use printf to set up sections with GitLab CI, which requires us to + print a bunch of escape sequences via printf. The group name is + controlled by the user and is expanded directly into the formatting + string, which may cause problems in case the argument controls escape + sequences or formatting directives. - Simplify the code to instead use echo. + Fix this potential issue by using formatting directives to pass variable + data. Signed-off-by: Patrick Steinhardt @@ ci/lib.sh: elif test true = "$GITLAB_CI" begin_group () { need_to_end_group=t - printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n" -+ echo "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1" ++ printf '\e[0Ksection_start:%s:%s[collapsed=true]\r\e[0K%s\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" "$1" trap "end_group '$1'" EXIT set -x } @@ ci/lib.sh: then set +x need_to_end_group= - printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n" -+ echo "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K" ++ printf '\e[0Ksection_end:%s:%s\r\e[0K\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" trap - EXIT } else 4: a2bf5ac44f = 4: 6b67c4c238 ci/lib: fix "CI setup" sections with GitLab CI --- base-commit: e66fd72e972df760a53c3d6da023c17adfc426d6 change-id: 20241206-pks-ci-section-fixes-1bb91ceb50b8 ``` ## Patrick Steinhardt, 2024-12-10 12:01 Subject: [PATCH v2 1/4] gitlab-ci: update macOS images to Sonoma Message-ID: <20241210-pks-ci-section-fixes-v2-1-e087cfd174f4@pks.im> URL: https://gitlist.dev/e/20241210-pks-ci-section-fixes-v2-1-e087cfd174f4%40pks.im In-Reply-To: <20241210-pks-ci-section-fixes-v2-0-e087cfd174f4@pks.im> ``` The macOS Ventura images we use for GitLab CI runners have been deprecated. Update them to macOS 14, aka Sonoma. Signed-off-by: Patrick Steinhardt --- .gitlab-ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 526ecfe030a43e0a5a83ddd35cb7c96d46ab2485..61c56ccac8fdc940075d91dd4cb0b54ee33d5199 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -99,10 +99,10 @@ test:osx: parallel: matrix: - jobname: osx-clang - image: macos-13-xcode-14 + image: macos-14-xcode-15 CC: clang - jobname: osx-reftable - image: macos-13-xcode-14 + image: macos-14-xcode-15 CC: clang artifacts: paths: -- 2.47.1.447.ga7e8429e30.dirty ``` ## Patrick Steinhardt, 2024-12-10 12:01 Subject: [PATCH v2 2/4] ci/lib: remove duplicate trap to end "CI setup" group Message-ID: <20241210-pks-ci-section-fixes-v2-2-e087cfd174f4@pks.im> URL: https://gitlist.dev/e/20241210-pks-ci-section-fixes-v2-2-e087cfd174f4%40pks.im In-Reply-To: <20241210-pks-ci-section-fixes-v2-0-e087cfd174f4@pks.im> ``` We exlicitly trap on EXIT in order to end the "CI setup" group. This isn't necessary though given that `begin_group ()` already sets up the trap for us. Remove the duplicate trap. Signed-off-by: Patrick Steinhardt --- ci/lib.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/ci/lib.sh b/ci/lib.sh index 930f98d7228166c37c236beb062b14675fb68ef3..a54601be923bf475ba1a9cafd98bb1cb71a10255 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -56,7 +56,6 @@ group () { } begin_group "CI setup" -trap "end_group 'CI setup'" EXIT # Set 'exit on error' for all CI scripts to let the caller know that # something went wrong. -- 2.47.1.447.ga7e8429e30.dirty ``` ## Patrick Steinhardt, 2024-12-10 12:01 Subject: [PATCH v2 3/4] ci/lib: do not interpret escape sequences in `group ()` arguments Message-ID: <20241210-pks-ci-section-fixes-v2-3-e087cfd174f4@pks.im> URL: https://gitlist.dev/e/20241210-pks-ci-section-fixes-v2-3-e087cfd174f4%40pks.im In-Reply-To: <20241210-pks-ci-section-fixes-v2-0-e087cfd174f4@pks.im> ``` We use printf to set up sections with GitLab CI, which requires us to print a bunch of escape sequences via printf. The group name is controlled by the user and is expanded directly into the formatting string, which may cause problems in case the argument controls escape sequences or formatting directives. Fix this potential issue by using formatting directives to pass variable data. Signed-off-by: Patrick Steinhardt --- ci/lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/lib.sh b/ci/lib.sh index a54601be923bf475ba1a9cafd98bb1cb71a10255..f15f77f03a06120afbee438cee76ddc2683e1fa2 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -18,7 +18,7 @@ elif test true = "$GITLAB_CI" then begin_group () { need_to_end_group=t - printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n" + printf '\e[0Ksection_start:%s:%s[collapsed=true]\r\e[0K%s\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" "$1" trap "end_group '$1'" EXIT set -x } @@ -27,7 +27,7 @@ then test -n "$need_to_end_group" || return 0 set +x need_to_end_group= - printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n" + printf '\e[0Ksection_end:%s:%s\r\e[0K\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" trap - EXIT } else -- 2.47.1.447.ga7e8429e30.dirty ``` ## Patrick Steinhardt, 2024-12-10 12:01 Subject: [PATCH v2 4/4] ci/lib: fix "CI setup" sections with GitLab CI Message-ID: <20241210-pks-ci-section-fixes-v2-4-e087cfd174f4@pks.im> URL: https://gitlist.dev/e/20241210-pks-ci-section-fixes-v2-4-e087cfd174f4%40pks.im In-Reply-To: <20241210-pks-ci-section-fixes-v2-0-e087cfd174f4@pks.im> ``` Whenever we source "ci/lib.sh" we wrap the directives in a separate group so that they can easily be collapsed in the web UI. And as we source the script multiple times during a single CI run we thus end up with the same section name reused multiple times, as well. This is broken on GitLab CI though, where reusing the same group name is not supported. The consequence is that only the last of these sections can be collapsed. Fix this issue by including the name of the sourcing script in the group's name. Signed-off-by: Patrick Steinhardt --- ci/lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/lib.sh b/ci/lib.sh index f15f77f03a06120afbee438cee76ddc2683e1fa2..8ae8c0f77461d5bedc24e47be75711f2da2ade4f 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -55,7 +55,7 @@ group () { return $res } -begin_group "CI setup" +begin_group "CI setup via $(basename $0)" # Set 'exit on error' for all CI scripts to let the caller know that # something went wrong. @@ -393,5 +393,5 @@ esac MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}" -end_group "CI setup" +end_group "CI setup via $(basename $0)" set -x -- 2.47.1.447.ga7e8429e30.dirty ``` ## Toon Claes, 2024-12-11 12:37 Subject: Re: [PATCH v2 3/4] ci/lib: do not interpret escape sequences in `group ()` arguments Message-ID: <874j3ajsym.fsf@iotcl.com> URL: https://gitlist.dev/e/874j3ajsym.fsf%40iotcl.com In-Reply-To: <20241210-pks-ci-section-fixes-v2-3-e087cfd174f4@pks.im> ``` Patrick Steinhardt writes: > We use printf to set up sections with GitLab CI, which requires us to > print a bunch of escape sequences via printf. The group name is > controlled by the user and is expanded directly into the formatting > string, which may cause problems in case the argument controls escape > sequences or formatting directives. Could it be you mean "contains" instead of "controls"? > > Fix this potential issue by using formatting directives to pass variable > data. > > Signed-off-by: Patrick Steinhardt > --- > ci/lib.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/ci/lib.sh b/ci/lib.sh > index a54601be923bf475ba1a9cafd98bb1cb71a10255..f15f77f03a06120afbee438cee76ddc2683e1fa2 100755 > --- a/ci/lib.sh > +++ b/ci/lib.sh > @@ -18,7 +18,7 @@ elif test true = "$GITLAB_CI" > then > begin_group () { > need_to_end_group=t > - printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n" > + printf '\e[0Ksection_start:%s:%s[collapsed=true]\r\e[0K%s\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" "$1" Personally I find this line rather lengthy and hard to read with all the single and double quotes. So I would suggest to split the line with a backslash and put the arguments on a separate line. But I don't think there's a general guideline on this, so feel free to ignore. > trap "end_group '$1'" EXIT > set -x > } > @@ -27,7 +27,7 @@ then > test -n "$need_to_end_group" || return 0 > set +x > need_to_end_group= > - printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n" > + printf '\e[0Ksection_end:%s:%s\r\e[0K\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" Same here. But that's all I've got on this patch series. Looking good! -- Toon ``` ## Patrick Steinhardt, 2024-12-12 06:47 Subject: Re: [PATCH v2 3/4] ci/lib: do not interpret escape sequences in `group ()` arguments Message-ID: URL: https://gitlist.dev/e/Z1qGmp4NC21XYC4a%40pks.im In-Reply-To: <874j3ajsym.fsf@iotcl.com> ``` On Wed, Dec 11, 2024 at 01:37:53PM +0100, Toon Claes wrote: > Patrick Steinhardt writes: > > > We use printf to set up sections with GitLab CI, which requires us to > > print a bunch of escape sequences via printf. The group name is > > controlled by the user and is expanded directly into the formatting > > string, which may cause problems in case the argument controls escape > > sequences or formatting directives. > > Could it be you mean "contains" instead of "controls"? Oh, yeah. > > Fix this potential issue by using formatting directives to pass variable > > data. > > > > Signed-off-by: Patrick Steinhardt > > --- > > ci/lib.sh | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/ci/lib.sh b/ci/lib.sh > > index a54601be923bf475ba1a9cafd98bb1cb71a10255..f15f77f03a06120afbee438cee76ddc2683e1fa2 100755 > > --- a/ci/lib.sh > > +++ b/ci/lib.sh > > @@ -18,7 +18,7 @@ elif test true = "$GITLAB_CI" > > then > > begin_group () { > > need_to_end_group=t > > - printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n" > > + printf '\e[0Ksection_start:%s:%s[collapsed=true]\r\e[0K%s\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" "$1" > > Personally I find this line rather lengthy and hard to read with all the > single and double quotes. So I would suggest to split the line with a > backslash and put the arguments on a separate line. But I don't think > there's a general guideline on this, so feel free to ignore. Fair, will do. > > trap "end_group '$1'" EXIT > > set -x > > } > > @@ -27,7 +27,7 @@ then > > test -n "$need_to_end_group" || return 0 > > set +x > > need_to_end_group= > > - printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n" > > + printf '\e[0Ksection_end:%s:%s\r\e[0K\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" > > Same here. > > But that's all I've got on this patch series. Looking good! Thanks! Patrick ```