| Message ID | 6af0075fd875f176e7fdf6c219e7117dac5cd71c.1698667545.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ci: add GitLab CI definition | expand |
Hi Patrick On 30/10/2023 12:15, Patrick Steinhardt wrote: > 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. 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 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. I don't mind this change, but the 'export VAR=value' syntax is in POSIX[2] > 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 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: > Hi Patrick > > On 30/10/2023 12:15, Patrick Steinhardt wrote: > > 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. > > 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) 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 :) > > - 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. > > I don't mind this change, but the 'export VAR=value' syntax is in POSIX[2] 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. > > 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 > > 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 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: > On 30/10/2023 12:15, Patrick Steinhardt wrote: >> 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. > > 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)" 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. > 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)
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"
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(-)