* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Jeff King @ 2023-10-30 9:13 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <ZTjedSluwyrVY+L9@ugly>
On Wed, Oct 25, 2023 at 11:23:01AM +0200, Oswald Buddenhagen wrote:
> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
> > The "//" operator was added in perl 5.10. I'm not sure what you found
> > that makes you think the ship has sailed. The only hits for "//" I see
> > look like the end of substitution regexes ("s/foo//" and similar).
> >
> grep with spaces around the operator, then you can see the instance in
> git-credential-netrc.perl easily.
Ah, yeah, there is one instance there. That script does not have a "use"
marker, though, and we do not necessarily need or want to be as strict
with contrib/ scripts, which are quite optional compared to core
functionality like send-email.
That said, I do suspect that requiring 5.10 or later would not be too
burdensome these days. If we want to do so, then the first step would be
updating the text in INSTALL, along with the "use" directives in most
files. Probably d48b284183 (perl: bump the required Perl version to 5.8
from 5.6.[21], 2010-09-24) could serve as a template.
-Peff
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Jeff King @ 2023-10-30 9:29 UTC (permalink / raw)
To: Todd Zullinger
Cc: Michael Strawbridge, Junio C Hamano, Uwe Kleine-König,
Luben Tuikov, git, entwicklung
In-Reply-To: <ZTp7xvXDw1GF-NUB@pobox.com>
On Thu, Oct 26, 2023 at 10:46:30AM -0400, Todd Zullinger wrote:
> I'd lean toward dropping the dependency entirely and leave
> the more basic validation of git-send-email in place. That
> may not catch every type of address error, but I would argue
> that what we do without Email::Valid is perfectly reasonable
> for checking basic email address syntax sanity.
I am somewhat tempted to say the same, but in this case didn't it help
us find a real bug? True, the bug was that we were feeding garbage to
Email::Valid because we were calling it to early, and I _think_ the
ultimate emails we sent would have been OK. But I think we were possibly
feeding that garbage to the user-visible validation hook.
That might just mean we need to beef up our homegrown validation a bit
(though as you indicate here, I am generally of the opinion that the
best validation is trying to actually send things).
-Peff
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Michal Suchánek @ 2023-10-30 9:30 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030090626.GA84866@coredump.intra.peff.net>
On Mon, Oct 30, 2023 at 05:06:26AM -0400, Jeff King wrote:
> On Mon, Oct 30, 2023 at 09:52:05AM +0100, Michal Suchánek wrote:
>
> > > If we look very carfully at the log, we see that all the messages prefixed
> > > with "remote:" come from the remote (git server).
> > > In your case github. Other "Git repo servers" like gitlab or bitbucket have
> > > the same feature.
> > >
> > > Git itself, running on your local computer, does not produce this links.
> > > If you really want to push directly to the main branch and want to see the
> > > a link, then you better talk to the gihub folks.
> >
> > On the other hand, option to NOT display those remote messages is also
> > missing. At least with git 2.35 they are displayed even when -q argument
> > is given.
>
> That is also up to GitHub to fix on the server side:
>
> https://lore.kernel.org/git/20230519090559.GA3515410@coredump.intra.peff.net/
If the server did not reject the push the messages it returns could be
considered informational.
Not sure if the final status of the push is available or of the
rejection is only noted in these messages, though.
Thanks
Michal
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #09; Mon, 30)
From: Robert Coup @ 2023-10-30 9:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1qddhtpi.fsf@gitster.g>
Hi Junio,
Any thoughts on picking up "upload-pack: add tracing for fetches" [1]?
It received positive feedback from Taylor and Peff, and I submitted a
v2 [2] addressing the comments that Peff has re-reviewed. Any further
comments are most welcome.
Thanks,
Rob :)
[1] https://lore.kernel.org/git/pull.1598.git.1697040242703.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/pull.1598.v2.git.1697577168128.gitgitgadget@gmail.com/
^ permalink raw reply
* Re: Repository cloned using SSH does not respect bare repository initial branch
From: Jeff King @ 2023-10-30 9:36 UTC (permalink / raw)
To: Sheik; +Cc: git
In-Reply-To: <63eb269e-72b9-4830-98fc-aeef8b8180d7@gmail.com>
On Thu, Oct 26, 2023 at 07:36:36AM +1100, Sheik wrote:
> Repository cloned using SSH does not use the branch configured in the bare
> repository however repository cloned using filesystem does as expected.
> Shouldn't they both behave the same?
What version of Git is running on the ssh server?
Your example seems to show that the parent repository has an unborn
branch (i.e., HEAD points to "refs/heads/test", but there are no commits
yet). I think the server-side bits you need for that to work showed up
in 59e1205d16 (ls-refs: report unborn targets of symrefs, 2021-02-05),
which is in v2.31.
So even though your client seems to be v2.42 (from the output you gave),
if the server is older it may not be sending sufficient information.
There were also some other fixes on top of that, but I _think_ they were
all client-side (so your v2.42 clone command should be doing the right
thing).
-Peff
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Jeff King @ 2023-10-30 9:39 UTC (permalink / raw)
To: Michal Suchánek; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030093023.GG6241@kitsune.suse.cz>
On Mon, Oct 30, 2023 at 10:30:23AM +0100, Michal Suchánek wrote:
> > > On the other hand, option to NOT display those remote messages is also
> > > missing. At least with git 2.35 they are displayed even when -q argument
> > > is given.
> >
> > That is also up to GitHub to fix on the server side:
> >
> > https://lore.kernel.org/git/20230519090559.GA3515410@coredump.intra.peff.net/
>
> If the server did not reject the push the messages it returns could be
> considered informational.
Hmm, yeah, I could perhaps buy that line of argument. Though it's
possible they are "warnings" of the sort that should still be emitted
with "--quiet"; really only the server knows. And note that even "reject
the push" is not necessarily all-or-nothing. The server may accept some
subset of the refs.
But...
> Not sure if the final status of the push is available or of the
> rejection is only noted in these messages, though.
Yes, this is a problem. Those messages are streamed out as we receive
them from the server, and before we get any status report back. I don't
think we'd want to buffer them, as they can be arbitrarily large (and
may even be progress reports that are meant to be shown in real-time).
-Peff
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Michal Suchánek @ 2023-10-30 9:45 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030093919.GF84866@coredump.intra.peff.net>
On Mon, Oct 30, 2023 at 05:39:19AM -0400, Jeff King wrote:
> On Mon, Oct 30, 2023 at 10:30:23AM +0100, Michal Suchánek wrote:
>
> > > > On the other hand, option to NOT display those remote messages is also
> > > > missing. At least with git 2.35 they are displayed even when -q argument
> > > > is given.
> > >
> > > That is also up to GitHub to fix on the server side:
> > >
> > > https://lore.kernel.org/git/20230519090559.GA3515410@coredump.intra.peff.net/
> >
> > If the server did not reject the push the messages it returns could be
> > considered informational.
>
> Hmm, yeah, I could perhaps buy that line of argument. Though it's
> possible they are "warnings" of the sort that should still be emitted
> with "--quiet"; really only the server knows. And note that even "reject
> the push" is not necessarily all-or-nothing. The server may accept some
> subset of the refs.
>
> But...
>
> > Not sure if the final status of the push is available or of the
> > rejection is only noted in these messages, though.
>
> Yes, this is a problem. Those messages are streamed out as we receive
> them from the server, and before we get any status report back. I don't
> think we'd want to buffer them, as they can be arbitrarily large (and
> may even be progress reports that are meant to be shown in real-time).
Not when the user asked to not show progress.
Thanks
Michal
^ permalink raw reply
* Re: [PATCH 5/5] ci: add support for GitLab CI
From: Phillip Wood @ 2023-10-30 9:49 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Patrick Steinhardt, git
In-Reply-To: <ZTv3to98QT8Ma+ag@ugly>
On 27/10/2023 18:47, Oswald Buddenhagen wrote:
> On Fri, Oct 27, 2023 at 03:32:48PM +0100, Phillip Wood wrote:
>> On 27/10/2023 11:43, Oswald Buddenhagen wrote:
>>> On Fri, Oct 27, 2023 at 11:22:35AM +0100, Phillip Wood wrote:
>>>>>>> + CI_BRANCH="$CI_COMMIT_REF_NAME"
>>>>>>> + CI_COMMIT="$CI_COMMIT_SHA"
>>>>>>>
>>>>>> assignments need no quoting to prevent word splitting.
>>>>>> repeats below.
>>>>>>
>>>> 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.
>>>>
>>> the point was that it *isn't* content-dependent; it's simply the
>>> shell rules.
>>
>> Oh, I'd misunderstood what you were saying which was that assignment
>> and case statements are not subject to field splitting.
>>
>>> of course, many people (apparently you included) don't know these
>>> subtleties
>>
>> I find this comment to be condescending, needlessly antagonistic and
>> completely at odds with the norms of constructive discussion on this
>> list.
>>
> the observation was necessary for the point i subsequently made (which
> was basically agreeing with the first part of your response).
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/
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Jeff King @ 2023-10-30 9:54 UTC (permalink / raw)
To: Michal Suchánek; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030094556.GI6241@kitsune.suse.cz>
On Mon, Oct 30, 2023 at 10:45:56AM +0100, Michal Suchánek wrote:
> > Yes, this is a problem. Those messages are streamed out as we receive
> > them from the server, and before we get any status report back. I don't
> > think we'd want to buffer them, as they can be arbitrarily large (and
> > may even be progress reports that are meant to be shown in real-time).
>
> Not when the user asked to not show progress.
Sure, if you make the buffering behavior dependent on the presence of
"-q". I guess that is not that hard to do, but it does increase the
complexity of the code (you have both a streaming and a buffering code
path).
Anyway, I am not all that convinced this is a fruitful path versus just
fixing the server side. But if you want to look into writing a patch, go
for it.
-Peff
^ permalink raw reply
* Re: [PATCH v3] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Phillip Wood @ 2023-10-30 9:55 UTC (permalink / raw)
To: Marc Branchaud, Oswald Buddenhagen, git
Cc: Junio C Hamano, Taylor Blau, Christian Couder, Charvi Mendiratta
In-Reply-To: <56e3e974-a027-439f-871d-c7fbae65a04e@xiplink.com>
On 27/10/2023 14:14, Marc Branchaud wrote:
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index e7b39ad244..578d1d34a6 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -890,20 +890,22 @@ command "pick" with the command "reword".
>> To drop a commit, replace the command "pick" with "drop", or just
>> delete the matching line.
>> -If you want to fold two or more commits into one, replace the command
>> -"pick" for the second and subsequent commits with "squash" or "fixup".
>> -If the commits had different authors, the folded commit will be
>> -attributed to the author of the first commit. The suggested commit
>> -message for the folded commit is the concatenation of the first
>> -commit's message with those identified by "squash" commands, omitting
>> the
>> -messages of commits identified by "fixup" commands, unless "fixup -c"
>> -is used. In that case the suggested commit message is only the message
>> -of the "fixup -c" commit, and an editor is opened allowing you to edit
>> -the message. The contents (patch) of the "fixup -c" commit are still
>> -incorporated into the folded commit. If there is more than one "fixup
>> -c"
>> -commit, the message from the final one is used. You can also use
>> -"fixup -C" to get the same behavior as "fixup -c" except without opening
>> -an editor.
>> +If you want to fold two or more commits into one (that is, to combine
>> +their contents/patches), replace the command "pick" for the second and
>> +subsequent commits with "squash" or "fixup".
>> +The commit message for the folded commit is the concatenation of the
>> +message of the first commit with those of commits identified by "squash"
>> +commands, omitting those of commits identified by "fixup" commands,
>> +unless "fixup -c" is used. In the latter case, the message is obtained
>> +only from the "fixup -c" commit (if multiple are present, the last one
>> +takes precedence, but this should not be relied upon).
>
> I like the overall phrasing here.
>
> But I think you should remove the "but this should not be relied upon"
> phrase. This reads as if Git's current behaviour is undefined, which
> most definitely is not true.
I agree it would be better to remove that phrase, as you say it makes it
sounds like the behaviour cannot be relied on.
> Even changing this to something like "but this might change in the
> future" is unhelpful. Everything in Git is subject to change over a
> long-enough time span, so the same could be said about every aspect of Git.
>
> Until the behaviour actually changes, it's perfectly fine for people to
> use multiple "fixup -c" commands. There's no reason to scare them off
> of it.
Indeed
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 0/2] Avoid passing global comment_line_char repeatedly
From: Phillip Wood @ 2023-10-30 9:59 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Calvin Wan, Jonathan Tan
In-Reply-To: <20231030051034.2295242-1-gitster@pobox.com>
Hi Junio
On 30/10/2023 05:10, Junio C Hamano wrote:
> Two strbuf functions used to produce commented lines take the
> comment_line_char as their parameter, but in practice, all callers
> feed the global variable comment_line_char from environment.[ch].
>
> Dropping the parameter from the callchain will make the interface
> less flexible, and less error prone. If we choose to change the
> implementation of the customizable comment line character (e.g., we
> may want to stop referencing the global variable and instead use a
> getter function), we will have fewer places we need to modify.
While I agree with your reasoning here, I think that parameter was
recently added as part of the libification effort - I can't remember
exactly why and am too lazy to look it up so I've cc'd Calvin and
Johathan instead.
Best Wishes
Phillip
> Junio C Hamano (2):
> strbuf_commented_addf(): drop the comment_line_char parameter
> strbuf_add_commented_lines(): drop the comment_line_char parameter
>
> add-patch.c | 8 ++++----
> builtin/branch.c | 2 +-
> builtin/merge.c | 8 ++++----
> builtin/notes.c | 9 ++++-----
> builtin/stripspace.c | 2 +-
> builtin/tag.c | 4 ++--
> fmt-merge-msg.c | 9 +++------
> rebase-interactive.c | 8 ++++----
> sequencer.c | 14 ++++++--------
> strbuf.c | 9 +++++----
> strbuf.h | 7 +++----
> wt-status.c | 6 +++---
> 12 files changed, 40 insertions(+), 46 deletions(-)
>
^ permalink raw reply
* Re: [PATCH] git-push: more visibility for -q option
From: Michal Suchánek @ 2023-10-30 10:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwmvhoy57.fsf@gitster.g>
Hello,
On Fri, Oct 20, 2023 at 01:08:04PM -0700, Junio C Hamano wrote:
> Michal Suchanek <msuchanek@suse.de> writes:
>
> > The -v option listed at the top as option al parameter while -q is not.
>
> "as option al parameter" - ECANNOTPARSE. Probably
>
> The `-v` option is shown in the SYNOPSIS section near the top,
> but `-q` is not shown anywhere there.
>
> or something, I think. I agree showing it next to "-v" would make
> the most sense.
when rebasing to current master I see this is already applied with the
fixed commit message, great!
Thanks
Michal
>
> >
> > List -q alongside -v.
> >
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > Documentation/git-push.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> > index 5b4edaf4a8..003bc7d9ce 100644
> > --- a/Documentation/git-push.txt
> > +++ b/Documentation/git-push.txt
> > @@ -10,7 +10,7 @@ SYNOPSIS
> > --------
> > [verse]
> > 'git push' [--all | --branches | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
> > - [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-v | --verbose]
> > + [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-q | --quiet] [-v | --verbose]
>
> Yup, the change makes sense. We may want to wrap the first line to
> a more reasonable length in a separate commit, and when that
> happens, we probably would want to start [-v] [-q] on a separate
> line as well, but for now this would do.
>
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Michal Suchánek @ 2023-10-30 10:42 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030095401.GA848451@coredump.intra.peff.net>
On Mon, Oct 30, 2023 at 05:54:01AM -0400, Jeff King wrote:
> On Mon, Oct 30, 2023 at 10:45:56AM +0100, Michal Suchánek wrote:
>
> > > Yes, this is a problem. Those messages are streamed out as we receive
> > > them from the server, and before we get any status report back. I don't
> > > think we'd want to buffer them, as they can be arbitrarily large (and
> > > may even be progress reports that are meant to be shown in real-time).
> >
> > Not when the user asked to not show progress.
>
> Sure, if you make the buffering behavior dependent on the presence of
> "-q". I guess that is not that hard to do, but it does increase the
> complexity of the code (you have both a streaming and a buffering code
> path).
>
> Anyway, I am not all that convinced this is a fruitful path versus just
> fixing the server side. But if you want to look into writing a patch, go
> for it.
I will try to bug the forge upstream first.
Thanks
Michal
^ permalink raw reply
* Re: [PATCH v2 5/5] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-10-30 10:45 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Oswald Buddenhagen
In-Reply-To: <a165fd76-a44b-45c7-b02c-e9b90fda3db3@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3972 bytes --]
On Sun, Oct 29, 2023 at 04:27:44PM +0000, Phillip Wood wrote:
> Hi Patrick
>
> On 27/10/2023 12:19, Patrick Steinhardt wrote:
> > On Fri, Oct 27, 2023 at 11:19:04AM +0100, Phillip Wood wrote:
> > > On 27/10/2023 10:25, Patrick Steinhardt wrote:
> > > > + parallel:
> > > > + matrix:
> > > > + - jobname: linux-sha256
> > > > + image: ubuntu:latest
> > > > + CC: clang
> > > > + - jobname: linux-gcc
> > > > + image: ubuntu:20.04
> > > > + CC: gcc
> > > > + CC_PACKAGE: gcc-8
> > > > + - jobname: linux-TEST-vars
> > > > + image: ubuntu:20.04
> > > > + CC: gcc
> > > > + CC_PACKAGE: gcc-8
> > > > + - jobname: linux-gcc-default
> > > > + image: ubuntu:latest
> > > > + CC: gcc
> > > > + - jobname: linux-leaks
> > > > + image: ubuntu:latest
> > > > + CC: gcc
> > > > + - jobname: linux-asan-ubsan
> > > > + image: ubuntu:latest
> > > > + CC: clang
> > > > + - jobname: linux-musl
> > > > + image: alpine:latest
>
> I assume you've chosen the configurations from the existing GitHub Actions
> that give the best coverage of the various options. One thing I noticed is
> that the is no equivalent of the "pedantic" job that builds git with
> "DEVELOPER=1 DEVOPTS=pedantic"
Yeah, indeed. The list for sure isn't complete, and I also want to
iterate in the future to add support for macOS and Windows. But adding
the pedantic job is easy enough, so I'll include it.
> > > > diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> > > > index d0bc19d3bb3..1cd92db1876 100755
> > > > --- a/ci/install-docker-dependencies.sh
> > > > +++ b/ci/install-docker-dependencies.sh
> > > > @@ -16,9 +19,13 @@ linux32)
> > > > '
> > > > ;;
> > > > linux-musl)
> > > > - apk add --update build-base curl-dev openssl-dev expat-dev gettext \
> > > > + apk add --update git shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
> > > > pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
> > >
> > > It would be helpful to explain the new dependencies in the commit message. I
> > > can see why you're adding sudo, but how were we getting away without
> > > installing the other packages for GitHub Actions?
I've dropped the "git" dependency for now. It doesn't do anything useful
anyway for our Docker-based jobs. Instead, I've added a preparatory
commit that makes the relevant CI library functions more robust by
detecting the case where we either don't have Git available or don't use
a repository.
> > True, that part is missing.
> >
> > - Both sudo and shadow are now required because of `useradd` that we use
> > to set up the unprivileged build.
> >
> > - Git has been required all along, I think. `save_good_tree ()` is used
> > in our CI scripts, and Toon (fellow GitLabber from my team) has
> > noticed that the CI job warned about missing Git. The warning was
> > mostly benign as it seems, but still, doesn't hurt to fix it while at
> > it.
>
> Oh I had a look at this and the docker based jobs on GitHub do not have a
> git repository so installing git means we now get a repository not found
> error from save_good_tree() instead. We should probably make
> save_good_tree() and check_unignored_build_artifacts() return early if there
> isn't a repository but that's orthogonal to this series.
Yup, done now.
> Looking at the test output from the link in your cover letter we should we
> should also install apache2[1] and gnupg[2]
True. Also missing are Bash, CVS, the Perl CGI module, Subversion and
Perforce. I'll add them as available in the respective distro's
repositories.
Patrick
> [1] https://gitlab.com/gitlab-org/git/-/jobs/5349205374#L1444
> [2] https://gitlab.com/gitlab-org/git/-/jobs/5349205374#L1167
>
> Best Wishes
>
> Phillip
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 5/5] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-10-30 10:46 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Oswald Buddenhagen
In-Reply-To: <8db10489-546d-428d-9b3e-7ecea87e1313@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]
On Sun, Oct 29, 2023 at 04:13:39PM +0000, Phillip Wood wrote:
> Hi Patrick
>
> On 27/10/2023 14:02, Phillip Wood wrote:
> > On 27/10/2023 12:57, Patrick Steinhardt wrote:
> > > 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.
> >
> > I was trying to suggest that we start using these scripts again.
>
> 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.
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 0/8] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-10-30 12:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7779 bytes --]
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
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 1/8] ci: reorder definitions for grouping functions
From: Patrick Steinhardt @ 2023-10-30 12:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]
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(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 6fbb5bade12..eb384f4e952 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -1,16 +1,7 @@
# Library of functions shared by all CI scripts
-if test true != "$GITHUB_ACTIONS"
+if test true = "$GITHUB_ACTIONS"
then
- begin_group () { :; }
- end_group () { :; }
-
- group () {
- shift
- "$@"
- }
- set -x
-else
begin_group () {
need_to_end_group=t
echo "::group::$1" >&2
@@ -42,6 +33,15 @@ else
}
begin_group "CI setup"
+else
+ begin_group () { :; }
+ end_group () { :; }
+
+ group () {
+ shift
+ "$@"
+ }
+ set -x
fi
# Set 'exit on error' for all CI scripts to let the caller know that
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 2/8] ci: make grouping setup more generic
From: Patrick Steinhardt @ 2023-10-30 12:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]
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(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index eb384f4e952..b3411afae8e 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,36 +14,34 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
- trap end_group EXIT
-
- group () {
- set +x
- begin_group "$1"
- 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
- return $res
- }
-
- begin_group "CI setup"
else
begin_group () { :; }
end_group () { :; }
- 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
+
# Set 'exit on error' for all CI scripts to let the caller know that
# something went wrong.
#
@@ -287,5 +285,5 @@ esac
MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
-end_group
+end_group "CI setup"
set -x
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 3/8] ci: group installation of Docker dependencies
From: Patrick Steinhardt @ 2023-10-30 12:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]
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(+)
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 78b7e326da6..d0bc19d3bb3 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -3,6 +3,10 @@
# Install dependencies required to build and test Git inside container
#
+. ${0%/*}/lib.sh
+
+begin_group "Install dependencies"
+
case "$jobname" in
linux32)
linux32 --32bit i386 sh -c '
@@ -20,3 +24,5 @@ pedantic)
dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
;;
esac
+
+end_group "Install dependencies"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 4/8] ci: split out logic to set up failed test artifacts
From: Patrick Steinhardt @ 2023-10-30 12:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2381 bytes --]
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(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index b3411afae8e..9ffdf743903 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -131,6 +131,27 @@ handle_failed_tests () {
return 1
}
+create_failed_test_artifacts () {
+ mkdir -p t/failed-test-artifacts
+
+ for test_exit in t/test-results/*.exit
+ do
+ test 0 != "$(cat "$test_exit")" || continue
+
+ test_name="${test_exit%.exit}"
+ test_name="${test_name##*/}"
+ printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
+ echo "The full logs are in the 'print test failures' step below."
+ echo "See also the 'failed-tests-*' artifacts attached to this run."
+ cat "t/test-results/$test_name.markup"
+
+ trash_dir="t/trash directory.$test_name"
+ 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
export TERM=${TERM:-dumb}
@@ -171,25 +192,8 @@ then
CC="${CC_PACKAGE:-${CC:-gcc}}"
DONT_SKIP_TAGS=t
handle_failed_tests () {
- mkdir -p t/failed-test-artifacts
echo "FAILED_TEST_ARTIFACTS=t/failed-test-artifacts" >>$GITHUB_ENV
-
- for test_exit in t/test-results/*.exit
- do
- test 0 != "$(cat "$test_exit")" || continue
-
- test_name="${test_exit%.exit}"
- test_name="${test_name##*/}"
- printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
- echo "The full logs are in the 'print test failures' step below."
- echo "See also the 'failed-tests-*' artifacts attached to this run."
- cat "t/test-results/$test_name.markup"
-
- trash_dir="t/trash directory.$test_name"
- 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
}
cache_dir="$HOME/none"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 5/8] ci: unify setup of some environment variables
From: Patrick Steinhardt @ 2023-10-30 12:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2818 bytes --]
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(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 9ffdf743903..c7a716a6e3f 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -175,11 +175,7 @@ then
# among *all* phases)
cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
- export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
- export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows_nt != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--write-junit-xml"
elif test true = "$GITHUB_ACTIONS"
then
CI_TYPE=github-actions
@@ -198,17 +194,25 @@ then
cache_dir="$HOME/none"
- export GIT_PROVE_OPTS="--timer --jobs 10"
- export GIT_TEST_OPTS="--verbose-log -x --github-workflow-markup"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--github-workflow-markup"
else
echo "Could not identify CI type" >&2
env >&2
exit 1
fi
+MAKEFLAGS="$MAKEFLAGS --jobs=$(nproc)"
+GIT_PROVE_OPTS="--timer --jobs $(nproc) --state=failed,slow,save"
+
+GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
+if test windows = "$CI_OS_NAME"
+then
+ GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
+fi
+
+export GIT_TEST_OPTS
+export GIT_PROVE_OPTS
+
good_trees_file="$cache_dir/good-trees"
mkdir -p "$cache_dir"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 6/8] ci: squelch warnings when testing with unusable Git repo
From: Patrick Steinhardt @ 2023-10-30 12:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]
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(+)
diff --git a/ci/lib.sh b/ci/lib.sh
index c7a716a6e3f..c13b6527cac 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -69,10 +69,32 @@ skip_branch_tip_with_tag () {
fi
}
+# Check whether we can use the path passed via the first argument as Git
+# repository.
+is_usable_git_repository () {
+ # We require Git in our PATH, otherwise we cannot access repositories
+ # at all.
+ if ! command -v git >/dev/null
+ then
+ return 1
+ fi
+
+ # And the target directory needs to be a proper Git repository.
+ if ! git -C "$1" rev-parse 2>/dev/null
+ then
+ return 1
+ fi
+}
+
# Save some info about the current commit's tree, so we can skip the build
# job if we encounter the same tree again and can provide a useful info
# message.
save_good_tree () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
echo "$(git rev-parse $CI_COMMIT^{tree}) $CI_COMMIT $CI_JOB_NUMBER $CI_JOB_ID" >>"$good_trees_file"
# limit the file size
tail -1000 "$good_trees_file" >"$good_trees_file".tmp
@@ -88,6 +110,11 @@ skip_good_tree () {
return
fi
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
if ! good_tree_info="$(grep "^$(git rev-parse $CI_COMMIT^{tree}) " "$good_trees_file")"
then
# Haven't seen this tree yet, or no cached good trees file yet.
@@ -119,6 +146,11 @@ skip_good_tree () {
}
check_unignored_build_artifacts () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
! git ls-files --other --exclude-standard --error-unmatch \
-- ':/*' 2>/dev/null ||
{
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-10-30 12:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]
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(-)
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index d0bc19d3bb3..05dde5c5d40 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -17,7 +17,8 @@ linux32)
;;
linux-musl)
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
;;
pedantic)
dnf -yq update >/dev/null &&
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 2fb1b2ae561..9791f94b16f 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -67,7 +67,8 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
'/usr/lib/apache2/modules' \
'/usr/lib64/httpd/modules' \
'/usr/lib/httpd/modules' \
- '/usr/libexec/httpd'
+ '/usr/libexec/httpd' \
+ '/usr/lib/apache2'
do
if test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 8/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-10-30 12:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6995 bytes --]
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
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 00000000000..cd98bcb18aa
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,53 @@
+default:
+ timeout: 2h
+
+workflow:
+ rules:
+ - if: $CI_PIPELINE_SOURCE == "merge_request_event"
+ - if: $CI_COMMIT_TAG
+ - if: $CI_COMMIT_REF_PROTECTED == "true"
+
+test:
+ image: $image
+ before_script:
+ - ./ci/install-docker-dependencies.sh
+ script:
+ - 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:
+ - |
+ if test "$CI_JOB_STATUS" != 'success'
+ then
+ sudo --preserve-env --set-home --user=builder ./ci/print-test-failures.sh
+ fi
+ parallel:
+ matrix:
+ - jobname: linux-sha256
+ image: ubuntu:latest
+ CC: clang
+ - jobname: linux-gcc
+ image: ubuntu:20.04
+ CC: gcc
+ CC_PACKAGE: gcc-8
+ - jobname: linux-TEST-vars
+ image: ubuntu:20.04
+ CC: gcc
+ CC_PACKAGE: gcc-8
+ - jobname: linux-gcc-default
+ image: ubuntu:latest
+ CC: gcc
+ - jobname: linux-leaks
+ image: ubuntu:latest
+ CC: gcc
+ - jobname: linux-asan-ubsan
+ image: ubuntu:latest
+ CC: clang
+ - jobname: pedantic
+ image: fedora:latest
+ - jobname: linux-musl
+ image: alpine:latest
+ artifacts:
+ paths:
+ - t/failed-test-artifacts
+ when: on_failure
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 05dde5c5d40..5e28adf55b6 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -7,6 +7,9 @@
begin_group "Install dependencies"
+# Required so that apt doesn't wait for user input on certain packages.
+export DEBIAN_FRONTEND=noninteractive
+
case "$jobname" in
linux32)
linux32 --32bit i386 sh -c '
@@ -16,10 +19,18 @@ linux32)
'
;;
linux-musl)
- apk add --update build-base curl-dev openssl-dev expat-dev gettext \
+ apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
pcre2-dev python3 musl-libintl perl-utils ncurses \
apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
+linux-*)
+ apt update -q &&
+ apt install -q -y sudo git make language-pack-is libsvn-perl apache2 libssl-dev \
+ libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev \
+ perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
+ libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
+ apache2 cvs cvsps gnupg libcgi-pm-perl subversion
+ ;;
pedantic)
dnf -yq update >/dev/null &&
dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
diff --git a/ci/lib.sh b/ci/lib.sh
index c13b6527cac..d836fad7dd6 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,6 +14,22 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
+elif test true = "$GITLAB_CI"
+then
+ begin_group () {
+ need_to_end_group=t
+ printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1\n"
+ trap "end_group '$1'" EXIT
+ set -x
+ }
+
+ end_group () {
+ test -n "$need_to_end_group" || return 0
+ set +x
+ need_to_end_group=
+ printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n"
+ trap - EXIT
+ }
else
begin_group () { :; }
end_group () { :; }
@@ -227,6 +243,33 @@ then
cache_dir="$HOME/none"
GIT_TEST_OPTS="--github-workflow-markup"
+elif test true = "$GITLAB_CI"
+then
+ CI_TYPE=gitlab-ci
+ CI_BRANCH="$CI_COMMIT_REF_NAME"
+ CI_COMMIT="$CI_COMMIT_SHA"
+ case "$CI_JOB_IMAGE" in
+ macos-*)
+ CI_OS_NAME=osx;;
+ alpine:*|fedora:*|ubuntu:*)
+ CI_OS_NAME=linux;;
+ *)
+ echo "Could not identify OS image" >&2
+ env >&2
+ exit 1
+ ;;
+ esac
+ CI_REPO_SLUG="$CI_PROJECT_PATH"
+ CI_JOB_ID="$CI_JOB_ID"
+ CC="${CC_PACKAGE:-${CC:-gcc}}"
+ DONT_SKIP_TAGS=t
+ handle_failed_tests () {
+ create_failed_test_artifacts
+ }
+
+ cache_dir="$HOME/none"
+
+ runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
else
echo "Could not identify CI type" >&2
env >&2
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 57277eefcd0..c33ad4e3a22 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -51,6 +51,12 @@ do
tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
continue
;;
+ gitlab-ci)
+ mkdir -p failed-test-artifacts
+ cp "${TEST_EXIT%.exit}.out" failed-test-artifacts/
+ tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
+ continue
+ ;;
*)
echo "Unhandled CI type: $CI_TYPE" >&2
exit 1
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v3 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-10-30 12:47 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <f150d61a1ceec5dd6cf370c3f4421a3e2d9f11a3.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3326 bytes --]
On Mon, Oct 30, 2023 at 01:15:10PM +0100, Patrick Steinhardt wrote:
> 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(-)
>
> diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> index d0bc19d3bb3..05dde5c5d40 100755
> --- a/ci/install-docker-dependencies.sh
> +++ b/ci/install-docker-dependencies.sh
> @@ -17,7 +17,8 @@ linux32)
> ;;
> linux-musl)
> 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
Meh. I just noticed that I missed a few other dependencies to make
Apache2 work:
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 5e28adf55b..ce910e3f3c 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -21,7 +21,8 @@ linux32)
linux-musl)
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 \
+ bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
linux-*)
apt update -q &&
But once fixed, tests do indeed start to fail:
t5540-http-push-webdav.sh (Wstat: 256 (exited 1) Tests: 20 Failed: 11)
Failed tests: 5-11, 13, 15-16, 18
Non-zero exit status: 1
Seems like another thing to fix in a separate patch series.
Patrick
> ;;
> pedantic)
> dnf -yq update >/dev/null &&
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index 2fb1b2ae561..9791f94b16f 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -67,7 +67,8 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
> '/usr/lib/apache2/modules' \
> '/usr/lib64/httpd/modules' \
> '/usr/lib/httpd/modules' \
> - '/usr/libexec/httpd'
> + '/usr/libexec/httpd' \
> + '/usr/lib/apache2'
> do
> if test -d "$DEFAULT_HTTPD_MODULE_PATH"
> then
> --
> 2.42.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox