Re: [PATCH v4 5/8] ci: unify setup of some environment variables

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3584 bytes --]

On Tue, Oct 31, 2023 at 10:06:24AM -0700, Victoria Dye wrote:
> Patrick Steinhardt wrote:
> > Both GitHub Actions and Azue Pipelines set up the environment variables
> 
> s/Azue/Azure
> 
> > 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 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..9a9b92c05b3 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=10"
> > +GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> > +
> > +GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
> > +if test windows = "$CI_OS_NAME"
> 
> Based on the deleted lines above, I think this would need to be:
> 
> 	if test windows = "$CI_OS_NAME" || test windows_nt = "$CI_OS_NAME"
> 
> I believe these settings are required on all Windows builds, though, so you could 
> instead match on the first 7 characters of $CI_OS_NAME:
> 
> 	if test windows = "$(echo "$CI_OS_NAME" | cut -c1-7)"
> 
> (full disclosure: I'm not 100% confident in the correctness of that shell syntax)

Oh, right. I didn't notice the slight difference between "windows" and
"windows_nt". Thanks!

Patrick

> > +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"
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]