[PATCH 0/4] Random improvements to GitLab CI

[PATCH 0/4] Random improvements to GitLab CI

[Patrick Steinhardt]

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

[PATCH 1/4] gitlab-ci: update macOS images to Sonoma

[Patrick Steinhardt]

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 <ps@pks.im>
---
 .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

Re: [PATCH 1/4] gitlab-ci: update macOS images to Sonoma

[karthik nayak]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im>
> ---
>  .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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/4] gitlab-ci: update macOS images to Sonoma

[Patrick Steinhardt]

On Fri, Dec 06, 2024 at 06:39:59AM -0500, karthik nayak wrote:
> Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im>
> > ---
> >  .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

Re: [PATCH 1/4] gitlab-ci: update macOS images to Sonoma

[karthik nayak]

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Dec 06, 2024 at 06:39:59AM -0500, karthik nayak wrote:
>> Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im>
>> > ---
>> >  .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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH 2/4] ci/lib: remove duplicate trap to end "CI setup" group

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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

[PATCH 3/4] ci/lib: use echo instead of printf to set up sections

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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

Re: [PATCH 3/4] ci/lib: use echo instead of printf to set up sections

[Oswald Buddenhagen]

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"

Re: [PATCH 3/4] ci/lib: use echo instead of printf to set up sections

[Patrick Steinhardt]

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

[PATCH 4/4] ci/lib: fix "CI setup" sections with GitLab CI

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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

Re: [PATCH 4/4] ci/lib: fix "CI setup" sections with GitLab CI

[karthik nayak]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 0/4] Random improvements to GitLab CI

[karthik nayak]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

Patrick Steinhardt <ps@pks.im> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 0/4] Random improvements to GitLab CI

[Patrick Steinhardt]

On Fri, Dec 06, 2024 at 03:47:25AM -0800, karthik nayak wrote:
> Patrick Steinhardt <ps@pks.im> 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

[PATCH v2 0/4] Random improvements to GitLab CI

[Patrick Steinhardt]

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 <ps@pks.im>
     
      ## 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 <ps@pks.im>
     
    @@ 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

[PATCH v2 1/4] gitlab-ci: update macOS images to Sonoma

[Patrick Steinhardt]

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 <ps@pks.im>
---
 .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

[PATCH v2 2/4] ci/lib: remove duplicate trap to end "CI setup" group

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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

[PATCH v2 3/4] ci/lib: do not interpret escape sequences in `group ()` arguments

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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

Re: [PATCH v2 3/4] ci/lib: do not interpret escape sequences in `group ()` arguments

[Toon Claes]

Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im>
> ---
>  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

Re: [PATCH v2 3/4] ci/lib: do not interpret escape sequences in `group ()` arguments

[Patrick Steinhardt]

On Wed, Dec 11, 2024 at 01:37:53PM +0100, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im>
> > ---
> >  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

[PATCH v2 4/4] ci/lib: fix "CI setup" sections with GitLab CI

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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

[PATCH v3 0/4] Random improvements to GitLab CI

[Patrick Steinhardt]

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

Changes in v3:
- Wrap overly long lines.
- Fix a word swap in a commit message.
- Link to v2: https://lore.kernel.org/r/20241210-pks-ci-section-fixes-v2-0-e087cfd174f4@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      | 11 ++++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

Range-diff versus v2:

1:  e504aa4a1f = 1:  633af36640 gitlab-ci: update macOS images to Sonoma
2:  021ac694ef = 2:  6b6c6b2937 ci/lib: remove duplicate trap to end "CI setup" group
3:  6e355e22d7 ! 3:  bd54f8e1b6 ci/lib: do not interpret escape sequences in `group ()` arguments
    @@ Commit message
         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
    +    string, which may cause problems in case the argument contains escape
         sequences or formatting directives.
     
         Fix this potential issue by using formatting directives to pass variable
    @@ 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"
    -+		printf '\e[0Ksection_start:%s:%s[collapsed=true]\r\e[0K%s\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" "$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"
    -+		printf '\e[0Ksection_end:%s:%s\r\e[0K\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)"
    ++		printf '\e[0Ksection_end:%s:%s\r\e[0K\n' \
    ++			"$(date +%s)" "$(echo "$1" | tr ' ' _)"
      		trap - EXIT
      	}
      else
4:  bae2071e47 = 4:  41ad98d633 ci/lib: fix "CI setup" sections with GitLab CI

---
base-commit: e66fd72e972df760a53c3d6da023c17adfc426d6
change-id: 20241206-pks-ci-section-fixes-1bb91ceb50b8

[PATCH v3 1/4] gitlab-ci: update macOS images to Sonoma

[Patrick Steinhardt]

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 <ps@pks.im>
---
 .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

[PATCH v3 2/4] ci/lib: remove duplicate trap to end "CI setup" group

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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

[PATCH v3 3/4] ci/lib: do not interpret escape sequences in `group ()` arguments

[Patrick Steinhardt]

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 contains escape
sequences or formatting directives.

Fix this potential issue by using formatting directives to pass variable
data.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/lib.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index a54601be923bf475ba1a9cafd98bb1cb71a10255..d403ada911722af554df6255e5cd3fa01b56fd22 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -18,7 +18,8 @@ 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 +28,8 @@ 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

[PATCH v3 4/4] ci/lib: fix "CI setup" sections with GitLab CI

[Patrick Steinhardt]

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 <ps@pks.im>
---
 ci/lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index d403ada911722af554df6255e5cd3fa01b56fd22..5440eb6dc02784c03a5e4919f97dd7f07881f5c7 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -57,7 +57,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.
@@ -395,5 +395,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

Re: [PATCH v3 0/4] Random improvements to GitLab CI

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> 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
>
> Changes in v3:
> - Wrap overly long lines.
> - Fix a word swap in a commit message.
> - Link to v2: https://lore.kernel.org/r/20241210-pks-ci-section-fixes-v2-0-e087cfd174f4@pks.im

Thanks, all looks good to me.

--
Toon