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@xxxxxxxxx>
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@xxxxxxxxx/
[...]
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
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.
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.
Moving things from environment variables into MAKEFLAGS adds
unnecessary complexity as we still have to export those variables on
windows.
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.
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.
Best Wishes
Phillip
