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

[Patrick Steinhardt <ps@pks.im>]

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