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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 30, 2023, 15:09 UTC
Message-ID: <87430c6c-91c0-4be1-b89d-bf442b3f018b@gmail.com>
In-Reply-To: <6af0075fd875f176e7fdf6c219e7117dac5cd71c.1698667545.git.ps@pks.im>

Hi Patrick

On 30/10/2023 12:15, Patrick Steinhardt wrote:

Show 12 quoted lines

> 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)

Show 7 quoted lines

>      - 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]

Show 46 quoted lines

> 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