From: Phillip Wood Date: Fri, 22 Apr 2022 09:30:38 GMT Subject: Re: ab/ci-setup-simplify (was Re: What's cooking in git.git (Apr 2022, #05; Mon, 18)) Message-ID: <34a9c2dc-f8f5-4c1b-fed8-115429ae5a9f@gmail.com> In-Reply-To: <220421.86fsm66zmz.gmgdl@evledraar.gmail.com> 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: >> >> 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 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