Re: [PATCH 5/5] ci: add support for GitLab CI

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 27, 2023, 08:17 UTC
Message-ID: <ZTtyHScKkMg4qnMH@tanuki>
In-Reply-To: <ZTosPCkpx/FMTDH5@ugly>

On Thu, Oct 26, 2023 at 11:07:08AM +0200, Oswald Buddenhagen wrote:

Show 31 quoted lines

> On Thu, Oct 26, 2023 at 10:00:20AM +0200, Patrick Steinhardt wrote:
> > project has. More esoteric jobs like "linux-TEST-vars" that also sets a
> 								     ^ 								     -s
> 
> > couple of environment variables do not exist in GitLab's custom CI
> > setup, and maintaining them to keep up with what Git does feels like
> > wasted time. The result is that we regularly send patch series upstream
> > that would otherwise fail to compile or pass tests in GitHub Workflows.
>       ^^^^^^^^^^^^^^^
>       that inverts the meaning
> 
> > [...]
> > project to help us ensure to send better patch series upstream and thus
> 			   ^^
> 			   "we"
> > reduce overhead for the maintainer.
> 
> > --- a/ci/install-docker-dependencies.sh
> > +++ b/ci/install-docker-dependencies.sh
> > @@ -16,9 +19,13 @@ linux32)
> > 	'
> > 	;;
> > linux-musl)
> > -	apk add --update build-base curl-dev openssl-dev expat-dev gettext \
> > +	apk add --update git shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
> > 		pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
> > 	;;
> > +linux-*)
> > 
> you should probably choose a less generic name for the jobs, at least
> debian-*.

The names are all preexisting, so I cannot change them. And the CI infra does indeed rely on the exact names to choose what to do.

Show 15 quoted lines

> > diff --git a/ci/lib.sh b/ci/lib.sh
> > index 33005854520..102e9d04a1f 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -15,6 +15,42 @@ then
> > 		echo '::endgroup::' >&2
> > 	}
> > 
> > +	group () {
> > [...]
> > +	}
> > 
> this counter-intutive patch structure reveals that the function is
> duplicated between github and gitlab. you may want to factor it out and
> alias it. or change the structure entirely (circling back to patch 1/5).

I don't quite know what you mean by counter-intuitive patch structure. But regarding the duplication for the `group ()` function I agree, it's a bit unfortunate. My first version did de-duplicate it indeed, but it resulted in some weirdness in the stubbed case.

I'll revisit this.

Show 9 quoted lines

> > +	CI_BRANCH="$CI_COMMIT_REF_NAME"
> > +	CI_COMMIT="$CI_COMMIT_SHA"
> > 
> assignments need no quoting to prevent word splitting.
> repeats below.
> 
> > +	case "$CI_JOB_IMAGE" in
> > 
> ... as does the selector in case statements.

True, but I'm simply matching the coding style in this script.

Show 16 quoted lines

> > --- a/ci/print-test-failures.sh
> > +++ b/ci/print-test-failures.sh
> > @@ -51,6 +51,12 @@ do
> > 			tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
> > 			continue
> > 			;;
> > +		gitlab-ci)
> > +			mkdir -p failed-test-artifacts
> > +			cp "${TEST_EXIT%.exit}.out" failed-test-artifacts/
> > +			tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
> > 
> you're just following the precedent, but imo it's more legible to quote the
> entire string, not just the variable expansion. the code doesn't even agree
> with itself, as the line directly above apparently agrees with me.
> 
> regards

Yeah, as you say, this is another case where I follow precedent. I honestly don't quite care which way we go in this case.

Patrick