On Thu, Oct 26, 2023 at 11:07:08AM +0200, Oswald Buddenhagen wrote:
> 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.
> > 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.
> > + 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.
> > --- 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
Attachment:
signature.asc
Description: PGP signature
