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

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

[Harald Nordgren via GitGitGadget]

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.

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

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

[Junio C Hamano]

"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

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

[Patrick Steinhardt]

On Sat, Sep 12, 2026 at 02:38:02PM +0000, Harald Nordgren via GitGitGadget wrote:
> 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

I wonder whether it makes sense to have these warnings in the first
place.

Part of the reason why we have these checks is that we allow the
installation of these tools to fail, and if so we know to gracefully
continue anyway. Tests will be skipped, and the pipeline will be green
in such a case. But is that even a safe thing to do? I strongly doubt
that we'd start to notice such failures anytime soon, so it very much
gives us a false sense of confidence.

So I'd suggest that instead of warning, we should make the whole build
fail outright if we fail to install any of those tools. And once we do,
these warnings here become quite useless, because we know that the build
would fail on platforms where we expect the tools to be present. And on
platforms where we don't, the warning is pointless anyway.

Thanks!

Patrick

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

[Harald Nordgren]

> I wonder whether it makes sense to have these warnings in the first
> place.
>
> Part of the reason why we have these checks is that we allow the
> installation of these tools to fail, and if so we know to gracefully
> continue anyway. Tests will be skipped, and the pipeline will be green
> in such a case. But is that even a safe thing to do? I strongly doubt
> that we'd start to notice such failures anytime soon, so it very much
> gives us a false sense of confidence.
>
> So I'd suggest that instead of warning, we should make the whole build
> fail outright if we fail to install any of those tools. And once we do,
> these warnings here become quite useless, because we know that the build
> would fail on platforms where we expect the tools to be present. And on
> platforms where we don't, the warning is pointless anyway.

Fine by me!


Harald

[PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

A failed install of perforce, git-lfs, or JGit was silently
tolerated. The corresponding tests would just skip and the build
stayed green, so a broken download could go unnoticed indefinitely.

Fail the build immediately instead on the platforms that install
these tools. The presence checks and warnings at the end of the
script are gone too, since we already know by then that everything
installed or the build wouldn't have gotten this far.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
    ci: fail the build if perforce, git-lfs or JGit fail to install
    
    Fail the build if perforce, git-lfs or JGit fail to install on the
    platforms that install them.
    
    Changes in v2:
    
     * Perforce, git-lfs, and JGit install failures now fail the build
       instead of being silently swallowed. Removed the presence checks and
       warnings at the end of the script, dead code once install failure is
       fatal.
     * Version output (p4d -V, git-lfs version, jgit version) moved inline
       right after each successful install.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2403%2FHaraldNordgren%2Fci-scope-optional-tool-warnings-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2403/HaraldNordgren/ci-scope-optional-tool-warnings-v2
Pull-Request: https://github.com/git/git/pull/2403

Range-diff vs v1:

 1:  3ce51dc322 ! 1:  dde1b38567 ci: only warn about perforce/git-lfs/JGit on platforms that need them
     @@ Metadata
      Author: Harald Nordgren <haraldnordgren@gmail.com>
      
       ## Commit message ##
     -    ci: only warn about perforce/git-lfs/JGit on platforms that need them
     +    ci: fail the build if perforce, git-lfs, or JGit fail to install
      
     -    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.
     +    A failed install of perforce, git-lfs, or JGit was silently
     +    tolerated. The corresponding tests would just skip and the build
     +    stayed green, so a broken download could go unnoticed indefinitely.
      
     -    Scope each check to the platforms that attempt the install, so a
     -    warning means one actually failed.
     +    Fail the build immediately instead on the platforms that install
     +    these tools. The presence checks and warnings at the end of the
     +    script are gone too, since we already know by then that everything
     +    installed or the build wouldn't have gotten this far.
      
          Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
      
       ## ci/install-dependencies.sh ##
     +@@ ci/install-dependencies.sh: ubuntu-*|i386/ubuntu-*|debian-*)
     + 			"$P4WHENCE/bin.linux26x86_64/p4d" \
     + 			"$P4WHENCE/bin.linux26x86_64/p4" &&
     + 		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
     +-			rm -f "$CUSTOM_PATH/p4"
     +-			rm -f "$CUSTOM_PATH/p4d"
     ++			echo >&2 "error: could not install perforce"
     ++			exit 1
     + 		}
     ++		echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
     ++		p4d -V
     ++		echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
     ++		p4 -V
     + 
     + 		wget --quiet \
     + 		     "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" &&
     + 		tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
     + 			-C "$CUSTOM_PATH" --strip-components=1 \
     + 			"git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" &&
     +-		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" ||
     +-		rm -f "$CUSTOM_PATH/git-lfs"
     ++		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" || {
     ++			echo >&2 "error: could not install git-lfs"
     ++			exit 1
     ++		}
     ++		echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
     ++		git-lfs version
     + 
     + 		wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit" &&
     +-		chmod a+x "$CUSTOM_PATH/jgit" ||
     +-		rm -f "$CUSTOM_PATH/jgit"
     ++		chmod a+x "$CUSTOM_PATH/jgit" || {
     ++			echo >&2 "error: could not install JGit"
     ++			exit 1
     ++		}
     ++		echo "$(tput setaf 6)JGit Version$(tput sgr0)"
     ++		jgit version
     + 		;;
     + 	esac
     + 	;;
     +@@ ci/install-dependencies.sh: macos-*)
     + 
     + 	mkdir -p "$CUSTOM_PATH"
     + 	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
     +-	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
     ++	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d || {
     ++		echo >&2 "error: could not install perforce"
     ++		exit 1
     ++	}
     + 	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
     + 	rm helix-core-server.tgz
     ++	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
     ++	p4d -V
     ++	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
     ++	p4 -V
     + 
     + 	case "$jobname" in
     + 	osx-meson)
      @@ ci/install-dependencies.sh: Documentation)
       	;;
       esac
     @@ ci/install-dependencies.sh: Documentation)
      -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)"
     @@ ci/install-dependencies.sh: Documentation)
      -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)"
     @@ ci/install-dependencies.sh: Documentation)
      -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"


 ci/install-dependencies.sh | 59 +++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index d57dce5663..dad35db78b 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -95,21 +95,33 @@ ubuntu-*|i386/ubuntu-*|debian-*)
 			"$P4WHENCE/bin.linux26x86_64/p4d" \
 			"$P4WHENCE/bin.linux26x86_64/p4" &&
 		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
-			rm -f "$CUSTOM_PATH/p4"
-			rm -f "$CUSTOM_PATH/p4d"
+			echo >&2 "error: could not install perforce"
+			exit 1
 		}
+		echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
+		p4d -V
+		echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
+		p4 -V
 
 		wget --quiet \
 		     "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" &&
 		tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
 			-C "$CUSTOM_PATH" --strip-components=1 \
 			"git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" &&
-		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" ||
-		rm -f "$CUSTOM_PATH/git-lfs"
+		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" || {
+			echo >&2 "error: could not install git-lfs"
+			exit 1
+		}
+		echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
+		git-lfs version
 
 		wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit" &&
-		chmod a+x "$CUSTOM_PATH/jgit" ||
-		rm -f "$CUSTOM_PATH/jgit"
+		chmod a+x "$CUSTOM_PATH/jgit" || {
+			echo >&2 "error: could not install JGit"
+			exit 1
+		}
+		echo "$(tput setaf 6)JGit Version$(tput sgr0)"
+		jgit version
 		;;
 	esac
 	;;
@@ -121,9 +133,16 @@ macos-*)
 
 	mkdir -p "$CUSTOM_PATH"
 	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
-	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
+	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d || {
+		echo >&2 "error: could not install perforce"
+		exit 1
+	}
 	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
 	rm helix-core-server.tgz
+	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
+	p4d -V
+	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
+	p4 -V
 
 	case "$jobname" in
 	osx-meson)
@@ -171,30 +190,4 @@ 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
-
-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
-
 end_group "Install dependencies"

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e
-- 
gitgitgadget

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Junio C Hamano]

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

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> A failed install of perforce, git-lfs, or JGit was silently
> tolerated. The corresponding tests would just skip and the build
> stayed green, so a broken download could go unnoticed indefinitely.
>
> Fail the build immediately instead on the platforms that install
> these tools. The presence checks and warnings at the end of the
> script are gone too, since we already know by then that everything
> installed or the build wouldn't have gotten this far.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> ...
>  ci/install-dependencies.sh | 59 +++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 33 deletions(-)
>
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index d57dce5663..dad35db78b 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -95,21 +95,33 @@ ubuntu-*|i386/ubuntu-*|debian-*)
>  			"$P4WHENCE/bin.linux26x86_64/p4d" \
>  			"$P4WHENCE/bin.linux26x86_64/p4" &&
>  		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
> -			rm -f "$CUSTOM_PATH/p4"
> -			rm -f "$CUSTOM_PATH/p4d"
> +			echo >&2 "error: could not install perforce"
> +			exit 1
>  		}

We used to attempt cleaning after ourselves before giving up (and
continuing), but now we explicitly die and there is no need to
remove the half-installed stuff, which makes sense.  The same
comment for lfs and jgit (omitted from quotes).

> @@ -121,9 +133,16 @@ macos-*)
>  
>  	mkdir -p "$CUSTOM_PATH"
>  	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
> -	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
> +	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d || {
> +		echo >&2 "error: could not install perforce"
> +		exit 1
> +	}
>  	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
>  	rm helix-core-server.tgz

The earlier one tried to make them executable, and failed when it
cannot. I wonder you want to move "|| { ... fail ... }" below the
xattr line for consistency, but the command line says "we do not
care if we cannot remove the attribute and will keep going", so it
cannot be done without dropping "|| true" there.  Hmm...

> +	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
> +	p4d -V
> +	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
> +	p4 -V
>  
>  	case "$jobname" in
>  	osx-meson)
> @@ -171,30 +190,4 @@ Documentation)
>  	;;
>  esac
>  
> -if type p4d >/dev/null 2>&1 && type p4 >/dev/null 2>&1
> -then
> ...
> -	jgit version
> -else
> -	echo >&2 "::warning:: JGit wasn't installed, see above for clues why"
> -fi
> -

And it is very nice to see these all go, as we have done the
reporting all very close to where they are installed.

>  end_group "Install dependencies"
>
> base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Harald Nordgren]

I interpret this as needing no further action now. Is that correct?

Thanks for looking at this!


Harald

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

> I interpret this as needing no further action now. Is that correct?

Not necessarily.  It merely means that I just didn't find anything
wrong within the time I could allocate for this topic.  Somebody
else may want to give a real positive review to move it forward.

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Patrick Steinhardt]

On Fri, Oct 23, 2026 at 04:50:49PM +0000, Harald Nordgren via GitGitGadget wrote:
>     Changes in v2:
>     
>      * Perforce, git-lfs, and JGit install failures now fail the build
>        instead of being silently swallowed. Removed the presence checks and
>        warnings at the end of the script, dead code once install failure is
>        fatal.

Thanks for adapting!

> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index d57dce5663..dad35db78b 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -95,21 +95,33 @@ ubuntu-*|i386/ubuntu-*|debian-*)
>  			"$P4WHENCE/bin.linux26x86_64/p4d" \
>  			"$P4WHENCE/bin.linux26x86_64/p4" &&
>  		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
> -			rm -f "$CUSTOM_PATH/p4"
> -			rm -f "$CUSTOM_PATH/p4d"
> +			echo >&2 "error: could not install perforce"
> +			exit 1
>  		}
> +		echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
> +		p4d -V
> +		echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
> +		p4 -V

Okay. I was wondering whether we'd even see these binaries or whether
we'd have to `"${CUSTOM_PATH}"/p4d -V` instead. But `$CUSTOM_PATH` gets
set up in "ci/lib.sh" already, so there's no need for that.

>  		wget --quiet \
>  		     "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" &&
>  		tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
>  			-C "$CUSTOM_PATH" --strip-components=1 \
>  			"git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" &&
> -		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" ||
> -		rm -f "$CUSTOM_PATH/git-lfs"
> +		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" || {
> +			echo >&2 "error: could not install git-lfs"
> +			exit 1
> +		}

This could all be a bit less convoluted if we had `set -e` in
"ci/lib.sh". But that certainly is outside the scope of this patch
series, so meanwhile this looks good to me.

> @@ -121,9 +133,16 @@ macos-*)
>  
>  	mkdir -p "$CUSTOM_PATH"
>  	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
> -	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
> +	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d || {
> +		echo >&2 "error: could not install perforce"
> +		exit 1
> +	}
>  	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true

This line is a bit curious, it makes me wonder whether we want to
tighten it as well now. Any reason why we wouldn't?

>  	rm helix-core-server.tgz

Nit: this is inconsistent with git-lfs, where we also have the call to
rm as part of the condition chain.

Thanks!

Patrick

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Harald Nordgren]

> >               wget --quiet \
> >                    "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" &&
> >               tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
> >                       -C "$CUSTOM_PATH" --strip-components=1 \
> >                       "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" &&
> > -             rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" ||
> > -             rm -f "$CUSTOM_PATH/git-lfs"
> > +             rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" || {
> > +                     echo >&2 "error: could not install git-lfs"
> > +                     exit 1
> > +             }
>
> This could all be a bit less convoluted if we had `set -e` in
> "ci/lib.sh". But that certainly is outside the scope of this patch
> series, so meanwhile this looks good to me.

Yeah, `set -e` is a gem of a command. Would be good to do later!

> > @@ -121,9 +133,16 @@ macos-*)
> >
> >       mkdir -p "$CUSTOM_PATH"
> >       wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
> > -     tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
> > +     tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d || {
> > +             echo >&2 "error: could not install perforce"
> > +             exit 1
> > +     }
> >       sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
>
> This line is a bit curious, it makes me wonder whether we want to
> tighten it as well now. Any reason why we wouldn't?

I asked Claud about it and it said this, does it make sense?

'wget/curl never set com.apple.quarantine in the first place (only
Safari/Finder/AirDrop do, via the LaunchServices quarantine API), so
xattr -d on these files always fails with "No such xattr." Tightening
it to fatal would break every macOS build, every time.'

> >       rm helix-core-server.tgz
>
> Nit: this is inconsistent with git-lfs, where we also have the call to
> rm as part of the condition chain.

Good point!



Harald

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Harald Nordgren]

> > >               wget --quiet \
> > >                    "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" &&
> > >               tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
> > >                       -C "$CUSTOM_PATH" --strip-components=1 \
> > >                       "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" &&
> > > -             rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" ||
> > > -             rm -f "$CUSTOM_PATH/git-lfs"
> > > +             rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" || {
> > > +                     echo >&2 "error: could not install git-lfs"
> > > +                     exit 1
> > > +             }
> >
> > This could all be a bit less convoluted if we had `set -e` in
> > "ci/lib.sh". But that certainly is outside the scope of this patch
> > series, so meanwhile this looks good to me.
>
> Yeah, `set -e` is a gem of a command. Would be good to do later!

But wait, we already have it in "ci/lib.sh"!


Harald

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Patrick Steinhardt]

On Mon, Oct 26, 2026 at 09:38:10AM +0200, Harald Nordgren wrote:
> > > @@ -121,9 +133,16 @@ macos-*)
> > >
> > >       mkdir -p "$CUSTOM_PATH"
> > >       wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
> > > -     tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
> > > +     tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d || {
> > > +             echo >&2 "error: could not install perforce"
> > > +             exit 1
> > > +     }
> > >       sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
> >
> > This line is a bit curious, it makes me wonder whether we want to
> > tighten it as well now. Any reason why we wouldn't?
> 
> I asked Claud about it and it said this, does it make sense?
> 
> 'wget/curl never set com.apple.quarantine in the first place (only
> Safari/Finder/AirDrop do, via the LaunchServices quarantine API), so
> xattr -d on these files always fails with "No such xattr." Tightening
> it to fatal would break every macOS build, every time.'

Well, if so we should be able to remove it without any detriment, right?
Maybe that could be part of a preparatory patch.

Patrick

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Patrick Steinhardt]

On Mon, Oct 26, 2026 at 09:41:45AM +0200, Harald Nordgren wrote:
> > > >               wget --quiet \
> > > >                    "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" &&
> > > >               tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
> > > >                       -C "$CUSTOM_PATH" --strip-components=1 \
> > > >                       "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" &&
> > > > -             rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" ||
> > > > -             rm -f "$CUSTOM_PATH/git-lfs"
> > > > +             rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" || {
> > > > +                     echo >&2 "error: could not install git-lfs"
> > > > +                     exit 1
> > > > +             }
> > >
> > > This could all be a bit less convoluted if we had `set -e` in
> > > "ci/lib.sh". But that certainly is outside the scope of this patch
> > > series, so meanwhile this looks good to me.
> >
> > Yeah, `set -e` is a gem of a command. Would be good to do later!
> 
> But wait, we already have it in "ci/lib.sh"!

Oh, I missed that!

The question then becomes whether the error message is worth it. In case
we see traces (`set -x`) I think it's somewhat pointless as you'd
trivially see the last failing command anyway, so you'd know why it has
failed.

Anyway, this is all a bit of a tangent, I don't think there's a strong
reason that this really must change.

Patrick

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Harald Nordgren]

> > > > This could all be a bit less convoluted if we had `set -e` in
> > > > "ci/lib.sh". But that certainly is outside the scope of this patch
> > > > series, so meanwhile this looks good to me.
> > >
> > > Yeah, `set -e` is a gem of a command. Would be good to do later!
> >
> > But wait, we already have it in "ci/lib.sh"!
>
> Oh, I missed that!
>
> The question then becomes whether the error message is worth it. In case
> we see traces (`set -x`) I think it's somewhat pointless as you'd
> trivially see the last failing command anyway, so you'd know why it has
> failed.

Yes, probably.

> Anyway, this is all a bit of a tangent, I don't think there's a strong
> reason that this really must change.

I'll take a look at what I can do without ballooning this topic.


Harald

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Harald Nordgren]

On Mon, Oct 26, 2026 at 10:07 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 26, 2026 at 09:38:10AM +0200, Harald Nordgren wrote:
> > > > @@ -121,9 +133,16 @@ macos-*)
> > > >
> > > >       mkdir -p "$CUSTOM_PATH"
> > > >       wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
> > > > -     tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
> > > > +     tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d || {
> > > > +             echo >&2 "error: could not install perforce"
> > > > +             exit 1
> > > > +     }
> > > >       sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
> > >
> > > This line is a bit curious, it makes me wonder whether we want to
> > > tighten it as well now. Any reason why we wouldn't?
> >
> > I asked Claud about it and it said this, does it make sense?
> >
> > 'wget/curl never set com.apple.quarantine in the first place (only
> > Safari/Finder/AirDrop do, via the LaunchServices quarantine API), so
> > xattr -d on these files always fails with "No such xattr." Tightening
> > it to fatal would break every macOS build, every time.'
>
> Well, if so we should be able to remove it without any detriment, right?
> Maybe that could be part of a preparatory patch.

Best way to find out is just to do it, I guess. I can include it in v3
and then you can trigger a build on GitLab with it? I'll let GitHub CI
pass before pushing anything out.


Harald

Re: [PATCH v2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Patrick Steinhardt]

On Mon, Oct 26, 2026 at 10:13:26AM +0200, Harald Nordgren wrote:
> On Mon, Oct 26, 2026 at 10:07 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, Oct 26, 2026 at 09:38:10AM +0200, Harald Nordgren wrote:
> > > > > @@ -121,9 +133,16 @@ macos-*)
> > > > >
> > > > >       mkdir -p "$CUSTOM_PATH"
> > > > >       wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
> > > > > -     tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
> > > > > +     tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d || {
> > > > > +             echo >&2 "error: could not install perforce"
> > > > > +             exit 1
> > > > > +     }
> > > > >       sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
> > > >
> > > > This line is a bit curious, it makes me wonder whether we want to
> > > > tighten it as well now. Any reason why we wouldn't?
> > >
> > > I asked Claud about it and it said this, does it make sense?
> > >
> > > 'wget/curl never set com.apple.quarantine in the first place (only
> > > Safari/Finder/AirDrop do, via the LaunchServices quarantine API), so
> > > xattr -d on these files always fails with "No such xattr." Tightening
> > > it to fatal would break every macOS build, every time.'
> >
> > Well, if so we should be able to remove it without any detriment, right?
> > Maybe that could be part of a preparatory patch.
> 
> Best way to find out is just to do it, I guess. I can include it in v3
> and then you can trigger a build on GitLab with it? I'll let GitHub CI
> pass before pushing anything out.

Sounds good, thanks.

Patrick

[PATCH v3 0/2] ci: fail the build if perforce, git-lfs or JGit fail to install

[Harald Nordgren via GitGitGadget]

Fail the build when perforce, git-lfs, or JGit fail to install, and drop the
warnings that used to fire on every platform regardless of whether it even
attempted the install.

Changes in v3:

 * New prep commit drops the xattr -d com.apple.quarantine call for the
   macOS perforce binaries, it's been a silent no-op since perforce's
   install moved off Homebrew in 2022, confirmed wget downloads never carry
   that attribute.
 * Drops the explicit echo "error: ..." / exit 1 blocks for perforce,
   git-lfs, and JGit, relying on set -e (already active via ci/lib.sh) to
   abort on the first failed install, matching how every other install step
   in this file already works.

Changes in v2:

 * Perforce, git-lfs, and JGit install failures now fail the build instead
   of being silently swallowed. Removed the presence checks and warnings at
   the end of the script, dead code once install failure is fatal.
 * Version output (p4d -V, git-lfs version, jgit version) moved inline right
   after each successful install.

Harald Nordgren (2):
  ci: drop quarantine removal for macos perforce binaries
  ci: fail the build if perforce, git-lfs, or JGit fail to install

 ci/install-dependencies.sh | 50 ++++++++++++--------------------------
 1 file changed, 15 insertions(+), 35 deletions(-)


base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2403%2FHaraldNordgren%2Fci-scope-optional-tool-warnings-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2403/HaraldNordgren/ci-scope-optional-tool-warnings-v3
Pull-Request: https://github.com/git/git/pull/2403

Range-diff vs v2:

 -:  ---------- > 1:  e85ce85d92 ci: drop quarantine removal for macos perforce binaries
 1:  dde1b38567 ! 2:  a57cfbb2a5 ci: fail the build if perforce, git-lfs, or JGit fail to install
     @@ Commit message
      
       ## ci/install-dependencies.sh ##
      @@ ci/install-dependencies.sh: ubuntu-*|i386/ubuntu-*|debian-*)
     + 		wget --quiet --directory-prefix="$CUSTOM_PATH" \
       			"$P4WHENCE/bin.linux26x86_64/p4d" \
       			"$P4WHENCE/bin.linux26x86_64/p4" &&
     - 		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
     +-		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
      -			rm -f "$CUSTOM_PATH/p4"
      -			rm -f "$CUSTOM_PATH/p4d"
     -+			echo >&2 "error: could not install perforce"
     -+			exit 1
     - 		}
     +-		}
     ++		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
      +		echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
      +		p4d -V
      +		echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
     @@ ci/install-dependencies.sh: ubuntu-*|i386/ubuntu-*|debian-*)
       			"git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" &&
      -		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" ||
      -		rm -f "$CUSTOM_PATH/git-lfs"
     -+		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" || {
     -+			echo >&2 "error: could not install git-lfs"
     -+			exit 1
     -+		}
     ++		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
      +		echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
      +		git-lfs version
       
       		wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit" &&
      -		chmod a+x "$CUSTOM_PATH/jgit" ||
      -		rm -f "$CUSTOM_PATH/jgit"
     -+		chmod a+x "$CUSTOM_PATH/jgit" || {
     -+			echo >&2 "error: could not install JGit"
     -+			exit 1
     -+		}
     ++		chmod a+x "$CUSTOM_PATH/jgit"
      +		echo "$(tput setaf 6)JGit Version$(tput sgr0)"
      +		jgit version
       		;;
       	esac
       	;;
      @@ ci/install-dependencies.sh: macos-*)
     - 
     - 	mkdir -p "$CUSTOM_PATH"
       	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
     --	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
     -+	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d || {
     -+		echo >&2 "error: could not install perforce"
     -+		exit 1
     -+	}
     - 	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
     + 	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
       	rm helix-core-server.tgz
      +	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
      +	p4d -V

-- 
gitgitgadget

[PATCH v3 1/2] ci: drop quarantine removal for macos perforce binaries

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

A file downloaded with wget never gets com.apple.quarantine set,
only Safari, Finder, and AirDrop apply that attribute through the
LaunchServices quarantine API.

5ed9fc3fc8 (ci: prevent `perforce` from being quarantined,
2020-02-27) added this to stop p4d -V from hanging on a GUI
confirmation dialog that never comes on a CI box, back when perforce
was installed via Homebrew's --no-quarantine cask flag. When
49af448197 (ci: reintroduce prevention from perforce being
quarantined in macOS, 2022-05-12) replaced that with a plain wget and
tar download, it kept an xattr -d call as a precaution while noting
that call already does nothing on GitHub Actions. It has been a
silent no-op ever since.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 ci/install-dependencies.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index d57dce5663..203e8401cf 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -122,7 +122,6 @@ macos-*)
 	mkdir -p "$CUSTOM_PATH"
 	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
 	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
-	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
 	rm helix-core-server.tgz
 
 	case "$jobname" in
-- 
gitgitgadget

[PATCH v3 2/2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

A failed install of perforce, git-lfs, or JGit was silently
tolerated. The corresponding tests would just skip and the build
stayed green, so a broken download could go unnoticed indefinitely.

Fail the build immediately instead on the platforms that install
these tools. The presence checks and warnings at the end of the
script are gone too, since we already know by then that everything
installed or the build wouldn't have gotten this far.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 ci/install-dependencies.sh | 49 ++++++++++++--------------------------
 1 file changed, 15 insertions(+), 34 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 203e8401cf..7c3c6ad434 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -94,22 +94,25 @@ ubuntu-*|i386/ubuntu-*|debian-*)
 		wget --quiet --directory-prefix="$CUSTOM_PATH" \
 			"$P4WHENCE/bin.linux26x86_64/p4d" \
 			"$P4WHENCE/bin.linux26x86_64/p4" &&
-		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
-			rm -f "$CUSTOM_PATH/p4"
-			rm -f "$CUSTOM_PATH/p4d"
-		}
+		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
+		echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
+		p4d -V
+		echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
+		p4 -V
 
 		wget --quiet \
 		     "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" &&
 		tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
 			-C "$CUSTOM_PATH" --strip-components=1 \
 			"git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" &&
-		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" ||
-		rm -f "$CUSTOM_PATH/git-lfs"
+		rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
+		echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
+		git-lfs version
 
 		wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit" &&
-		chmod a+x "$CUSTOM_PATH/jgit" ||
-		rm -f "$CUSTOM_PATH/jgit"
+		chmod a+x "$CUSTOM_PATH/jgit"
+		echo "$(tput setaf 6)JGit Version$(tput sgr0)"
+		jgit version
 		;;
 	esac
 	;;
@@ -123,6 +126,10 @@ macos-*)
 	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
 	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
 	rm helix-core-server.tgz
+	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
+	p4d -V
+	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
+	p4 -V
 
 	case "$jobname" in
 	osx-meson)
@@ -170,30 +177,4 @@ 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
-
-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
-
 end_group "Install dependencies"
-- 
gitgitgadget

Re: [PATCH v3 2/2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Patrick Steinhardt]

On Mon, Oct 26, 2026 at 09:54:25AM +0000, Harald Nordgren via GitGitGadget wrote:
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 203e8401cf..7c3c6ad434 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -94,22 +94,25 @@ ubuntu-*|i386/ubuntu-*|debian-*)
>  		wget --quiet --directory-prefix="$CUSTOM_PATH" \
>  			"$P4WHENCE/bin.linux26x86_64/p4d" \
>  			"$P4WHENCE/bin.linux26x86_64/p4" &&
> -		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
> -			rm -f "$CUSTOM_PATH/p4"
> -			rm -f "$CUSTOM_PATH/p4d"
> -		}
> +		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
> +		echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
> +		p4d -V
> +		echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
> +		p4 -V

Isn't the && chain completely useless now, both here and in the other
cases? We let `set -e` do its thing, but we don't have any error
handling for the whole block anymore after we dropped the error message.

Patrick

Re: [PATCH v3 1/2] ci: drop quarantine removal for macos perforce binaries

[Patrick Steinhardt]

On Mon, Oct 26, 2026 at 09:54:24AM +0000, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> 
> A file downloaded with wget never gets com.apple.quarantine set,
> only Safari, Finder, and AirDrop apply that attribute through the
> LaunchServices quarantine API.

This is the explanation why we can remove the flag, but we haven't even
talked about the observation or what we want to do.

> 5ed9fc3fc8 (ci: prevent `perforce` from being quarantined,
> 2020-02-27) added this to stop p4d -V from hanging on a GUI

Nit: you start talking about "this" without even saying what "this" is.

> confirmation dialog that never comes on a CI box, back when perforce
> was installed via Homebrew's --no-quarantine cask flag. When
> 49af448197 (ci: reintroduce prevention from perforce being
> quarantined in macOS, 2022-05-12) replaced that with a plain wget and
> tar download, it kept an xattr -d call as a precaution while noting
> that call already does nothing on GitHub Actions. It has been a
> silent no-op ever since.

So the question is what this does on GitLab CI. [1] will answer that
question once the pipeline has succeeded.

Patrick

[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/714

Re: [PATCH v3 2/2] ci: fail the build if perforce, git-lfs, or JGit fail to install

[Harald Nordgren]

On Mon, Oct 26, 2026 at 1:08 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 26, 2026 at 09:54:25AM +0000, Harald Nordgren via GitGitGadget wrote:
> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> > index 203e8401cf..7c3c6ad434 100755
> > --- a/ci/install-dependencies.sh
> > +++ b/ci/install-dependencies.sh
> > @@ -94,22 +94,25 @@ ubuntu-*|i386/ubuntu-*|debian-*)
> >               wget --quiet --directory-prefix="$CUSTOM_PATH" \
> >                       "$P4WHENCE/bin.linux26x86_64/p4d" \
> >                       "$P4WHENCE/bin.linux26x86_64/p4" &&
> > -             chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
> > -                     rm -f "$CUSTOM_PATH/p4"
> > -                     rm -f "$CUSTOM_PATH/p4d"
> > -             }
> > +             chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
> > +             echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
> > +             p4d -V
> > +             echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
> > +             p4 -V
>
> Isn't the && chain completely useless now, both here and in the other
> cases? We let `set -e` do its thing, but we don't have any error
> handling for the whole block anymore after we dropped the error message.

That's true! It would open up a lot of simplifications on the regular
git tests if we could drop '&&' there too :)


Harald

Re: [PATCH v3 1/2] ci: drop quarantine removal for macos perforce binaries

[Harald Nordgren]

> So the question is what this does on GitLab CI. [1] will answer that
> question once the pipeline has succeeded.
>
> Patrick
>
> [1]: https://gitlab.com/gitlab-org/git/-/merge_requests/714

Seems GitLab CI passed.


Harald

Re: [PATCH v3 1/2] ci: drop quarantine removal for macos perforce binaries

[Harald Nordgren]

> > A file downloaded with wget never gets com.apple.quarantine set,
> > only Safari, Finder, and AirDrop apply that attribute through the
> > LaunchServices quarantine API.
>
> This is the explanation why we can remove the flag, but we haven't even
> talked about the observation or what we want to do.

I'll take a look at that.

> > 5ed9fc3fc8 (ci: prevent `perforce` from being quarantined,
> > 2020-02-27) added this to stop p4d -V from hanging on a GUI
>
> Nit: you start talking about "this" without even saying what "this" is.

Ok.


Harald

Re: [PATCH v3 1/2] ci: drop quarantine removal for macos perforce binaries

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

>> > A file downloaded with wget never gets com.apple.quarantine set,
>> > only Safari, Finder, and AirDrop apply that attribute through the
>> > LaunchServices quarantine API.
>>
>> This is the explanation why we can remove the flag, but we haven't even
>> talked about the observation or what we want to do.
>
> I'll take a look at that.
>
>> > 5ed9fc3fc8 (ci: prevent `perforce` from being quarantined,
>> > 2020-02-27) added this to stop p4d -V from hanging on a GUI
>>
>> Nit: you start talking about "this" without even saying what "this" is.
>
> Ok.

Will look forward to seeing this step updated.  Nevertheless, thanks
for finding why we can remove this line and making it into a
separate commit.