Re: [PATCH] ci: only warn about perforce/git-lfs/JGit on platforms that need them

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> perforce, git-lfs, and JGit test git's own interop code, not anything
> platform-specific, so installing them once on ubuntu-* (all three)
> and macos-* (perforce) is enough coverage. debian, i386/ubuntu,
> alpine, fedora and almalinux never install them, yet the presence
> check at the end of the script warned on all of them anyway.
>
> Scope each check to the platforms that attempt the install, so a
> warning means one actually failed.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>     ci: only warn about perforce/git-lfs/JGit on platforms that need them
>     
>     Only warn about a missing perforce/git-lfs/JGit install on the platforms
>     that actually need and attempt them (ubuntu-*, plus macOS for perforce),
>     since every other platform never installs them and was warning
>     regardless.

It is curious that nobody seems to have looked at this patch, as my
cursory look suggests it would be a no brainer to check correctness
of this under the assumption that nothing will change externally.

The maintenance to keep this in sync with what exactly are tested in
each platforms will be made more costly with this change, but I do
not know by how much.  If somebody wants to start testing p4 on a
different platform, for example, as I think t98xx will just punt
without failing if p4 is not available, it will probably be a while
until they eventually notice that their test do not run due to lack
of p4 and then they have to add their platform to the logic added by
this patch.  I think jgit is also the same; silent success when JGIT
prerequisite is not met.  But if they are motivated enough to add
tests, they will eventually notice when the tests they wanted to run
were not running, so it is a reasonably low risk.  If somebody wants
to drop testing jgit on a platform, we may still install jgit even
though we do not run tests that require jgit, which may take longer
for us to notice, but the result is just as bad at most as the state
without this change, so overall I think it makes sense.

Any volunteers to offer a second pair of eyes?

Thanks.


> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2403%2FHaraldNordgren%2Fci-scope-optional-tool-warnings-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2403/HaraldNordgren/ci-scope-optional-tool-warnings-v1
> Pull-Request: https://github.com/git/git/pull/2403
>
>  ci/install-dependencies.sh | 54 ++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 2f61fbb07c..a68cec64b4 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -171,30 +171,38 @@ Documentation)
>  	;;
>  esac
>  
> -if type p4d >/dev/null 2>&1 && type p4 >/dev/null 2>&1
> -then
> -	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
> -	p4d -V
> -	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
> -	p4 -V
> -else
> -	echo >&2 "::warning:: perforce wasn't installed, see above for clues why"
> -fi
> +case "$distro" in
> +ubuntu-*|macos-*)
> +	if type p4d >/dev/null 2>&1 && type p4 >/dev/null 2>&1
> +	then
> +		echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
> +		p4d -V
> +		echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
> +		p4 -V
> +	else
> +		echo >&2 "::warning:: perforce wasn't installed, see above for clues why"
> +	fi
> +	;;
> +esac
>  
> -if type git-lfs >/dev/null 2>&1
> -then
> -	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
> -	git-lfs version
> -else
> -	echo >&2 "::warning:: git-lfs wasn't installed, see above for clues why"
> -fi
> +case "$distro" in
> +ubuntu-*)
> +	if type git-lfs >/dev/null 2>&1
> +	then
> +		echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
> +		git-lfs version
> +	else
> +		echo >&2 "::warning:: git-lfs wasn't installed, see above for clues why"
> +	fi
>  
> -if type jgit >/dev/null 2>&1
> -then
> -	echo "$(tput setaf 6)JGit Version$(tput sgr0)"
> -	jgit version
> -else
> -	echo >&2 "::warning:: JGit wasn't installed, see above for clues why"
> -fi
> +	if type jgit >/dev/null 2>&1
> +	then
> +		echo "$(tput setaf 6)JGit Version$(tput sgr0)"
> +		jgit version
> +	else
> +		echo >&2 "::warning:: JGit wasn't installed, see above for clues why"
> +	fi
> +	;;
> +esac
>  
>  end_group "Install dependencies"
>
> base-commit: 47ce80527c56f462cb97db4ca8125342204d3783