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

[Patrick Steinhardt <ps@pks.im>]

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

On Sun, Oct 29, 2023 at 04:27:44PM +0000, Phillip Wood wrote:
> Hi Patrick
> 
> On 27/10/2023 12:19, Patrick Steinhardt wrote:
> > On Fri, Oct 27, 2023 at 11:19:04AM +0100, Phillip Wood wrote:
> > > On 27/10/2023 10:25, Patrick Steinhardt wrote:
> > > > +  parallel:
> > > > +    matrix:
> > > > +      - jobname: linux-sha256
> > > > +        image: ubuntu:latest
> > > > +        CC: clang
> > > > +      - jobname: linux-gcc
> > > > +        image: ubuntu:20.04
> > > > +        CC: gcc
> > > > +        CC_PACKAGE: gcc-8
> > > > +      - jobname: linux-TEST-vars
> > > > +        image: ubuntu:20.04
> > > > +        CC: gcc
> > > > +        CC_PACKAGE: gcc-8
> > > > +      - jobname: linux-gcc-default
> > > > +        image: ubuntu:latest
> > > > +        CC: gcc
> > > > +      - jobname: linux-leaks
> > > > +        image: ubuntu:latest
> > > > +        CC: gcc
> > > > +      - jobname: linux-asan-ubsan
> > > > +        image: ubuntu:latest
> > > > +        CC: clang
> > > > +      - jobname: linux-musl
> > > > +        image: alpine:latest
> 
> I assume you've chosen the configurations from the existing GitHub Actions
> that give the best coverage of the various options. One thing I noticed is
> that the is no equivalent of the "pedantic" job that builds git with
> "DEVELOPER=1 DEVOPTS=pedantic"

Yeah, indeed. The list for sure isn't complete, and I also want to
iterate in the future to add support for macOS and Windows. But adding
the pedantic job is easy enough, so I'll include it.

> > > > diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> > > > index d0bc19d3bb3..1cd92db1876 100755
> > > > --- 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
> > > 
> > > It would be helpful to explain the new dependencies in the commit message. I
> > > can see why you're adding sudo, but how were we getting away without
> > > installing the other packages for GitHub Actions?

I've dropped the "git" dependency for now. It doesn't do anything useful
anyway for our Docker-based jobs. Instead, I've added a preparatory
commit that makes the relevant CI library functions more robust by
detecting the case where we either don't have Git available or don't use
a repository.

> > True, that part is missing.
> > 
> > - Both sudo and shadow are now required because of `useradd` that we use
> >    to set up the unprivileged build.
> > 
> > - Git has been required all along, I think. `save_good_tree ()` is used
> >    in our CI scripts, and Toon (fellow GitLabber from my team) has
> >    noticed that the CI job warned about missing Git. The warning was
> >    mostly benign as it seems, but still, doesn't hurt to fix it while at
> >    it.
> 
> Oh I had a look at this and the docker based jobs on GitHub do not have a
> git repository so installing git means we now get a repository not found
> error from save_good_tree() instead. We should probably make
> save_good_tree() and check_unignored_build_artifacts() return early if there
> isn't a repository but that's orthogonal to this series.

Yup, done now.

> Looking at the test output from the link in your cover letter we should we
> should also install apache2[1] and gnupg[2]

True. Also missing are Bash, CVS, the Perl CGI module, Subversion and
Perforce. I'll add them as available in the respective distro's
repositories.

Patrick

> [1] https://gitlab.com/gitlab-org/git/-/jobs/5349205374#L1444
> [2] https://gitlab.com/gitlab-org/git/-/jobs/5349205374#L1167
> 
> Best Wishes
> 
> Phillip

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