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

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 1, 2023, 11:44 UTC
Message-ID: <ZUI6Nv-HIxXuPMVC@tanuki>
In-Reply-To: <4c8c2f19-1a7e-4524-81e7-c74091e88edf@github.com>

On Tue, Oct 31, 2023 at 10:47:44AM -0700, Victoria Dye wrote:

Show 72 quoted lines

> Patrick Steinhardt wrote:> diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> > index 6e845283680..48cb2e735b5 100755
> > --- a/ci/install-docker-dependencies.sh
> > +++ b/ci/install-docker-dependencies.sh
> > @@ -7,6 +7,9 @@
> >  
> >  begin_group "Install dependencies"
> >  
> > +# Required so that apt doesn't wait for user input on certain packages.
> > +export DEBIAN_FRONTEND=noninteractive
> > +
> >  case "$jobname" in
> >  linux32)
> >  	linux32 --32bit i386 sh -c '
> > @@ -16,11 +19,19 @@ linux32)
> >  	'
> >  	;;
> >  linux-musl)
> > -	apk add --update build-base curl-dev openssl-dev expat-dev gettext \
> > +	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
> >  		pcre2-dev python3 musl-libintl perl-utils ncurses \
> >  		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
> >  		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
> >  	;;
> > +linux-*)
> > +	apt update -q &&
> > +	apt install -q -y sudo git make language-pack-is libsvn-perl apache2 libssl-dev \
> > +		libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev \
> > +		perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
> > +		libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
> > +		apache2 cvs cvsps gnupg libcgi-pm-perl subversion
> > +	;;
> >  pedantic)
> >  	dnf -yq update >/dev/null &&
> >  	dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
> 
> ...
> 
> > diff --git a/ci/lib.sh b/ci/lib.sh
> > index e14b1029fad..6e3d64004ec 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -208,6 +224,7 @@ then
> >  	cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
> >  
> >  	GIT_TEST_OPTS="--write-junit-xml"
> > +	JOBS=10
> >  elif test true = "$GITHUB_ACTIONS"
> >  then
> >  	CI_TYPE=github-actions
> 
> ...
> 
> > -MAKEFLAGS="$MAKEFLAGS --jobs=10"
> > -GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> > +MAKEFLAGS="$MAKEFLAGS --jobs=${JOBS}"
> > +GIT_PROVE_OPTS="--timer --jobs ${JOBS} --state=failed,slow,save"
> >  
> 
> Organizationally, this commit seems to be doing two things at once:
> 
> - Adding GitLab-specific CI setup (either in the new .gitlab-ci.yml or in
>   conditions gated on "gitlab-ci").
> - Updating the common CI scripts with things that are needed for GitLab CI,
>   but aren't conditioned on it (i.e. the patch excerpts I've included
>   above). 
> 
> I'd prefer these being separated into two patches, mainly to isolate "things
> that affect all CI" from "things that affect only GitLab CI". This is
> ultimately a pretty minor nit, though; if you're not planning on re-rolling
> (or just disagree with what I'm suggesting :) ), I'm okay with leaving it
> as-is.

Yeah, the JOBS refactoring can certainly be split out into a preparatory commit where we unify the envvars (currently patch 5). But for the other changes it makes a bit less sense to do so, in my opinion:

    - The DEBIAN_FRONTEND variable isn't needed before as the there are
      no Docker-based CI jobs that use apt.

    - Adding the shadow and sudo packages to the linux-musl job wouldn't
      be needed either as there are no cases yet where we run
      unprivileged CI builds via Docker.

    - Adding the apt packages as a preparatory step doesn't make much
      sense either as there is no Docker job using it.

But anyway. I will:

    - Move around the JOBS variable refactoring to a preparatory patch,
      which feels sensible to me.

    - Move the `DEBIAN_FRONTEND` varible into the "linux-*" case, which
      should further clarify that this only impacts the newly added and
      thus GitLab-specific infrastructure.

With these changes, the only thing left in this commit that is not guarded by a GitLab CI specific condition is the change to the "linux-musl" case where we install shadow and sudo now. But I don't feel like it makes sense to move them into a standalone preparatory commit.

Thanks!

Patrick

> Otherwise, I can't comment on the correctness of the GitLab CI definition (I
> assume you've tested it anyway), but AFAICT the changes above shouldn't break
> GitHub CI.