Re: ab/ci-setup-simplify (was Re: What's cooking in git.git (Apr 2022, #05; Mon, 18))

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 22, 2022, 11:28 UTC
Message-ID: <220422.86v8v14a4w.gmgdl@evledraar.gmail.com>
In-Reply-To: <34a9c2dc-f8f5-4c1b-fed8-115429ae5a9f@gmail.com>

 
On Fri, Apr 22 2022, Phillip Wood wrote:

Show 40 quoted lines

> Hi Ævar
>
> On 21/04/2022 19:36, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, Apr 19 2022, Phillip Wood wrote:
>> 
>>>> * ab/ci-setup-simplify (2022-04-14) 29 commits
>>>> [...]
>>>>    Will merge to 'next'?
>>>>    source: <cover-v3-00.29-00000000000-20220413T194847Z-avarab@gmail.com>
>>>
>>> I haven't had time to read all 31 patches from v4 in detail but I have
>>> looked at the results in seen.
>>>
>>> Looking at seen:ci/install-dependencies.sh the shebang has been
>>> changed to "#!/bin/sh" but it contains
>>> "BREW_PACKAGE=${CC_PACKAGE/-/@}" which is a bashism.
>>>
>>> Looking at seen:.github/workflows/main.yaml to skip running the tests
>>> one has to set "skip-tests: no" which is utterly confusing.
>>>
>>>  From what I saw scanning the patches there seemed to be a lot of
>>> churn, both of existing code and code that gets added and then
>>> moved/refactored within the series.
>>>
>>> Looking at the output of a recent ci run of seen the steps to prepare
>>> the environment before building and testing print all the environment
>>> variables rather than just the ones being set for that step which
>>> seems to go against the aim of "CI: narrow down variable definitions
>>> in --build and --test". (Also the "SKIP" prefix in the output lacks a
>>> ":")
>> Thanks. Those were all helpful. I replied to these in a re-roll CL
>> at:
>> https://lore.kernel.org/git/cover-v5-00.29-00000000000-20220421T181526Z-avarab@gmail.com/
>> [...] 
>>> I think splitting out the build and test steps is a good idea but I'm
>>> less convinced by some of the other changes.
>> What other changes are you referring to here?
>
> Here's a list from memory - apologies if anything here has changed in
> v5

Thanks for following up, I think "probably not" re "anything changed"...

Show 5 quoted lines

> Separating the environment setup from running the build/tests is quite
> github specific. If instead we just had scripts that setup the
> environment and ran the appropriate make command they could be used by
> any future CI setup. Doing that would avoid lib.sh being a top level
> script rather than a library as well.

The part where we do :

 - ci/lib.sh --build
 - make

Is very GitHub-specific, i.e. it's relying on the "setenv" wrapper setting things in $GITHUB_ENV, which is then carried across "steps".

Now, it could just use "export" and have every "step" be the equivalent of:

 - ci/lib.sh --build && make

I.e. to have it "export", and not use $GITHUB_ENV, but I think the resulting UX is much better. I.e. the second step shows what variables are in use by the "step" now.

And for future-proofing I think we're better off as far as CI portability is concerned, and in fact I mostly want this so I can trivially run things locally "like in CI".

Adding another CI is a trivial matter of either eval-ing the ci/lib.sh output (which the tip commit emits instructions about how to do), or echo-ing to whatever that CI's equivalent of $GITHUB_ENV is.

> I expected tput.sh to be some kind of wrapper around tput but it just
> sets $TERM if it is not already set. We already do that in lib.sh I
> think and I don't see what the point of changing that. I think we'd
> want to do that whenever TERM is not set, not just for github actions.

You're right that we could do that earlier, i.e. to stick it in $GITHUB_ENV, but for these variables that were used only for some implementation detail of the scripts themselves I wanted to "source" them so they wouldn't appear in the variable list at the top.

I.e. that variable (unlike MAKEFLAGS etc.) isn't important to see how a run failed, and how it was configured, it's just working around platform-specific nits in the GitHub CI itself.

On the other hand it's just used in two places, and it's two lines, I could just copy/paste it. Do you think that's better?

> Moving things from environment variables into MAKEFLAGS adds
> unnecessary complexity as we still have to export those variables on
> windows.

I may have gotten something wrong here, but I was careful to move only the ones that are *for the Makefile* into MAKEFLAGS.

In most cases it's equivalent, but re $GITHUB_ENV dump readability it makes it more obvious what's being set for what things. I.e. Makefile itself v.s. the actual tests it then runs.

> The script to run docker builds and tests under a unprivileged user
> is just deleted rather than fixing our docker builds to not run as
> root. At the moment some of the httpd tests are skipped because they
> refuse to run as root.

What tests are being skipped? I may have gotten this horribly wrong, but I "git rm'd" those as part of deleting more leftover dead Travis code. I don't see where our current CI uses these at all.

*looks a bit*

I think you mean this, but this is in next (and master, but for some reason that run wasn't there): https://github.com/git/git/runs/6064756450?check_suite_focus=true#step:5:1279

So we should probably fix that, but I think that's unrelated to my series. It looks like we've been skipping those tests on GitHub CI ever since we had it at all.

> One thing I do like is moving the environment setup out of the github
> actions yaml and into the scripts as it should make it easier to
> support other CI setups.

*nod*, I very much have that as a goal.