patch, 5 partsci: reorder definitions for grouping functions

Hi,

this patch series adds GitLab CI definitions to the Git project.

At GitLab, we're have already and will continue to ramp up our involvement with the Git project. I myself will be working almost exclusively on Git in the context of the reftable reference backend. This has surfaced some issues in our own workflow, and the current CI integration is one of the biggest pain points I personally feel right now.

It's happened multiple times already that we sent patch series upstream that passed on our own self-made pipeline at GitLab, but that didn't pass the GitHub Actions pipeline. This is because the latter is a lot more involved than what we have. There are pipelines for:

    - Various sanitizers in the form of the linux-leaks and
      linux-asan-ubsan jobs.

    - SHA256 in the form of the linux-sha256 job.

    - The linux-TEST-vars job, which sets several environment variables
      to non-default values.

    - The linux-musl job that tests on Alpine Linux.

We have none of that. And while we could of course iterate on our own pipeline definition, the current setup results in quite a convoluted workflow on our side. While I realize that this is not the problem of the Git project, I really hope that we can integrate GitLab CI into the Git project.

And this is exactly what this patch series does: it adds GitLab-specific knowledge to our CI scripts and adds a CI definition that builds on top of those scripts. This is rather straight forward, as the scripts already know to discern Azure Pipelines and GitHub Actions, and adding a third item to this list feels quite natural. And by building on top of the preexisting infra, the actual ".gitlab-ci.yml" is really quite small.

I acknowledge that the Git project may not be willing to fully support GitLab CI, and that's fine with me. If we want to further stress that point then I'd also be perfectly happy to move the definitions into the "contrib/" directory -- it would still be a huge win for our workflow. In any case, I'm happy to keep on maintaining the intgeration with GitLab CI, and if things break I'll do my best to fix them fast.

I hope that this sheds some light on my motivations here. I do not wish to replace GitHub Actions and would be okay if this was only a semi-supported thing. But it would help us at GitLab and by extension also the Git project because we will hopefully send higher-quality patch series to the mailing list. And maybe this is even useful to somebody outside of GitLab.

If this is accepted I'll likely eventually iterate to also support macOS and/or Windows. A full pipeline run of this can be found at [1].

Patrick

[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1045746751

Patrick Steinhardt (5):
  ci: reorder definitions for grouping functions
  ci: make grouping setup more generic
  ci: group installation of Docker dependencies
  ci: split out logic to set up failed test artifacts
  ci: add support for GitLab CI

 .gitlab-ci.yml                    |  51 +++++++++++
 ci/install-docker-dependencies.sh |  15 +++-
 ci/lib.sh                         | 139 +++++++++++++++++++++++-------
 ci/print-test-failures.sh         |   6 ++
 4 files changed, 179 insertions(+), 32 deletions(-)
 create mode 100644 .gitlab-ci.yml

We define a set of grouping functions that are used to group together output in our CI, where these groups then end up as collapsible sections in the respective pipeline platform. The way these functions are defined is not easily extensible though as we have an up front check for the CI _not_ being GitLab Actions, where we define the non-stub logic in the else branch.

Reorder the definitions such that we explicitly handle GitHub Actions.

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

On Thu, Oct 26, 2023 at 10:00:03AM +0200, Patrick Steinhardt wrote:

you meant GitHub here.

i'd say something like "the conditional branches". imo that makes it clearer that you're actually talking about code, not some markup or whatever. for that matter, this is my overall impression of the commit message - it seems way too detached from the near-trivial fact that you're just slightly adjusting the code structure to make it easier to implement a cascade (aka a switch).

regards

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

The change is trivial indeed, but even a trivial change needs a reason why it should be done. Maybe it's too long, maybe it isn't... I'm happy to take suggestions.

But anyway, I've adopted both of the other two suggestions you made, thanks.

Patrick

Make the grouping setup more generic by always calling `begin_group ()` and `end_group ()` regardless of whether we have stubbed those functions or not. This ensures we can more readily add support for additional CI platforms.

Second, this commit changes `end_group ()` to also accept a parameter that indicates _which_ group should end. This will be required by a later commit that introduces support for GitLab CI.

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

Pull in "lib.sh" into "install-docker-dependencies.sh" such that we can set up proper groups for those dependencise. This allows the reader to collapse sections in the CI output on GitHub Actions (and later on on GitLab CI).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh | 6 ++++++
 1 file changed, 6 insertions(+)

On Thu, Oct 26, 2023 at 10:00:12AM +0200, Patrick Steinhardt wrote:

					  ^^ !!!

the structure of the text is kind of backwards - the fact that you need to pull in a lib is just a consequence, not the intent, which imo should come first. tough it mostly doesn't matter, as it's just one short paragraph.

regards

On Thu, Oct 26, 2023 at 10:34:10AM +0200, Oswald Buddenhagen wrote:

Good point indeed. Will rewrite.

Patrick

We have some logic in place to create a directory with the output from failed tests, which will then subsequently be uploaded as CI artifact. We're about to add support for GitLab CI, which will want to reuse the logic.

Split the logic into a separate function so that it is reusable.

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

On Thu, Oct 26, 2023 at 10:00:16AM +0200, Patrick Steinhardt wrote:

								      ^ 
								      +s

regards

On Thu, Oct 26, 2023 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:

Nit: I think it's more the responsibility of handle_failed_tests() to
`return 1` than of create_failed_test_artifacts(). I understand that
putting it into this new function means one more line that is common
to several CI tools though.

Otherwise everything in this series LGTM. Thanks!

On Fri, Nov 03, 2023 at 11:35:01PM +0100, Christian Couder wrote:

Good point indeed. Not sure whether this is worth a reroll though?

Patrick

We already support Azure Pipelines and GitHub Workflows in the Git project, but until now we do not have support for GitLab CI. While it is arguably not in the interest of the Git project to maintain a ton of different CI platforms, GitLab has recently ramped up its efforts and tries to contribute to the Git project more regularly.

Part of a problem we hit at GitLab rather frequently is that our own, custom CI setup we have is so different to the setup that the Git project has. More esoteric jobs like "linux-TEST-vars" that also sets a 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. We would thus like to integrate the GitLab CI configuration into the Git project to help us ensure to send better patch series upstream and thus reduce overhead for the maintainer.

The integration does not necessarily have to be a first-class citizen, which would in practice only add to the fallout that pipeline failures have for the maintainer. That being said, we are happy to maintain this alternative CI setup for the Git project and will make test results available as part of our own mirror of the Git project at [1].

This commit introduces the integration into our regular CI scripts so that most of the setup continues to be shared across all of the CI solutions.

[1]: https://gitlab.com/gitlab-org/git

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml                    | 51 +++++++++++++++++++++++
 ci/install-docker-dependencies.sh |  9 +++-
 ci/lib.sh                         | 69 +++++++++++++++++++++++++++++++
 ci/print-test-failures.sh         |  6 +++
 4 files changed, 134 insertions(+), 1 deletion(-)
 create mode 100644 .gitlab-ci.yml

On Thu, Oct 26, 2023 at 10:00:20AM +0200, Patrick Steinhardt wrote:

								     ^ 
								     -s

       ^^^^^^^^^^^^^^^
       that inverts the meaning

			   ^^
			   "we"

you should probably choose a less generic name for the jobs, at least debian-*.

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).

assignments need no quoting to prevent word splitting. repeats below.

... as does the selector in case statements.

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

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

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.

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.

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

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

On 27/10/2023 09:17, Patrick Steinhardt wrote:

I think it is quite common for us to quote variables when it isn't strictly necessary as it makes it clear to anyone reading the script that there is no word splitting going on and ensures that we don't start splitting the variable if the contents changes in the future.

Yes, if you're following existing practice I don't think this is worth worrying about.

Best Wishes

Phillip

On Fri, Oct 27, 2023 at 11:22:35AM +0100, Phillip Wood wrote:

the point was that it *isn't* content-dependent; it's simply the shell rules. of course, many people (apparently you included) don't know these subtleties, so just quoting everything isn't the worst idea (though it would backfire with some *really* old buggy shells, but this doesn't need to concern us).

regards

On 27/10/2023 11:43, Oswald Buddenhagen wrote:

Oh, I'd misunderstood what you were saying which was that assignment and case statements are not subject to field splitting.

I find this comment to be condescending, needlessly antagonistic and completely at odds with the norms of constructive discussion on this list.

Best Wishes

Phillip

On Fri, Oct 27, 2023 at 03:32:48PM +0100, Phillip Wood wrote:

the observation was necessary for the point i subsequently made (which was basically agreeing with the first part of your response).

i think it's a rather uncontroversial statement that many people don't know the ins and outs of the shell's word splitting rules. in fact, this is to be expected given how arbitrary the rules seem.

that you seem to be among these people was a reasonable inference from what you actually wrote. mentioning it was meant to provide a segue and add emphasis.

regards

On 27/10/2023 18:47, Oswald Buddenhagen wrote:

It was not necessary to phrase it as you did though. Before replying on Friday I showed your comment to someone else and their reaction was "That's rude". You could have made your point by saying something like

     It is hard to remember all the shell's word splitting rules so
     quoting everywhere is not a bad idea.

This is not the first time I've found your comments unnecessarily adversarial and at odds with the norms of constructive discussion and respectful disagreement on this list. I don't think I'm the only one either - in [1] Junio points out an ad-hominem remark and in [2] Marc comments on the unreceptive tone of you review responses.

I would urge you to try and strike a more conciliatory tone in your messages - it is perfectly possible to correct or disagree with someone without alienating them in the process.

Best Wishes

Phillip

[1] https://lore.kernel.org/git/xmqqleeihok5.fsf@gitster.g/ [2] https://lore.kernel.org/git/e33f919d-1b6a-4944-ab5d-93ad0d323b68@xiplink.com/

On 2023-10-30 10:49, Phillip Wood wrote:

Yeah, I've also noticed a not so great tone is some of Oswald's messages. It's perfectly fine to disagree on something, but it isn't that great to put the other party down while doing that, even to the point of insulting them.

We're all humans, and we should treat each other with respect. Furthermore, disagreeing in a friendly and polite way can many times lead to finding a better solution together.

On Fri, Oct 27, 2023 at 10:17:33AM +0200, Patrick Steinhardt wrote:

aren't they coming from the yml file? would adjusting them in the company setup be an unreasonable effort?

it looked like you're adding the function to the github branch, not to the freshly added gitlab branch. of course that's just a diffing artifact.

regards

On Fri, Oct 27, 2023 at 12:49:19PM +0200, Oswald Buddenhagen wrote:

They come from the ".gitlab-ci.yml" file, but I have to reuse the exact names that GitHub Actions already uses or otherwise we're not testing for the same thing. The preexisting CI scripts for Git expect exactly those names.

I do agree that they may benefit from a redesign so that they're more explicit. But I don't think this patch series here is where we should do that refactoring.

Ah, thought you meant the larger "structure of how things were layed out". Agreed and fixed in v2 of the patch series.

Patrick

Hi,

this is the second version of this patch series that adds GitLab CI definitions to the Git project. Please refer to the cover letter for v1 of this series [1] for the motivation and intent -- I won't repeat it here as it's a bit on the longer side.

Changes compared to v1:

    - Improved commit messages.

    - The `group ()` function is now generic across all the different CI
      solutions to reduce duplication.

This should hopefully address the feedback from Oswald -- thanks again!

Patrick

[1]: <cover.1698305961.git.ps@pks.im>

Patrick Steinhardt (5):
  ci: reorder definitions for grouping functions
  ci: make grouping setup more generic
  ci: group installation of Docker dependencies
  ci: split out logic to set up failed test artifacts
  ci: add support for GitLab CI

 .gitlab-ci.yml                    |  51 +++++++++++
 ci/install-docker-dependencies.sh |  15 +++-
 ci/lib.sh                         | 139 ++++++++++++++++++++----------
 ci/print-test-failures.sh         |   6 ++
 4 files changed, 166 insertions(+), 45 deletions(-)
 create mode 100644 .gitlab-ci.yml

Range-diff against v1:
1:  586a8d1003b ! 1:  4eb9cfc816b ci: reorder definitions for grouping functions
    @@ Commit message
         output in our CI, where these groups then end up as collapsible sections
         in the respective pipeline platform. The way these functions are defined
         is not easily extensible though as we have an up front check for the CI
    -    _not_ being GitLab Actions, where we define the non-stub logic in the
    +    _not_ being GitHub Actions, where we define the non-stub logic in the
         else branch.
     
    -    Reorder the definitions such that we explicitly handle GitHub Actions.
    +    Reorder the conditional branches such that we explicitly handle GitHub
    +    Actions.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
2:  ec390354f15 ! 2:  85617ef8577 ci: make grouping setup more generic
    @@ Commit message
         or not. This ensures we can more readily add support for additional CI
         platforms.
     
    -    Second, this commit changes `end_group ()` to also accept a parameter
    -    that indicates _which_ group should end. This will be required by a
    -    later commit that introduces support for GitLab CI.
    +    Furthermore, the `group ()` function is made generic so that it is the
    +    same for both GitHub Actions and for other platforms. There is a
    +    semantic conflict here though: GitHub Actions used to call `set +x` in
    +    `group ()` whereas the non-GitHub case unconditionally uses `set -x`.
    +    The latter would get overriden if we kept the `set +x` in the generic
    +    version of `group ()`. To resolve this conflict, we simply drop the `set
    +    +x` in the generic variant of this function. As `begin_group ()` calls
    +    `set -x` anyway this is not much of a change though, as the only
    +    commands that aren't printed anymore now are the ones between the
    +    beginning of `group ()` and the end of `begin_group ()`.
    +
    +    Last, this commit changes `end_group ()` to also accept a parameter that
    +    indicates _which_ group should end. This will be required by a later
    +    commit that introduces support for GitLab CI.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ ci/lib.sh: then
      		echo '::endgroup::' >&2
      	}
     -	trap end_group EXIT
    - 
    - 	group () {
    - 		set +x
    +-
    +-	group () {
    +-		set +x
     -		begin_group "$1"
    -+
    -+		group="$1"
    - 		shift
    -+		begin_group "$group"
    -+
    - 		# work around `dash` not supporting `set -o pipefail`
    - 		(
    - 			"$@" 2>&1
    -@@ ci/lib.sh: then
    - 		sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
    - 		res=$(cat exit.status)
    - 		rm exit.status
    +-		shift
    +-		# work around `dash` not supporting `set -o pipefail`
    +-		(
    +-			"$@" 2>&1
    +-			echo $? >exit.status
    +-		) |
    +-		sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
    +-		res=$(cat exit.status)
    +-		rm exit.status
     -		end_group
    -+
    -+		end_group "$group"
    - 		return $res
    - 	}
    +-		return $res
    +-	}
     -
     -	begin_group "CI setup"
      else
      	begin_group () { :; }
      	end_group () { :; }
    -@@ ci/lib.sh: else
    + 
    +-	group () {
    +-		shift
    +-		"$@"
    +-	}
      	set -x
      fi
      
    ++group () {
    ++	group="$1"
    ++	shift
    ++	begin_group "$group"
    ++
    ++	# work around `dash` not supporting `set -o pipefail`
    ++	(
    ++		"$@" 2>&1
    ++		echo $? >exit.status
    ++	) |
    ++	sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
    ++	res=$(cat exit.status)
    ++	rm exit.status
    ++
    ++	end_group "$group"
    ++	return $res
    ++}
    ++
     +begin_group "CI setup"
     +trap "end_group 'CI setup'" EXIT
     +
3:  a65d235dd3c ! 3:  57bbc50e3dc ci: group installation of Docker dependencies
    @@ Metadata
      ## Commit message ##
         ci: group installation of Docker dependencies
     
    -    Pull in "lib.sh" into "install-docker-dependencies.sh" such that we can
    -    set up proper groups for those dependencise. This allows the reader to
    -    collapse sections in the CI output on GitHub Actions (and later on on
    -    GitLab CI).
    +    The output of CI jobs tends to be quite long-winded and hard to digest.
    +    To help with this, many CI systems provide the ability to group output
    +    into collapsible sections, and we're also doing this in some of our
    +    scripts.
    +
    +    One notable omission is the script to install Docker dependencies.
    +    Address it to bring more structure to the output for Docker-based jobs.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
4:  4a864a1d174 ! 4:  5ab11d5236d ci: split out logic to set up failed test artifacts
    @@ Commit message
         ci: split out logic to set up failed test artifacts
     
         We have some logic in place to create a directory with the output from
    -    failed tests, which will then subsequently be uploaded as CI artifact.
    +    failed tests, which will then subsequently be uploaded as CI artifacts.
         We're about to add support for GitLab CI, which will want to reuse the
         logic.
     
5:  35b07e5378d ! 5:  37a507e9b25 ci: add support for GitLab CI
    @@ Commit message
     
         Part of a problem we hit at GitLab rather frequently is that our own,
         custom CI setup we have is so different to the setup that the Git
    -    project has. More esoteric jobs like "linux-TEST-vars" that also sets a
    +    project has. More esoteric jobs like "linux-TEST-vars" that also set a
         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.
    -    We would thus like to integrate the GitLab CI configuration into the Git
    -    project to help us ensure to send better patch series upstream and thus
    -    reduce overhead for the maintainer.
    +    that fail to compile or pass tests in GitHub Workflows. We would thus
    +    like to integrate the GitLab CI configuration into the Git project to
    +    help us send better patch series upstream and thus reduce overhead for
    +    the maintainer.
     
         The integration does not necessarily have to be a first-class citizen,
         which would in practice only add to the fallout that pipeline failures
    @@ ci/install-docker-dependencies.sh: linux32)
     
      ## ci/lib.sh ##
     @@ ci/lib.sh: then
    + 		need_to_end_group=
      		echo '::endgroup::' >&2
      	}
    - 
    -+	group () {
    -+		set +x
    -+
    -+		group="$1"
    -+		shift
    -+		begin_group "$group"
    -+
    -+		# work around `dash` not supporting `set -o pipefail`
    -+		(
    -+			"$@" 2>&1
    -+			echo $? >exit.status
    -+		) |
    -+		sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
    -+		res=$(cat exit.status)
    -+		rm exit.status
    -+
    -+		end_group "$group"
    -+		return $res
    -+	}
     +elif test true = "$GITLAB_CI"
     +then
     +	begin_group () {
    @@ ci/lib.sh: then
     +		echo -e "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K"
     +		trap - EXIT
     +	}
    -+
    - 	group () {
    - 		set +x
    - 
    + else
    + 	begin_group () { :; }
    + 	end_group () { :; }
     @@ ci/lib.sh: then
      	MAKEFLAGS="$MAKEFLAGS --jobs=10"
      	test windows != "$CI_OS_NAME" ||

We define a set of grouping functions that are used to group together output in our CI, where these groups then end up as collapsible sections in the respective pipeline platform. The way these functions are defined is not easily extensible though as we have an up front check for the CI _not_ being GitHub Actions, where we define the non-stub logic in the else branch.

Reorder the conditional branches such that we explicitly handle GitHub Actions.

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

Make the grouping setup more generic by always calling `begin_group ()` and `end_group ()` regardless of whether we have stubbed those functions or not. This ensures we can more readily add support for additional CI platforms.

Furthermore, the `group ()` function is made generic so that it is the same for both GitHub Actions and for other platforms. There is a semantic conflict here though: GitHub Actions used to call `set +x` in `group ()` whereas the non-GitHub case unconditionally uses `set -x`. The latter would get overriden if we kept the `set +x` in the generic version of `group ()`. To resolve this conflict, we simply drop the `set +x` in the generic variant of this function. As `begin_group ()` calls `set -x` anyway this is not much of a change though, as the only commands that aren't printed anymore now are the ones between the beginning of `group ()` and the end of `begin_group ()`.

Last, this commit changes `end_group ()` to also accept a parameter that indicates _which_ group should end. This will be required by a later commit that introduces support for GitLab CI.

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

The output of CI jobs tends to be quite long-winded and hard to digest. To help with this, many CI systems provide the ability to group output into collapsible sections, and we're also doing this in some of our scripts.

One notable omission is the script to install Docker dependencies. Address it to bring more structure to the output for Docker-based jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh | 6 ++++++
 1 file changed, 6 insertions(+)

We have some logic in place to create a directory with the output from failed tests, which will then subsequently be uploaded as CI artifacts. We're about to add support for GitLab CI, which will want to reuse the logic.

Split the logic into a separate function so that it is reusable.

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

We already support Azure Pipelines and GitHub Workflows in the Git project, but until now we do not have support for GitLab CI. While it is arguably not in the interest of the Git project to maintain a ton of different CI platforms, GitLab has recently ramped up its efforts and tries to contribute to the Git project more regularly.

Part of a problem we hit at GitLab rather frequently is that our own, custom CI setup we have is so different to the setup that the Git project has. More esoteric jobs like "linux-TEST-vars" that also set a 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 fail to compile or pass tests in GitHub Workflows. We would thus like to integrate the GitLab CI configuration into the Git project to help us send better patch series upstream and thus reduce overhead for the maintainer.

The integration does not necessarily have to be a first-class citizen, which would in practice only add to the fallout that pipeline failures have for the maintainer. That being said, we are happy to maintain this alternative CI setup for the Git project and will make test results available as part of our own mirror of the Git project at [1].

This commit introduces the integration into our regular CI scripts so that most of the setup continues to be shared across all of the CI solutions.

[1]: https://gitlab.com/gitlab-org/git

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml                    | 51 +++++++++++++++++++++++++++++++
 ci/install-docker-dependencies.sh |  9 +++++-
 ci/lib.sh                         | 49 +++++++++++++++++++++++++++++
 ci/print-test-failures.sh         |  6 ++++
 4 files changed, 114 insertions(+), 1 deletion(-)
 create mode 100644 .gitlab-ci.yml

On 27/10/2023 10:25, Patrick Steinhardt wrote:

I agree we don't want to support too many CI platforms but I think adding support for GitLab is good as it helps to stop us being too tied to GitHub Actions (which should make it easier if we ever need to transition to a different platform in the future) and provides an alternative for contributors who want to use a different platform.

Having someone committed to on-going maintenance is great.

It's really good that you're running the tests as an unprivileged user. This is something we used to do when we were using Travis that got lost in the transition to Azure Pipelines which means some tests that rely on httpd are now skipped as they refuse to run as root. ci/run-docker-build.sh is currently bit-rotting, I wonder if it is possible to update it so that we can run the dockerized tests in the same way on all CI platforms.

This file is pleasingly small.

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 guess making this explicit is helpful, otherwise someone may wonder why CI_JOB_ID is not being set.

This last paragraph feels like it should be common to all the CI providers. There are some small differences but if we're going to support several providers it would be nice to set the common options centrally. I'm pretty sure the --jobs=10 we use for the GitHub and Azure is not optimal, using $(nproc) is nice so long as it is supported on all the images we use.

I had a quick glance through the previous patches and they all look like nice cleanups that make our ci support less dependent on a single provider. This series looks like a nice addition to our CI support.

Best Wishes

Phillip

On Fri, Oct 27, 2023 at 11:19:04AM +0100, Phillip Wood wrote:

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.

I'll have a look at whether I can add another patch on top that adjusts `ci/run-docker-build.sh` to do rootless builds, which would also make it more obvious why we now need to install sudo and shadow. And I'll make sure to document why we now need to have Git around.

True enough, I can have a go at it. I'm rather cautious around updating the build infra as I cannot easily verify that things continue to work for GitHub. But this part here might be a good candidate for deduping.

Thanks!

Patrick

On Fri, Oct 27, 2023 at 01:19:35PM +0200, Patrick Steinhardt wrote:

[snip]

Hum. After having a look at `ci/run-docker-build.sh` I don't feel like it's sensible to update it. It's not even used anymore by our CI but only by `ci/run-docker.sh`, which seems to be more of a developer-facing script?

As you said, this smells like rotting bits that might rather be removed. But in any case, as they don't relate to our current CI infrastructure except for being in "ci/" I'll leave them be for now.

Patrick

On 27/10/2023 12:57, Patrick Steinhardt wrote:

I was trying to suggest that we start using these scripts again. The fact that we run the dockerized tests as root on the other CI platforms is a regression from what we used to do. I'm not an expert but I think for the builds that use docker we're essentially using the some build environment regardless of the CI provider so it would make sense to handle them all in the same way. I think the existing script uses "su" but we could change it to use "sudo" like you're doing here.

Best Wishes

Phillip

Hi Patrick

On 27/10/2023 14:02, Phillip Wood wrote:

Having taken a closer look I think we'd be better off adding something like

   # Ensure the build and tests run as an unprivileged user
   if test "$(id -u)" -eq 0
   then
       useradd --home-dir "$(pwd)" builder
       chown -R builder .
       exec sudo --preserve-env --set-home --user=builder "$0"
   fi

To the beginning of ci/run-build-and-tests.sh.

Best Wishes

Phillip

On Sun, Oct 29, 2023 at 04:13:39PM +0000, Phillip Wood wrote:

That indeed looks like a nice way to handle this, agreed. As mentioned though, I don't really have an easy way to test this with GitHub Workflows or Azure Pipelines. So I'd propose to defer this change to a follow-up patch series -- and in the best case somebody who is familiar with these CI solutions would pick it up rather than me.

Patrick

Hi Patrick

On 27/10/2023 12:19, Patrick Steinhardt wrote:

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"

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.

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

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

Best Wishes

Phillip

On Sun, Oct 29, 2023 at 04:27:44PM +0000, Phillip Wood wrote:

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.

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.

Yup, done now.

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

Phillip Wood <phillip.wood123@gmail.com> writes:

Yeah, it is great to see that stakeholder companies are helping Git in ways they can.

Will queue.

On Fri, Oct 27, 2023 at 11:25:41AM +0200, Patrick Steinhardt wrote:

fwiw (as this is again only copied), export with assignment is a bash-ism (though (d)ash started to accept this syntax some time ago), and not all of the including scripts ask for bash (i didn't check whether they are using these functions, but the inconsistency is an armed trap).

regards

On 27/10/2023 12:01, Oswald Buddenhagen wrote:

Not according to https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#export

     SYNOPSIS

         export name[=word]...

     DESCRIPTION

         The shell shall give the export attribute to the variables
         corresponding to the specified names, which shall cause them
         to be in the environment of subsequently executed commands. If
         the name of a variable is followed by = word, then the value
         of that variable shall be set to word.

It is true that in our test suite we separate a variable assignment when exporting. Presumably that is because someone reported that their shell did not support the "export name=WORD" syntax in the past. As we're already using this syntax with the same docker images in Github Actions I think we can assume it is safe here.

Best Wishes

Phillip

On Fri, Oct 27, 2023 at 02:17:02PM +0100, Phillip Wood wrote:

hmm, it's there since at least SUSv1, aka XPG4v2, in 1994. i didn't bother digging deeper.

most likely it's just a historical default, not the result of a specific bug report. the assumption that it's bash-specific is wide-spread.

i guess so. docker seems a tad unlikely to run some ancient bourne shells ...

regards

On Fri, Oct 27, 2023 at 02:17:02PM +0100, Phillip Wood wrote:

I've wondered about the origin of this myself, and tried to do some digging. All of the commits I found removing "export var=val" vaguely say "unportable" or "some shells can't handle", etc.

The oldest mention I found on the mailing list was this thread:

  https://lore.kernel.org/git/7vfyb0wexo.fsf@assigned-by-dhcp.cox.net/

which is even more explicit about its vagueness.

I wouldn't be surprised if SunOS/Solaris /bin/sh had problems with it, as that has been a common headache shell in the past. But I think we finally declared it unusable for other reasons (and they ship a more capable shell in /usr/xpg6, if anybody even still wants to build on those operating systems these days).

So it's possible that avoiding "export var=val" is mostly superstition, and we could loosen our rules these days. But some things to consider:

  1. Some people may prefer reading the separated form (Junio indicates
     such in the thread linked above, but I don't know how strong a
     preference that is).

  2. We won't really know if there is a odd-ball shell that rejects it
     unless we make a change and wait for a while to see if anybody
     screams. The existing ones in ci/ show that it is not a problem for
     the platforms where we run CI, but I suspect the scripts in t/ see
     a wider audience.

I don't think this has any real bearing on the patches being proposed, but I have been curious about this for our other scripts for a while now.

-Peff

Jeff King <peff@peff.net> writes:

Thanks for digging up the old thread. I would not be surprised if it was already superstition back in the days.

We could start with a single weather balloon use in t/ somewhere to see if anybody screams. It would be a good place to start if we want to get rid of this particular superstition.

Hi,

this is the third version of this patch series that adds GitLab CI definitions to the Git project. Please refer to the cover letter for v1 of this series [1] for the motivation and intent -- I won't repeat it here as it's a bit on the longer side.

Changes compared to v2:

    - Patch 5: This is a new preparatory step to unify the setup of some
      environment variables. It also fixes some smallish issues, like
      e.g. the fact that some envvars were set _after_ the export.

    - Patch 6: Another new preparatory step. It makes our infra around
      certain helper functions that have the intent to interact with the
      project repository, e.g. to cache good trees. We now detect the
      case when there is no Git or when the project is not a Git repo
      and bail out gracefully.

    - Patch 7: The last new preparatory patch. Installs a bunch of
      dependencies which are required for the test runtime in the Alpine
      based job. This increases test coverage.

    - Patch 8: Several smaller improvements:
        - Added a note why we install sudo and shadow in linux-musl now.
        - Fixed an issue where the HOME directory was part of the
          project directory, and thus Git complained about newly added
          untracked files.
        - Added the "pedantic" job that does a pednatic compilation on
          Fedora.
        - Added more test time dependencies.
        - Made the "echo -e" invocation portable by using printf
          instead.

What I didn't address yet is a suggestion by Phillip, namely to unify the logic that sets up unprivileged builds. I don't have the infra available to test any such change that would ultimately also impact GitHub Workflows and Azure Pipelines and thus do not feel comfortable to refactor this. I agree with the suggestion though, so I propose to rather handle it at a later point in time.

A test run of this patch series can be found at [2].

Thanks!

Patrick

[1]: <cover.1698305961.git.ps@pks.im> [2]: https://gitlab.com/gitlab-org/git/-/pipelines/1054750795

Patrick Steinhardt (8):
  ci: reorder definitions for grouping functions
  ci: make grouping setup more generic
  ci: group installation of Docker dependencies
  ci: split out logic to set up failed test artifacts
  ci: unify setup of some environment variables
  ci: squelch warnings when testing with unusable Git repo
  ci: install test dependencies for linux-musl
  ci: add support for GitLab CI

 .gitlab-ci.yml                    |  53 +++++++++
 ci/install-docker-dependencies.sh |  22 +++-
 ci/lib.sh                         | 187 +++++++++++++++++++++---------
 ci/print-test-failures.sh         |   6 +
 t/lib-httpd.sh                    |   3 +-
 5 files changed, 215 insertions(+), 56 deletions(-)
 create mode 100644 .gitlab-ci.yml

Range-diff against v2:
1:  4eb9cfc816b = 1:  ef44ed5c3b1 ci: reorder definitions for grouping functions
2:  85617ef8577 = 2:  77798fa7a7a ci: make grouping setup more generic
3:  57bbc50e3dc = 3:  4542bd38dc2 ci: group installation of Docker dependencies
4:  5ab11d5236d = 4:  5fdda7fd83f ci: split out logic to set up failed test artifacts
-:  ----------- > 5:  6af0075fd87 ci: unify setup of some environment variables
-:  ----------- > 6:  78d863bf24e ci: squelch warnings when testing with unusable Git repo
-:  ----------- > 7:  f150d61a1ce ci: install test dependencies for linux-musl
5:  37a507e9b25 ! 8:  5272d66d9f1 ci: add support for GitLab CI
    @@ Commit message
     
         This commit introduces the integration into our regular CI scripts so
         that most of the setup continues to be shared across all of the CI
    -    solutions.
    +    solutions. Note that as the builds on GitLab CI run as unprivileged
    +    user, we need to pull in both sudo and shadow packages to our Alpine
    +    based job to set this up.
     
         [1]: https://gitlab.com/gitlab-org/git
     
    @@ .gitlab-ci.yml (new)
     +  before_script:
     +    - ./ci/install-docker-dependencies.sh
     +  script:
    -+    - useradd builder --home-dir "${CI_PROJECT_DIR}"
    ++    - useradd builder --create-home
     +    - chown -R builder "${CI_PROJECT_DIR}"
     +    - sudo --preserve-env --set-home --user=builder ./ci/run-build-and-tests.sh
     +  after_script:
    @@ .gitlab-ci.yml (new)
     +      - jobname: linux-asan-ubsan
     +        image: ubuntu:latest
     +        CC: clang
    ++      - jobname: pedantic
    ++        image: fedora:latest
     +      - jobname: linux-musl
     +        image: alpine:latest
     +  artifacts:
    @@ ci/install-docker-dependencies.sh: 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
    ++	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
    + 		pcre2-dev python3 musl-libintl perl-utils ncurses \
    + 		apache2 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 libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}}
    ++	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 &&
    @@ ci/lib.sh: then
     +then
     +	begin_group () {
     +		need_to_end_group=t
    -+		echo -e "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1"
    ++		printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1\n"
     +		trap "end_group '$1'" EXIT
     +		set -x
     +	}
    @@ ci/lib.sh: then
     +		test -n "$need_to_end_group" || return 0
     +		set +x
     +		need_to_end_group=
    -+		echo -e "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K"
    ++		printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n"
     +		trap - EXIT
     +	}
      else
      	begin_group () { :; }
      	end_group () { :; }
     @@ ci/lib.sh: then
    - 	MAKEFLAGS="$MAKEFLAGS --jobs=10"
    - 	test windows != "$CI_OS_NAME" ||
    - 	GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
    + 	cache_dir="$HOME/none"
    + 
    + 	GIT_TEST_OPTS="--github-workflow-markup"
     +elif test true = "$GITLAB_CI"
     +then
     +	CI_TYPE=gitlab-ci
    @@ ci/lib.sh: then
     +	case "$CI_JOB_IMAGE" in
     +	macos-*)
     +		CI_OS_NAME=osx;;
    -+	alpine:*|ubuntu:*)
    ++	alpine:*|fedora:*|ubuntu:*)
     +		CI_OS_NAME=linux;;
     +	*)
     +		echo "Could not identify OS image" >&2
    @@ ci/lib.sh: then
     +	cache_dir="$HOME/none"
     +
     +	runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
    -+
    -+	export GIT_PROVE_OPTS="--timer --jobs $(nproc)"
    -+	export GIT_TEST_OPTS="--verbose-log -x"
    -+	MAKEFLAGS="$MAKEFLAGS --jobs=$(nproc)"
    -+	test windows != "$CI_OS_NAME" ||
    -+	GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
      else
      	echo "Could not identify CI type" >&2
      	env >&2

We define a set of grouping functions that are used to group together output in our CI, where these groups then end up as collapsible sections in the respective pipeline platform. The way these functions are defined is not easily extensible though as we have an up front check for the CI _not_ being GitHub Actions, where we define the non-stub logic in the else branch.

Reorder the conditional branches such that we explicitly handle GitHub Actions.

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

Make the grouping setup more generic by always calling `begin_group ()` and `end_group ()` regardless of whether we have stubbed those functions or not. This ensures we can more readily add support for additional CI platforms.

Furthermore, the `group ()` function is made generic so that it is the same for both GitHub Actions and for other platforms. There is a semantic conflict here though: GitHub Actions used to call `set +x` in `group ()` whereas the non-GitHub case unconditionally uses `set -x`. The latter would get overriden if we kept the `set +x` in the generic version of `group ()`. To resolve this conflict, we simply drop the `set +x` in the generic variant of this function. As `begin_group ()` calls `set -x` anyway this is not much of a change though, as the only commands that aren't printed anymore now are the ones between the beginning of `group ()` and the end of `begin_group ()`.

Last, this commit changes `end_group ()` to also accept a parameter that indicates _which_ group should end. This will be required by a later commit that introduces support for GitLab CI.

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

The output of CI jobs tends to be quite long-winded and hard to digest. To help with this, many CI systems provide the ability to group output into collapsible sections, and we're also doing this in some of our scripts.

One notable omission is the script to install Docker dependencies. Address it to bring more structure to the output for Docker-based jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh | 6 ++++++
 1 file changed, 6 insertions(+)

We have some logic in place to create a directory with the output from failed tests, which will then subsequently be uploaded as CI artifacts. We're about to add support for GitLab CI, which will want to reuse the logic.

Split the logic into a separate function so that it is reusable.

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

Both GitHub Actions and Azue Pipelines set up the environment variables GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are actually the same, the setup is completely duplicate. With the upcoming support for GitLab CI this duplication would only extend even further.

Unify the setup of those environment variables so that only the uncommon parts are separated. While at it, we also perform some additional small improvements:

    - We use nproc instead of a hardcoded count of jobs for make and
      prove. This ensures that the number of concurrent processes adapts
      to the host automatically.

    - We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
      It doesn't hurt on platforms where we don't persist the state, so
      this further reduces boilerplate.

    - When running on Windows systems we set `--no-chain-lint` and
      `--no-bin-wrappers`. Interestingly though, we did so _after_
      already having exported the respective environment variables.

    - We stop using `export VAR=value` syntax, which is a Bashism. It's
      not quite worth it as we still use this syntax all over the place,
      but it doesn't hurt readability either.

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

Hi Patrick

On 30/10/2023 12:15, Patrick Steinhardt wrote:

Sadly this makes the Windows and MacOS jobs fail on GitHub Actions as nproc is not installed[1]. Perhaps we could do

	--jobs="$(nproc || echo 2)"

instead. (Maybe 2 is a bit low but the current value of 10 seems pretty high for the number of cores on the runners that we use)

 > >      - We stop using `export VAR=value` syntax, which is a Bashism. 
It's

I don't mind this change, but the 'export VAR=value' syntax is in POSIX[2]

 >

I was wondering why we don't export MAKEFLAGS here but it is exported earlier on before we set it. Apart from the nproc issue this looks like a nice improvement

Best Wishes

Phillip

[1] https://github.com/phillipwood/git/actions/runs/6694263874 [2] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#export

On Mon, Oct 30, 2023 at 03:09:01PM +0000, Phillip Wood wrote:

Ugh, thanks. I'll update it to keep the hardcoded 10 jobs in place for now for the other pipelines. Trying to do too many things at once is only going to make it harder to get this landed, doubly so when you have no easy way to verify what you're doing :)

I don't quite mind it, either. The reason why I chose to use it is that there was indeed a bug already with the order of exports and modification of the vars after the export. So with that in mind it made sense to me to adapt it accordingly.

In fact, I also noticed later today that we adapt MAKEFLAGS at a later point again without reexporting it, so it's got the same issue. I'll adapt it in the same way.

Patrick

On 2023-10-30 16:09, Phillip Wood wrote:

It would be better to use the following, to also suppress any error messages:

         --jobs=$(nproc 2> /dev/null || echo 2)

Having the quotation marks is also pretty much redundant.

Our CI jobs that run on Docker also use mostly the same architecture to build and test Git via the "ci/run-build-and-tests.sh" script. These scripts also provide some functionality to massage the Git repository we're supposedly operating in.

In our Docker-based infrastructure we may not even have a Git repository available though, which leads to warnings when those functions execute. Make the helpers exit gracefully in case either there is no Git in our PATH, or when not running in a Git repository.

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

The linux-musl CI job executes tests on Alpine Linux, which is based on musl libc instead of glibc. We're missing some test dependencies though, which causes us to skip a subset of tests.

Install these test dependencies to increase our test coverage on this platform. There are still some missing test dependecies, but these do not have a corresponding package in the Alpine repositories:

    - p4 and p4d, both parts of the Perforce version control system.

    - cvsps, which generates patch sets for CVS.

    - Subversion and the SVN::Core Perl library, the latter of which is
      not available in the Alpine repositories. While the tool itself is
      available, all Subversion-related tests are skipped without the
      SVN::Core Perl library anyway.

Furthermore, in order to make the Apache-based tests work, this commit also adds the Alpine-specific modules path of it to the list of known paths.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh | 3 ++-
 t/lib-httpd.sh                    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

On Mon, Oct 30, 2023 at 01:15:10PM +0100, Patrick Steinhardt wrote:

Meh. I just noticed that I missed a few other dependencies to make Apache2 work:

On Mon, Oct 30, 2023 at 01:47:30PM +0100, Patrick Steinhardt wrote:

I've been digging a bit, and the issue comes from the the DAV module indeed:

``` [Mon Oct 30 12:36:19.776149 2023] [dav_fs:crit] [pid 275752] (20019)DSO load failed: AH00576: The DBM driver could not be loaded [Mon Oct 30 12:36:19.776168 2023] [dav:error] [pid 275752] [client 127.0.0.1:51388] Could not LOCK /dumb/test_repo.git/info/refs due to a failed precondition (e.g. other locks). [500, #0] [Mon Oct 30 12:36:19.776174 2023] [dav:error] [pid 275752] [client 127.0.0.1:51388] The locks could not be queried for verification against a possible "If:" header. [500, #0] [Mon Oct 30 12:36:19.776177 2023] [dav:error] [pid 275752] [client 127.0.0.1:51388] Could not open the lock database. [500, #400] [Mon Oct 30 12:36:19.776181 2023] [dav:error] [pid 275752] (20019)DSO load failed: [client 127.0.0.1:51388] Could not open property database. [500, #1] ```

This seems to be a known limitation in Alpine Linux as they do not package apr-util-dbm_db anymore due to license incompatibilities with with Berkely DB [1], and the WebDAV module does rely on it to provide locking.

In the best case we'd be able to detect this limitation and skip those tests automatically so that we can at least execute all the other Apache tests. But again, this rather feels like something we should do as a follow up rather than as part of this series.

Patrick

[1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/12534

Hi Patrick

On 30/10/2023 12:47, Patrick Steinhardt wrote:

Yes, or we could just leave it - I had not realized before that it was only the musl job that was not running the httpd tests (I thought install-docker-dependencies.sh was missing the packages for ubuntu as well). Given that, the status quo does not seem so bad.

Best Wishes

Phillip

On Mon, Oct 30, 2023 at 03:13:35PM +0000, Phillip Wood wrote:

I of course couldn't let go. The following would fix this:

On 30/10/2023 15:23, Patrick Steinhardt wrote:

Great, that was fast! As you've got a fix I agree it makes sense to include it in this series.

Best Wishes

Phillip

We already support Azure Pipelines and GitHub Workflows in the Git project, but until now we do not have support for GitLab CI. While it is arguably not in the interest of the Git project to maintain a ton of different CI platforms, GitLab has recently ramped up its efforts and tries to contribute to the Git project more regularly.

Part of a problem we hit at GitLab rather frequently is that our own, custom CI setup we have is so different to the setup that the Git project has. More esoteric jobs like "linux-TEST-vars" that also set a 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 fail to compile or pass tests in GitHub Workflows. We would thus like to integrate the GitLab CI configuration into the Git project to help us send better patch series upstream and thus reduce overhead for the maintainer.

The integration does not necessarily have to be a first-class citizen, which would in practice only add to the fallout that pipeline failures have for the maintainer. That being said, we are happy to maintain this alternative CI setup for the Git project and will make test results available as part of our own mirror of the Git project at [1].

This commit introduces the integration into our regular CI scripts so that most of the setup continues to be shared across all of the CI solutions. Note that as the builds on GitLab CI run as unprivileged user, we need to pull in both sudo and shadow packages to our Alpine based job to set this up.

[1]: https://gitlab.com/gitlab-org/git

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml                    | 53 +++++++++++++++++++++++++++++++
 ci/install-docker-dependencies.sh | 13 +++++++-
 ci/lib.sh                         | 43 +++++++++++++++++++++++++
 ci/print-test-failures.sh         |  6 ++++
 4 files changed, 114 insertions(+), 1 deletion(-)
 create mode 100644 .gitlab-ci.yml

On Thu, Oct 26, 2023 at 09:59:59AM +0200, Patrick Steinhardt wrote:

I don't have any strong opinions here, but my preference would probably be to keep any GitLab-specific CI configuration limited to "contrib", if it lands in the tree at all.

We already have a rather complicated CI setup on GitHub, which I think we generally consider authoritative in terms of determining whether "CI" is green. I know we have some Azure remnants in "ci", but I'm not aware of any of the details there.

So I have some hesitation about trying to mirror this rather complicated set of build rules in another CI environment. My primary concern would be that the two might fall out of sync and a series that is green on GitHub would be red on GitLab, or vice-versa. Importantly, this can happen even without changes to the build definitions, since (AFAICT) both forges distribute new images automatically, so the set of packages installed in GitHub may not exactly match what's in GitLab (and vice-versa).

My other concern is that we're doubling the cost of any new changes to our CI definition. Perhaps this is more of an academic concern, but I think my fear would be that one or the other would fall behind on in implementation leading to further divergence between the two.

I think having the new CI definition live in "contrib" somewhat addresses the "which CI is authoritative?" problem, but that it doesn't address the "we have two of these" problem.

So my preference would probably to have this live out of Junio's tree, but I'm curious to hear what others think.

Thanks, Taylor

On Mon, Oct 30, 2023 at 11:46:44AM -0400, Taylor Blau wrote:

As mentioned, I would not mind at all if we wanted to instead carry this as part of "contrib/".

Yup, that's a valid concern. As mentioned, this patch series does not have the intent to make GitLab CI a second authoritative CI platform. GitHub Actions should remain the source of truth of whether a pipeline passes or not. Most importantly, I do not want to require the maintainer to now watch both pipelines on GitHub and GitLab. This might be another indicator that the pipeline should rather be in "contrib/", so that people don't start to treat it as authoritative.

I do see that this requires us to be a bit more careful with future changes to our CI definitions. But I think the additional work that this creates is really very limited. Except for the `.gitlab-ci.yml`, there are only 54 lines specific to GitLab in our CI scripts now, which I think should be rather manageable.

I also think that it is sensible to ensure that our CI scripts are as agnostic to the CI platform as possible, as it ensures that we continue to be agile here in the future if we are ever forced to switch due to whatever reason. In the best case, our CI scripts would allow a user to also easily run the tests locally via e.g. Docker. We're not there yet, but this patch series is a good step into that direction already.

Last but not least, I actually think that having multiple supported CI platforms also has the benefit that people can more readily set it up for themselves. In theory, this has the potential to broaden the set of people willing to contribute to our `ci/` scripts, which would in the end also benefit GitHub Actions.

In my opinion, this benefit is demonstrated by this patch series already: besides all the changes that aim to prepare for GitLab CI, there are also patches that deduplicate code and improve test coverage for Alpine Linux. These changes likely wouldn't have happened if it wasn't for the GitLab CI.

I understand your points, and especially the point about not having a second authoritative CI platform. I'm very much on the same page as you are here, and would be happy to move the definitions to "contrib/" if you want me to.

But I think we should also see the potential benefit of having a second CI platform, as it enables a more diverse set of people to contribute. which can ultimately end up benefitting our CI infra for both GitHub Actions and GitLab CI.

Patrick

On Tue, Oct 31, 2023 at 08:46:28AM +0100, Patrick Steinhardt wrote:

I think my concern with having it in Junio's tree at all is that it gives the impression that this is being maintained indefinitely. There is definitely some cruft in contrib that we should probably get rid of.

But since this is coming from an organization that sponsors work on the Git project, I think that my concerns there are somewhat more relaxed. I'm not worried about this going unmaintained, so I have no objection to having it live in contrib.

Yeah, I agree with everything you said here.

Agreed.

Agreed there as well.

Yeah, I appreciate your careful and patient explanation here. I think that my concerns here are resolved.

Thanks, Taylor

Patrick Steinhardt <ps@pks.im> writes:

Is it?

I rather naïvely think different set of build options and tools running the tests would mean we gain wider test coverage. Even with the current setup that relies on whatever GitHub offers, we already see "this version passes all tests except for the job on macOS" and "the version that was passing yesterday is not broken today---perhas the image of the test environment has been updated and we need to adjust to it" every once in a while.

I am not sure I follow. Often we take a version that happened to have failed Actions tests when we know the reason of the failure has nothing to do with the new code. From time to time people help to make CI tests less flakey, but flakes are expected.

I don't even make tests by GitHub Actions force me to do anything, so there is no worry here.

Let me step back and as more basic questions.

 - What do you mean by "authoritative"?  For an authoritative CI
   test, contributors whose changes do not pass it should take it as
   a sign that their changes need more work?  If so, isn't it a
   natural expectation and a good thing?  Unless you expect the CI
   tests to be extra flakey, that is.

 - Are there reasons why you do not trust the CI tests at GitLab
   more than those run at GitHub?

Yes, assuming that we can do so without much cutting and pasting but with a clear sharing of the infrastructure code, and the multiple supported CI environments are not too flakey, I am with this rather naïve worldview that the more we have the merrier we would be.

I do *not* want to add new things, if we were to use them ourselves, to "contrib/". We have passed that stage long time ago that keeping everything in my tree gives wider exposure and foster cooperation.

Thanks.

On Wed, Nov 01, 2023 at 09:15:51AM +0900, Junio C Hamano wrote:

Okay.

I was assuming that GitHub Actions was considered to be "the" CI platform of the Git project. But with your explanations above I think that assumption may not necessarily hold, or at least not to the extent I assumed.

No. Based on the above assumption I was simply treading carefully here. Most importantly, I didn't want to create the impression that either:

    - "Now you have to watch two pipelines", doubling the effort that CI
      infrastructure creates for you as a maintainer.

    - "I want to eventually replace GitHub Actions".

This carefulness probably also comes from the fact that GitLab and GitHub are direct competitors, so I was trying to preempt any kind of implied agenda here. There is none, I just want to make sure that it becomes easier for us at GitLab and other potential contributors that use GitLab to contribute to Git.

Hope that makes sense.

Fair enough.

Thanks for taking the time to make your thoughts clearer to me!

Patrick

Hi,

this is the fourth version of my patch series that introduces support for GitLab CI.

Changes compared to v3:

    - Stopped using nproc(1) to figure out the number of builds jobs for
      GitHub Actions and Azure Pipelines. Instead, we now continue to
      use the hardcoded 10 jobs there, whereas on GitLab CI we now use
      nproc. We can adapt GitHub/Azure at a later point as required, but
      I don't feel comfortable doing changes there.

    - Improved the linux-musl job. Namely, we now also install all
      required Apache modules, which makes the Apache-based test setup
      work. There is a packaging issue with the WebDAV module though, so
      we now skip tests that depend on it on Alpine Linux.

I still didn't move `.gitlab-ci.yml` to `contrib/`. As Taylor argued (and I don't disagree), moving it to `contrib/` would convey the spirit that this is _not_ an authoritative CI pipeline setup. But I wanted to hear other opinions first before moving it into `contrib/`.

Patrick

Patrick Steinhardt (8):
  ci: reorder definitions for grouping functions
  ci: make grouping setup more generic
  ci: group installation of Docker dependencies
  ci: split out logic to set up failed test artifacts
  ci: unify setup of some environment variables
  ci: squelch warnings when testing with unusable Git repo
  ci: install test dependencies for linux-musl
  ci: add support for GitLab CI

 .gitlab-ci.yml                    |  53 +++++++++
 ci/install-docker-dependencies.sh |  23 +++-
 ci/lib.sh                         | 190 +++++++++++++++++++++---------
 ci/print-test-failures.sh         |   6 +
 t/lib-httpd.sh                    |  17 ++-
 5 files changed, 233 insertions(+), 56 deletions(-)
 create mode 100644 .gitlab-ci.yml

Range-diff against v3:
1:  ef44ed5c3b1 = 1:  8595fe5016a ci: reorder definitions for grouping functions
2:  77798fa7a7a = 2:  7358a943392 ci: make grouping setup more generic
3:  4542bd38dc2 = 3:  6d842592c6f ci: group installation of Docker dependencies
4:  5fdda7fd83f = 4:  e15651b3f5d ci: split out logic to set up failed test artifacts
5:  6af0075fd87 ! 5:  a64799b6e25 ci: unify setup of some environment variables
    @@ Commit message
         parts are separated. While at it, we also perform some additional small
         improvements:
     
    -        - We use nproc instead of a hardcoded count of jobs for make and
    -          prove. This ensures that the number of concurrent processes adapts
    -          to the host automatically.
    -
             - We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
               It doesn't hurt on platforms where we don't persist the state, so
               this further reduces boilerplate.
    @@ ci/lib.sh: then
      	exit 1
      fi
      
    -+MAKEFLAGS="$MAKEFLAGS --jobs=$(nproc)"
    -+GIT_PROVE_OPTS="--timer --jobs $(nproc) --state=failed,slow,save"
    ++MAKEFLAGS="$MAKEFLAGS --jobs=10"
    ++GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
     +
     +GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
     +if test windows = "$CI_OS_NAME"
6:  78d863bf24e = 6:  f7d2a8666fe ci: squelch warnings when testing with unusable Git repo
7:  f150d61a1ce ! 7:  9b43b0d90e3 ci: install test dependencies for linux-musl
    @@ Commit message
               available, all Subversion-related tests are skipped without the
               SVN::Core Perl library anyway.
     
    -    Furthermore, in order to make the Apache-based tests work, this commit
    -    also adds the Alpine-specific modules path of it to the list of known
    -    paths.
    +    The Apache2-based tests require a bit more care though. For one, the
    +    module path is different on Alpine Linux, which requires us to add it to
    +    the list of known module paths to detect it. But second, the WebDAV
    +    module on Alpine Linux is broken because it does not bundle the default
    +    database backend [1]. We thus need to skip the WebDAV-based tests on
    +    Alpine Linux for now.
    +
    +    [1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ ci/install-docker-dependencies.sh: linux32)
      	apk add --update build-base curl-dev openssl-dev expat-dev gettext \
     -		pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
     +		pcre2-dev python3 musl-libintl perl-utils ncurses \
    -+		apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
    ++		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
    ++		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
      	;;
      pedantic)
      	dnf -yq update >/dev/null &&
    @@ t/lib-httpd.sh: for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
      do
      	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
      	then
    +@@ t/lib-httpd.sh: else
    + 		"Could not identify web server at '$LIB_HTTPD_PATH'"
    + fi
    + 
    ++if test -n "$LIB_HTTPD_DAV" && test -f /etc/os-release
    ++then
    ++	case "$(grep "^ID=" /etc/os-release | cut -d= -f2-)" in
    ++	alpine)
    ++		# The WebDAV module in Alpine Linux is broken at least up to
    ++		# Alpine v3.16 as the default DBM driver is missing.
    ++		#
    ++		# https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
    ++		test_skip_or_die GIT_TEST_HTTPD \
    ++			"Apache WebDAV module does not have default DBM backend driver"
    ++		;;
    ++	esac
    ++fi
    ++
    + install_script () {
    + 	write_script "$HTTPD_ROOT_PATH/$1" <"$TEST_PATH/$1"
    + }
8:  5272d66d9f1 ! 8:  f3f2c98a0dc ci: add support for GitLab CI
    @@ ci/install-docker-dependencies.sh: linux32)
     -	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 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
    + 		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 &&
    @@ ci/lib.sh: then
      else
      	begin_group () { :; }
      	end_group () { :; }
    +@@ ci/lib.sh: 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
     @@ ci/lib.sh: then
      	cache_dir="$HOME/none"
      
      	GIT_TEST_OPTS="--github-workflow-markup"
    ++	JOBS=10
     +elif test true = "$GITLAB_CI"
     +then
     +	CI_TYPE=gitlab-ci
    @@ ci/lib.sh: then
     +	cache_dir="$HOME/none"
     +
     +	runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
    ++	JOBS=$(nproc)
      else
      	echo "Could not identify CI type" >&2
      	env >&2
    + 	exit 1
    + fi
    + 
    +-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"
    + 
    + GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
    + if test windows = "$CI_OS_NAME"
     
      ## ci/print-test-failures.sh ##
     @@ ci/print-test-failures.sh: do

We define a set of grouping functions that are used to group together output in our CI, where these groups then end up as collapsible sections in the respective pipeline platform. The way these functions are defined is not easily extensible though as we have an up front check for the CI _not_ being GitHub Actions, where we define the non-stub logic in the else branch.

Reorder the conditional branches such that we explicitly handle GitHub Actions.

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

Make the grouping setup more generic by always calling `begin_group ()` and `end_group ()` regardless of whether we have stubbed those functions or not. This ensures we can more readily add support for additional CI platforms.

Furthermore, the `group ()` function is made generic so that it is the same for both GitHub Actions and for other platforms. There is a semantic conflict here though: GitHub Actions used to call `set +x` in `group ()` whereas the non-GitHub case unconditionally uses `set -x`. The latter would get overriden if we kept the `set +x` in the generic version of `group ()`. To resolve this conflict, we simply drop the `set +x` in the generic variant of this function. As `begin_group ()` calls `set -x` anyway this is not much of a change though, as the only commands that aren't printed anymore now are the ones between the beginning of `group ()` and the end of `begin_group ()`.

Last, this commit changes `end_group ()` to also accept a parameter that indicates _which_ group should end. This will be required by a later commit that introduces support for GitLab CI.

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

The output of CI jobs tends to be quite long-winded and hard to digest. To help with this, many CI systems provide the ability to group output into collapsible sections, and we're also doing this in some of our scripts.

One notable omission is the script to install Docker dependencies. Address it to bring more structure to the output for Docker-based jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh | 6 ++++++
 1 file changed, 6 insertions(+)

We have some logic in place to create a directory with the output from failed tests, which will then subsequently be uploaded as CI artifacts. We're about to add support for GitLab CI, which will want to reuse the logic.

Split the logic into a separate function so that it is reusable.

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

Both GitHub Actions and Azue Pipelines set up the environment variables GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are actually the same, the setup is completely duplicate. With the upcoming support for GitLab CI this duplication would only extend even further.

Unify the setup of those environment variables so that only the uncommon parts are separated. While at it, we also perform some additional small improvements:

    - We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
      It doesn't hurt on platforms where we don't persist the state, so
      this further reduces boilerplate.

    - When running on Windows systems we set `--no-chain-lint` and
      `--no-bin-wrappers`. Interestingly though, we did so _after_
      already having exported the respective environment variables.

    - We stop using `export VAR=value` syntax, which is a Bashism. It's
      not quite worth it as we still use this syntax all over the place,
      but it doesn't hurt readability either.

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

Patrick Steinhardt wrote:

s/Azue/Azure

Based on the deleted lines above, I think this would need to be:

	if test windows = "$CI_OS_NAME" || test windows_nt = "$CI_OS_NAME"

I believe these settings are required on all Windows builds, though, so you could instead match on the first 7 characters of $CI_OS_NAME:

	if test windows = "$(echo "$CI_OS_NAME" | cut -c1-7)"

(full disclosure: I'm not 100% confident in the correctness of that shell syntax)

Victoria Dye <vdye@github.com> writes:

Either

	case "$CI_OS_NAME" in
	windows*)
		GIT_TEST_OPTS=...
	esac

or if we want to be more selective, for documentation purposes especially when it is unlikely for us to gain the third variant of windows build:

	case ... in
	windows | windows_nt)
		GIT_TEST_OPTS=...
	esac

On Tue, Oct 31, 2023 at 10:06:24AM -0700, Victoria Dye wrote:

Oh, right. I didn't notice the slight difference between "windows" and "windows_nt". Thanks!

Patrick

Our CI jobs that run on Docker also use mostly the same architecture to build and test Git via the "ci/run-build-and-tests.sh" script. These scripts also provide some functionality to massage the Git repository we're supposedly operating in.

In our Docker-based infrastructure we may not even have a Git repository available though, which leads to warnings when those functions execute. Make the helpers exit gracefully in case either there is no Git in our PATH, or when not running in a Git repository.

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

The linux-musl CI job executes tests on Alpine Linux, which is based on musl libc instead of glibc. We're missing some test dependencies though, which causes us to skip a subset of tests.

Install these test dependencies to increase our test coverage on this platform. There are still some missing test dependecies, but these do not have a corresponding package in the Alpine repositories:

    - p4 and p4d, both parts of the Perforce version control system.

    - cvsps, which generates patch sets for CVS.

    - Subversion and the SVN::Core Perl library, the latter of which is
      not available in the Alpine repositories. While the tool itself is
      available, all Subversion-related tests are skipped without the
      SVN::Core Perl library anyway.

The Apache2-based tests require a bit more care though. For one, the module path is different on Alpine Linux, which requires us to add it to the list of known module paths to detect it. But second, the WebDAV module on Alpine Linux is broken because it does not bundle the default database backend [1]. We thus need to skip the WebDAV-based tests on Alpine Linux for now.

[1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh |  4 +++-
 t/lib-httpd.sh                    | 17 ++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

We already support Azure Pipelines and GitHub Workflows in the Git project, but until now we do not have support for GitLab CI. While it is arguably not in the interest of the Git project to maintain a ton of different CI platforms, GitLab has recently ramped up its efforts and tries to contribute to the Git project more regularly.

Part of a problem we hit at GitLab rather frequently is that our own, custom CI setup we have is so different to the setup that the Git project has. More esoteric jobs like "linux-TEST-vars" that also set a 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 fail to compile or pass tests in GitHub Workflows. We would thus like to integrate the GitLab CI configuration into the Git project to help us send better patch series upstream and thus reduce overhead for the maintainer.

The integration does not necessarily have to be a first-class citizen, which would in practice only add to the fallout that pipeline failures have for the maintainer. That being said, we are happy to maintain this alternative CI setup for the Git project and will make test results available as part of our own mirror of the Git project at [1].

This commit introduces the integration into our regular CI scripts so that most of the setup continues to be shared across all of the CI solutions. Note that as the builds on GitLab CI run as unprivileged user, we need to pull in both sudo and shadow packages to our Alpine based job to set this up.

[1]: https://gitlab.com/gitlab-org/git

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml                    | 53 +++++++++++++++++++++++++++++++
 ci/install-docker-dependencies.sh | 13 +++++++-
 ci/lib.sh                         | 50 +++++++++++++++++++++++++++--
 ci/print-test-failures.sh         |  6 ++++
 4 files changed, 119 insertions(+), 3 deletions(-)
 create mode 100644 .gitlab-ci.yml

Patrick Steinhardt wrote:> diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh

...

...

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.

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.

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

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

Patrick Steinhardt wrote:

I've read through some of the earlier discussion on this (as well as your original cover letter [1]), so I'll throw in my 2c.

The majority of the changes in this patch series aren't conditioned on anything that says "gitlab", they just improve the flexibility of our CI scripts. I personally didn't notice anything too cumbersome added in the series, so I'm happy with all of that (essentially, patches 1-7 & parts of 8).

As for adding the GitLab-specific stuff, I'm not opposed to having it in the main tree. For one, there doesn't seem to be a clean way to "move it into `contrib/`" - '.gitlab-ci.yml' must be at the root of the project [2], and moving the $GITLAB_CI conditions out of the 'ci/*.sh' files into dedicated scripts would likely result in a lot of duplicated code (which doesn't solve the maintenance burden issue this series intends to address).

More generally, there are lots of open source projects that include CI configurations across different forges, _especially_ those that are officially mirrored across a bunch of them. As long as there are contributors with a vested interest in keeping the GitLab CI definition stable (and your cover letter indicates that there are), and the GitLab stuff doesn't negatively impact any other CI configurations, I think it warrants the same treatment as e.g. GitHub CI.

[1] https://lore.kernel.org/git/cover.1698305961.git.ps@pks.im/ [2] https://docs.gitlab.com/ee/ci/index.html#the-gitlab-ciyml-file

Victoria Dye <vdye@github.com> writes:

Thanks for expressing this so clearly. I do prefer to add this as the first class citizen (more generally, I do not want to add new things to contrib/ at this point) if we are going to use it.

On Wed, Nov 01, 2023 at 12:22:56PM +0900, Junio C Hamano wrote:

It is possible to change the location of the `.gitlab-ci.yml` in GitLab projects, so moving it into `contrib/` would work, too. But it does of course require additional setup by the project admin.

I certainly know that we in the Gitaly team will use this CI definition on a daily basis, which is my main motivation to add and maintain it in the future. I personally don't mind to add this as a first-class citizen, because for my own workflows I'll certainly treat it as such. And if others can benefit from it, too, then I'm even happier.

I'll keep it in the default location at `.gitlab-ci.yml` then, thanks for your thoughts!

Patrick

Hi,

this is the fifth version of my patch series that introduces support for GitLab CI.

There are only minor changes compared to v4:

    - Patch 5: Fixed a typo in the commit message. Furthermore, I've
      dropped the note about `export VAR=value` being a Bashism.

    - Patch 5: Fixed the accidentally-dropped "windows_nt" case.

    - Patch 5: Introduce the JOBS variable right away so that there is
      less churn in the patch that introduces GitLab CI.

    - Patch 8: Moved `DEBIAN_FRONTEND` into the "linux-*" case to make
      clear that it shouldn't impact anything else.

    - Patch 8: Dropped the paragraph about GitLab CI not being a first
      class citizen based on Junio's feedback. This setup is going to be
      actively used (at least) by us at GitLab, and thus we're also
      going to maintain it.

After both Junio's and Victoria's feedback I've kept `.gitlab-ci.yml` in its canonical location.

The pipeline for this version can be found at [1].

Thanks for all the discussion and feedback so far!

Patrick

[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1057566736

Patrick Steinhardt (8):
  ci: reorder definitions for grouping functions
  ci: make grouping setup more generic
  ci: group installation of Docker dependencies
  ci: split out logic to set up failed test artifacts
  ci: unify setup of some environment variables
  ci: squelch warnings when testing with unusable Git repo
  ci: install test dependencies for linux-musl
  ci: add support for GitLab CI

 .gitlab-ci.yml                    |  53 +++++++++
 ci/install-docker-dependencies.sh |  23 +++-
 ci/lib.sh                         | 191 +++++++++++++++++++++---------
 ci/print-test-failures.sh         |   6 +
 t/lib-httpd.sh                    |  17 ++-
 5 files changed, 234 insertions(+), 56 deletions(-)
 create mode 100644 .gitlab-ci.yml

Range-diff against v4:
1:  8595fe5016a = 1:  0ba396f2a33 ci: reorder definitions for grouping functions
2:  7358a943392 = 2:  821cfcd6125 ci: make grouping setup more generic
3:  6d842592c6f = 3:  6e5bcf143c8 ci: group installation of Docker dependencies
4:  e15651b3f5d = 4:  2182acf5bfc ci: split out logic to set up failed test artifacts
5:  a64799b6e25 ! 5:  6078aea246d ci: unify setup of some environment variables
    @@ Metadata
      ## Commit message ##
         ci: unify setup of some environment variables
     
    -    Both GitHub Actions and Azue Pipelines set up the environment variables
    +    Both GitHub Actions and Azure Pipelines set up the environment variables
         GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
         actually the same, the setup is completely duplicate. With the upcoming
         support for GitLab CI this duplication would only extend even further.
    @@ Commit message
               `--no-bin-wrappers`. Interestingly though, we did so _after_
               already having exported the respective environment variables.
     
    -        - We stop using `export VAR=value` syntax, which is a Bashism. It's
    -          not quite worth it as we still use this syntax all over the place,
    -          but it doesn't hurt readability either.
    -
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## ci/lib.sh ##
    @@ ci/lib.sh: then
     -	test windows_nt != "$CI_OS_NAME" ||
     -	GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
     +	GIT_TEST_OPTS="--write-junit-xml"
    ++	JOBS=10
      elif test true = "$GITHUB_ACTIONS"
      then
      	CI_TYPE=github-actions
    @@ ci/lib.sh: then
     -	test windows != "$CI_OS_NAME" ||
     -	GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
     +	GIT_TEST_OPTS="--github-workflow-markup"
    ++	JOBS=10
      else
      	echo "Could not identify CI type" >&2
      	env >&2
      	exit 1
      fi
      
    -+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"
     +
     +GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
    -+if test windows = "$CI_OS_NAME"
    -+then
    ++case "$CI_OS_NAME" in
    ++windows|windows_nt)
     +	GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
    -+fi
    ++	;;
    ++esac
     +
     +export GIT_TEST_OPTS
     +export GIT_PROVE_OPTS
6:  f7d2a8666fe = 6:  d69bde92f2f ci: squelch warnings when testing with unusable Git repo
7:  9b43b0d90e3 = 7:  b911c005bae ci: install test dependencies for linux-musl
8:  f3f2c98a0dc ! 8:  5784d03a6f1 ci: add support for GitLab CI
    @@ Commit message
         that fail to compile or pass tests in GitHub Workflows. We would thus
         like to integrate the GitLab CI configuration into the Git project to
         help us send better patch series upstream and thus reduce overhead for
    -    the maintainer.
    -
    -    The integration does not necessarily have to be a first-class citizen,
    -    which would in practice only add to the fallout that pipeline failures
    -    have for the maintainer. That being said, we are happy to maintain this
    -    alternative CI setup for the Git project and will make test results
    -    available as part of our own mirror of the Git project at [1].
    +    the maintainer. Results of these pipeline runs will be made available
    +    (at least) in GitLab's mirror of the Git project at [1].
     
         This commit introduces the integration into our regular CI scripts so
         that most of the setup continues to be shared across all of the CI
    @@ .gitlab-ci.yml (new)
     +    when: on_failure
     
      ## ci/install-docker-dependencies.sh ##
    -@@
    - 
    - 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 '
     @@ ci/install-docker-dependencies.sh: linux32)
      	'
      	;;
    @@ ci/install-docker-dependencies.sh: linux32)
      		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
      	;;
     +linux-*)
    ++	# Required so that apt doesn't wait for user input on certain packages.
    ++	export DEBIAN_FRONTEND=noninteractive
    ++
     +	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 \
    @@ ci/lib.sh: then
      	begin_group () { :; }
      	end_group () { :; }
     @@ ci/lib.sh: 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
    -@@ ci/lib.sh: then
    - 	cache_dir="$HOME/none"
      
      	GIT_TEST_OPTS="--github-workflow-markup"
    -+	JOBS=10
    + 	JOBS=10
     +elif test true = "$GITLAB_CI"
     +then
     +	CI_TYPE=gitlab-ci
    @@ ci/lib.sh: then
      else
      	echo "Could not identify CI type" >&2
      	env >&2
    - 	exit 1
    - fi
    - 
    --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"
    - 
    - GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
    - if test windows = "$CI_OS_NAME"
     
      ## ci/print-test-failures.sh ##
     @@ ci/print-test-failures.sh: do

base-commit: 692be87cbba55e8488f805d236f2ad50483bd7d5

We define a set of grouping functions that are used to group together output in our CI, where these groups then end up as collapsible sections in the respective pipeline platform. The way these functions are defined is not easily extensible though as we have an up front check for the CI _not_ being GitHub Actions, where we define the non-stub logic in the else branch.

Reorder the conditional branches such that we explicitly handle GitHub Actions.

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

Make the grouping setup more generic by always calling `begin_group ()` and `end_group ()` regardless of whether we have stubbed those functions or not. This ensures we can more readily add support for additional CI platforms.

Furthermore, the `group ()` function is made generic so that it is the same for both GitHub Actions and for other platforms. There is a semantic conflict here though: GitHub Actions used to call `set +x` in `group ()` whereas the non-GitHub case unconditionally uses `set -x`. The latter would get overriden if we kept the `set +x` in the generic version of `group ()`. To resolve this conflict, we simply drop the `set +x` in the generic variant of this function. As `begin_group ()` calls `set -x` anyway this is not much of a change though, as the only commands that aren't printed anymore now are the ones between the beginning of `group ()` and the end of `begin_group ()`.

Last, this commit changes `end_group ()` to also accept a parameter that indicates _which_ group should end. This will be required by a later commit that introduces support for GitLab CI.

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

The output of CI jobs tends to be quite long-winded and hard to digest. To help with this, many CI systems provide the ability to group output into collapsible sections, and we're also doing this in some of our scripts.

One notable omission is the script to install Docker dependencies. Address it to bring more structure to the output for Docker-based jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh | 6 ++++++
 1 file changed, 6 insertions(+)

We have some logic in place to create a directory with the output from failed tests, which will then subsequently be uploaded as CI artifacts. We're about to add support for GitLab CI, which will want to reuse the logic.

Split the logic into a separate function so that it is reusable.

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

Both GitHub Actions and Azure Pipelines set up the environment variables GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are actually the same, the setup is completely duplicate. With the upcoming support for GitLab CI this duplication would only extend even further.

Unify the setup of those environment variables so that only the uncommon parts are separated. While at it, we also perform some additional small improvements:

    - We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
      It doesn't hurt on platforms where we don't persist the state, so
      this further reduces boilerplate.

    - When running on Windows systems we set `--no-chain-lint` and
      `--no-bin-wrappers`. Interestingly though, we did so _after_
      already having exported the respective environment variables.

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

Our CI jobs that run on Docker also use mostly the same architecture to build and test Git via the "ci/run-build-and-tests.sh" script. These scripts also provide some functionality to massage the Git repository we're supposedly operating in.

In our Docker-based infrastructure we may not even have a Git repository available though, which leads to warnings when those functions execute. Make the helpers exit gracefully in case either there is no Git in our PATH, or when not running in a Git repository.

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

The linux-musl CI job executes tests on Alpine Linux, which is based on musl libc instead of glibc. We're missing some test dependencies though, which causes us to skip a subset of tests.

Install these test dependencies to increase our test coverage on this platform. There are still some missing test dependecies, but these do not have a corresponding package in the Alpine repositories:

    - p4 and p4d, both parts of the Perforce version control system.

    - cvsps, which generates patch sets for CVS.

    - Subversion and the SVN::Core Perl library, the latter of which is
      not available in the Alpine repositories. While the tool itself is
      available, all Subversion-related tests are skipped without the
      SVN::Core Perl library anyway.

The Apache2-based tests require a bit more care though. For one, the module path is different on Alpine Linux, which requires us to add it to the list of known module paths to detect it. But second, the WebDAV module on Alpine Linux is broken because it does not bundle the default database backend [1]. We thus need to skip the WebDAV-based tests on Alpine Linux for now.

[1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh |  4 +++-
 t/lib-httpd.sh                    | 17 ++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

We already support Azure Pipelines and GitHub Workflows in the Git project, but until now we do not have support for GitLab CI. While it is arguably not in the interest of the Git project to maintain a ton of different CI platforms, GitLab has recently ramped up its efforts and tries to contribute to the Git project more regularly.

Part of a problem we hit at GitLab rather frequently is that our own, custom CI setup we have is so different to the setup that the Git project has. More esoteric jobs like "linux-TEST-vars" that also set a 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 fail to compile or pass tests in GitHub Workflows. We would thus like to integrate the GitLab CI configuration into the Git project to help us send better patch series upstream and thus reduce overhead for the maintainer. Results of these pipeline runs will be made available (at least) in GitLab's mirror of the Git project at [1].

This commit introduces the integration into our regular CI scripts so that most of the setup continues to be shared across all of the CI solutions. Note that as the builds on GitLab CI run as unprivileged user, we need to pull in both sudo and shadow packages to our Alpine based job to set this up.

[1]: https://gitlab.com/gitlab-org/git

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml                    | 53 +++++++++++++++++++++++++++++++
 ci/install-docker-dependencies.sh | 13 +++++++-
 ci/lib.sh                         | 44 +++++++++++++++++++++++++
 ci/print-test-failures.sh         |  6 ++++
 4 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 .gitlab-ci.yml

Hi,

this is the 6th version of my patch series to introduce support for GitLab CI into the Git project.

There's only a single change compared to v5 based on Chris' feedback, namely to move around a `return 1`. The newly extracted helper function `create_failed_test_artifacts()` indeed wasn't the correct place to put this error code.

A test run of this pipeline can be found at [1].

Thanks!

Patrick

[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1066250852

Patrick Steinhardt (8):
  ci: reorder definitions for grouping functions
  ci: make grouping setup more generic
  ci: group installation of Docker dependencies
  ci: split out logic to set up failed test artifacts
  ci: unify setup of some environment variables
  ci: squelch warnings when testing with unusable Git repo
  ci: install test dependencies for linux-musl
  ci: add support for GitLab CI

 .gitlab-ci.yml                    |  53 +++++++++
 ci/install-docker-dependencies.sh |  23 +++-
 ci/lib.sh                         | 190 ++++++++++++++++++++++--------
 ci/print-test-failures.sh         |   6 +
 t/lib-httpd.sh                    |  17 ++-
 5 files changed, 234 insertions(+), 55 deletions(-)
 create mode 100644 .gitlab-ci.yml

Range-diff against v5:
1:  0ba396f2a33 = 1:  a1413b76422 ci: reorder definitions for grouping functions
2:  821cfcd6125 = 2:  29039d7aa3a ci: make grouping setup more generic
3:  6e5bcf143c8 = 3:  414655ffb2d ci: group installation of Docker dependencies
4:  2182acf5bfc ! 4:  96d710faec8 ci: split out logic to set up failed test artifacts
    @@ ci/lib.sh: handle_failed_tests () {
     +		cp "t/test-results/$test_name.out" t/failed-test-artifacts/
     +		tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
     +	done
    -+	return 1
     +}
     +
      # GitHub Action doesn't set TERM, which is required by tput
    @@ ci/lib.sh: then
     -			cp "t/test-results/$test_name.out" t/failed-test-artifacts/
     -			tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
     -		done
    --		return 1
     +		create_failed_test_artifacts
    + 		return 1
      	}
      
    - 	cache_dir="$HOME/none"
5:  6078aea246d = 5:  486d4bbf8b0 ci: unify setup of some environment variables
6:  d69bde92f2f = 6:  534b14f0262 ci: squelch warnings when testing with unusable Git repo
7:  b911c005bae = 7:  a060613f039 ci: install test dependencies for linux-musl
8:  5784d03a6f1 ! 8:  c05ff28cc2c ci: add support for GitLab CI
    @@ ci/lib.sh: then
     +	DONT_SKIP_TAGS=t
     +	handle_failed_tests () {
     +		create_failed_test_artifacts
    ++		return 1
     +	}
     +
     +	cache_dir="$HOME/none"

base-commit: dadef801b365989099a9929e995589e455c51fed

We define a set of grouping functions that are used to group together output in our CI, where these groups then end up as collapsible sections in the respective pipeline platform. The way these functions are defined is not easily extensible though as we have an up front check for the CI _not_ being GitHub Actions, where we define the non-stub logic in the else branch.

Reorder the conditional branches such that we explicitly handle GitHub Actions.

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

Make the grouping setup more generic by always calling `begin_group ()` and `end_group ()` regardless of whether we have stubbed those functions or not. This ensures we can more readily add support for additional CI platforms.

Furthermore, the `group ()` function is made generic so that it is the same for both GitHub Actions and for other platforms. There is a semantic conflict here though: GitHub Actions used to call `set +x` in `group ()` whereas the non-GitHub case unconditionally uses `set -x`. The latter would get overriden if we kept the `set +x` in the generic version of `group ()`. To resolve this conflict, we simply drop the `set +x` in the generic variant of this function. As `begin_group ()` calls `set -x` anyway this is not much of a change though, as the only commands that aren't printed anymore now are the ones between the beginning of `group ()` and the end of `begin_group ()`.

Last, this commit changes `end_group ()` to also accept a parameter that indicates _which_ group should end. This will be required by a later commit that introduces support for GitLab CI.

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

The output of CI jobs tends to be quite long-winded and hard to digest. To help with this, many CI systems provide the ability to group output into collapsible sections, and we're also doing this in some of our scripts.

One notable omission is the script to install Docker dependencies. Address it to bring more structure to the output for Docker-based jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh | 6 ++++++
 1 file changed, 6 insertions(+)

We have some logic in place to create a directory with the output from failed tests, which will then subsequently be uploaded as CI artifacts. We're about to add support for GitLab CI, which will want to reuse the logic.

Split the logic into a separate function so that it is reusable.

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

Both GitHub Actions and Azure Pipelines set up the environment variables GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are actually the same, the setup is completely duplicate. With the upcoming support for GitLab CI this duplication would only extend even further.

Unify the setup of those environment variables so that only the uncommon parts are separated. While at it, we also perform some additional small improvements:

    - We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
      It doesn't hurt on platforms where we don't persist the state, so
      this further reduces boilerplate.

    - When running on Windows systems we set `--no-chain-lint` and
      `--no-bin-wrappers`. Interestingly though, we did so _after_
      already having exported the respective environment variables.

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

Our CI jobs that run on Docker also use mostly the same architecture to build and test Git via the "ci/run-build-and-tests.sh" script. These scripts also provide some functionality to massage the Git repository we're supposedly operating in.

In our Docker-based infrastructure we may not even have a Git repository available though, which leads to warnings when those functions execute. Make the helpers exit gracefully in case either there is no Git in our PATH, or when not running in a Git repository.

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

The linux-musl CI job executes tests on Alpine Linux, which is based on musl libc instead of glibc. We're missing some test dependencies though, which causes us to skip a subset of tests.

Install these test dependencies to increase our test coverage on this platform. There are still some missing test dependecies, but these do not have a corresponding package in the Alpine repositories:

    - p4 and p4d, both parts of the Perforce version control system.

    - cvsps, which generates patch sets for CVS.

    - Subversion and the SVN::Core Perl library, the latter of which is
      not available in the Alpine repositories. While the tool itself is
      available, all Subversion-related tests are skipped without the
      SVN::Core Perl library anyway.

The Apache2-based tests require a bit more care though. For one, the module path is different on Alpine Linux, which requires us to add it to the list of known module paths to detect it. But second, the WebDAV module on Alpine Linux is broken because it does not bundle the default database backend [1]. We thus need to skip the WebDAV-based tests on Alpine Linux for now.

[1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-docker-dependencies.sh |  4 +++-
 t/lib-httpd.sh                    | 17 ++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

We already support Azure Pipelines and GitHub Workflows in the Git project, but until now we do not have support for GitLab CI. While it is arguably not in the interest of the Git project to maintain a ton of different CI platforms, GitLab has recently ramped up its efforts and tries to contribute to the Git project more regularly.

Part of a problem we hit at GitLab rather frequently is that our own, custom CI setup we have is so different to the setup that the Git project has. More esoteric jobs like "linux-TEST-vars" that also set a 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 fail to compile or pass tests in GitHub Workflows. We would thus like to integrate the GitLab CI configuration into the Git project to help us send better patch series upstream and thus reduce overhead for the maintainer. Results of these pipeline runs will be made available (at least) in GitLab's mirror of the Git project at [1].

This commit introduces the integration into our regular CI scripts so that most of the setup continues to be shared across all of the CI solutions. Note that as the builds on GitLab CI run as unprivileged user, we need to pull in both sudo and shadow packages to our Alpine based job to set this up.

[1]: https://gitlab.com/gitlab-org/git

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml                    | 53 +++++++++++++++++++++++++++++++
 ci/install-docker-dependencies.sh | 13 +++++++-
 ci/lib.sh                         | 45 ++++++++++++++++++++++++++
 ci/print-test-failures.sh         |  6 ++++
 4 files changed, 116 insertions(+), 1 deletion(-)
 create mode 100644 .gitlab-ci.yml

Patrick Steinhardt <ps@pks.im> writes:

The caller of the helper always signals failure by returning with 1 after calling this helper, and if we later add new callers, it is very plausible that the new callers also are calling the helper because they have noticed that the tests failed.

If the helper were not named "create_failed_test_artifacts" but were given a more appropriate name, say, "fail_with_test_artifacts", we wouldn't have said that the helper is not the right place to return with a failure status. So, quite honestly, the difference since the previous round is strictly a Meh to me.

But let's queue this version, because it is not making it worse ;-)

Thanks.