Skip to contentSkip to conversation

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

PATCH In review Harald Nordgren via GitGitGadget 4 participants 29 messages Sep 12 – Oct 27

lore
Share & tools
lore

Harald Nordgren via GitGitGadget

author
#0 lore raw

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

From: Harald Nordgren <[email protected]>

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 <[email protected]>

---
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(-)
ci/install-dependencies.sh +31 -23
index 2f61fbb07c..a68cec64b4 100755
@@ -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

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 11 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

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.

Quoted 8 lines
Signed-off-by Harald Nordgren <[email protected]>

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

Quoted 75 lines

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(-)
ci/install-dependencies.sh +31 -23
index 2f61fbb07c..a68cec64b4 100755
@@ -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

#2 lore raw
In reply to Harald Nordgren via GitGitGadget
Quoted 54 lines · Harald Nordgren via GitGitGadget
ci/install-dependencies.sh +22 -22
index 2f61fbb07c..a68cec64b4 100755
@@ -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"

ci/install-dependencies.sh Documentation) · line 206

Quoted 10 lines
ci/install-dependencies.sh +9 -1
-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

#3 lore raw
Quoted 10 lines

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.

Quoted 5 lines

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

author
#4 lore raw

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

In reply to Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

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 <[email protected]>

---
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 <[email protected]>
      
       ## 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 <[email protected]>
      
       ## 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(-)
ci/install-dependencies.sh +26 -33
index d57dce5663..dad35db78b 100755
@@ -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

Signature · 1 line
gitgitgadget
#5 lore raw

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

Quoted 22 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

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 <[email protected]>

---
...

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

ci/install-dependencies.sh ubuntu-*|i386/ubuntu-*|debian-*) · line 100

Quoted 8 lines
ci/install-dependencies.sh +2 -2
@@ -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).

ci/install-dependencies.sh macos-*) · line 141

Quoted 11 lines
ci/install-dependencies.sh +4 -1
@@ -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...

Quoted 9 lines
ci/install-dependencies.sh +4 -0
+	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)
 	;;

ci/install-dependencies.sh Documentation) · line 199

Quoted 10 lines
ci/install-dependencies.sh +0 -7
 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.

Quoted 3 lines

end_group "Install dependencies"

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e

#6 lore raw

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

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

Thanks for looking at this!

Harald

#7 lore raw

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

Quoted 1 line · Harald Nordgren

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.

#8 lore raw

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

In reply to Harald Nordgren via GitGitGadget
Quoted 6 lines · Harald Nordgren via GitGitGadget

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!

Quoted 7 lines
ci/install-dependencies.sh +0 -0
index d57dce5663..dad35db78b 100755
@@ -95,21 +95,33 @@ ubuntu-*|i386/ubuntu-*|debian-*)
 			"$P4WHENCE/bin.linux26x86_64/p4d" \
 			"$P4WHENCE/bin.linux26x86_64/p4" &&

ci/install-dependencies.sh ubuntu-*|i386/ubuntu-*|debian-*) · line 104

Quoted 10 lines
ci/install-dependencies.sh +6 -2
 		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.

ci/install-dependencies.sh ubuntu-*|i386/ubuntu-*|debian-*) · line 114

Quoted 11 lines
ci/install-dependencies.sh +4 -2
 		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.

ci/install-dependencies.sh macos-*) · line 140

Quoted 10 lines
ci/install-dependencies.sh +4 -1
@@ -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?

ci/install-dependencies.sh macos-*) · line 141

Quoted 1 line
ci/install-dependencies.sh +0 -0
 	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

#9 lore raw

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

Quoted 12 lines

ci/install-dependencies.sh ubuntu-*|i386/ubuntu-*|debian-*) · line 114

Quoted 11 lines
              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" &&
ci/install-dependencies.sh +4 -2
-             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
+             }
Quoted 3 lines

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!

Quoted 13 lines

ci/install-dependencies.sh macos-*) · line 140

Quoted 10 lines
ci/install-dependencies.sh +4 -1
@@ -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.'

Quoted 4 lines

ci/install-dependencies.sh macos-*) · line 141

Quoted 1 line
ci/install-dependencies.sh +0 -0
      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

#10 lore raw

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

Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
              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" &&
ci/install-dependencies.sh +4 -2
-             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
+             }
Quoted 5 lines
Quoted 3 lines

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

#11 lore raw

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

Quoted 12 lines · Harald Nordgren
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
              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" &&
ci/install-dependencies.sh +4 -2
-             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
+             }
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

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

#12 lore raw

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

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

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!

Quoted 8 lines
Quoted 1 line

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.

Quoted 2 lines

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

#13 lore raw

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

In reply to Harald Nordgren
Quoted 14 lines · Harald Nordgren
Quoted 13 lines

ci/install-dependencies.sh macos-*) · line 140

Quoted 10 lines
ci/install-dependencies.sh +4 -1
@@ -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?

Quoted 6 lines

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

#14 lore raw

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

Quoted 18 lines · Patrick Steinhardt
Quoted 16 lines · Harald Nordgren
Quoted 13 lines

ci/install-dependencies.sh macos-*) · line 140

Quoted 10 lines
ci/install-dependencies.sh +4 -1
@@ -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?

Quoted 7 lines
Quoted 4 lines

'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

#15 lore raw

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

Quoted 24 lines · Harald Nordgren
Quoted 23 lines · Patrick Steinhardt
Quoted 20 lines · Harald Nordgren
Quoted 13 lines

ci/install-dependencies.sh macos-*) · line 140

Quoted 10 lines
ci/install-dependencies.sh +4 -1
@@ -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.'

Quoted 6 lines
Quoted 2 lines

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

author
#16 lore raw

v3 cover ci: fail the build if perforce, git-lfs or JGit fail to install

In reply to 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
Signature · 1 line
gitgitgadget
author
#17 lore raw

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

From: Harald Nordgren <[email protected]>

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 <[email protected]>
---
 ci/install-dependencies.sh | 1 -
 1 file changed, 1 deletion(-)
ci/install-dependencies.sh +0 -1
index d57dce5663..203e8401cf 100755
@@ -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
Signature · 1 line
gitgitgadget
#18 lore raw

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

Quoted 5 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

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.

Quoted 2 lines

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.

Quoted 7 lines

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

#20 lore raw

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

In reply to Patrick Steinhardt
Quoted 6 lines
Quoted 3 lines

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.

Quoted 4 lines
Quoted 2 lines

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

#21 lore raw

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

Quoted 7 lines · Harald Nordgren
Quoted 6 lines
Quoted 3 lines

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.

Quoted 8 lines

I'll take a look at that.

Quoted 4 lines
Quoted 2 lines

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.

author
#22 lore raw

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

In reply to Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

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 <[email protected]>
---
 ci/install-dependencies.sh | 49 ++++++++++++--------------------------
 1 file changed, 15 insertions(+), 34 deletions(-)
ci/install-dependencies.sh +15 -34
index 203e8401cf..7c3c6ad434 100755
@@ -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"
Signature · 1 line
gitgitgadget
#23 lore raw

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

Quoted 7 lines · Harald Nordgren via GitGitGadget
ci/install-dependencies.sh +0 -0
index 203e8401cf..7c3c6ad434 100755
@@ -94,22 +94,25 @@ ubuntu-*|i386/ubuntu-*|debian-*)
 		wget --quiet --directory-prefix="$CUSTOM_PATH" \
 			"$P4WHENCE/bin.linux26x86_64/p4d" \

ci/install-dependencies.sh ubuntu-*|i386/ubuntu-*|debian-*) · line 101

Quoted 10 lines
ci/install-dependencies.sh +5 -4
 			"$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

#24 lore raw

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

Quoted 15 lines · Patrick Steinhardt
Quoted 13 lines · Harald Nordgren via GitGitGadget
ci/install-dependencies.sh +1 -4
index 203e8401cf..7c3c6ad434 100755
@@ -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"
Quoted 8 lines

ci/install-dependencies.sh ubuntu-*|i386/ubuntu-*|debian-*) · line 101

Quoted 4 lines
ci/install-dependencies.sh +4 -0
+             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

author
#25 lore raw

v4 cover ci: fail the build if perforce, git-lfs or JGit fail to install

In reply to 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 v4:

* Drops the && chaining between the wget/tar/chmod install steps for perforce, git-lfs, and JGit, since under set -e a failing command inside an && chain is exempt from aborting the script unless it's the last one in the chain, so the chain was silently letting an earlier failure fall through instead of stopping the build.
* Reworded the quarantine-removal commit to lead with the observation (the call does nothing and can go) before the history, fixed a dangling "this" reference, and added that the same holds on GitLab CI's macOS runners, not just GitHub Actions.

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 | 64 +++++++++++++-------------------------
 1 file changed, 22 insertions(+), 42 deletions(-)

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

Range-diff vs v3:

1: e85ce85d92 ! 1: c1ee3fcb7d ci: drop quarantine removal for macos perforce binaries
@@ Metadata
## Commit message ##
ci: drop quarantine removal for macos perforce binaries

- A file downloaded with wget never gets com.apple.quarantine set,
- only Safari, Finder, and AirDrop apply that attribute through the
- LaunchServices quarantine API.
+ The xattr -d call that strips com.apple.quarantine from the macOS
+ perforce binaries has been doing nothing for years, and can simply
+ go.

5ed9fc3fc8 (ci: prevent perforce from being quarantined,
- 2020-02-27) added this to stop p4d -V from hanging on a GUI
+ 2020-02-27) added that call 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.
+ quarantined in macOS, 2022-05-12) replaced that with a plain wget
+ and tar download, it kept the xattr -d call as a precaution, while
+ noting that it already does nothing on GitHub Actions.
+
+ A file downloaded with wget never gets com.apple.quarantine set in
+ the first place, only Safari, Finder, and AirDrop apply that
+ attribute through the LaunchServices quarantine API. The same holds
+ on GitLab CI's macOS runners, so the call has been a silent no-op
+ everywhere since 2022.

Signed-off-by Harald Nordgren <[email protected]>

2: a57cfbb2a5 ! 2: c6dda879aa 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" &&
     +-			"$P4WHENCE/bin.linux26x86_64/p4" &&
      -		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || {
      -			rm -f "$CUSTOM_PATH/p4"
      -			rm -f "$CUSTOM_PATH/p4d"
      -		}
     ++			"$P4WHENCE/bin.linux26x86_64/p4"
      +		chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
      +		echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
      +		p4d -V
     @@ ci/install-dependencies.sh: ubuntu-*|i386/ubuntu-*|debian-*)
      +		p4 -V
       
       		wget --quiet \
     - 		     "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" &&
     +-		     "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" &&
     ++		     "$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" &&
     +-			"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"
     +-
     +-		wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit" &&
     +-		chmod a+x "$CUSTOM_PATH/jgit" ||
     +-		rm -f "$CUSTOM_PATH/jgit"
     ++			"git-lfs-$LINUX_GIT_LFS_VERSION/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"
     ++
     ++		wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit"
      +		chmod a+x "$CUSTOM_PATH/jgit"
      +		echo "$(tput setaf 6)JGit Version$(tput sgr0)"
      +		jgit version
     @@ ci/install-dependencies.sh: ubuntu-*|i386/ubuntu-*|debian-*)
       	esac
       	;;
      @@ ci/install-dependencies.sh: macos-*)
     - 	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
     - 	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
     + 	brew link --force gettext
     + 
     + 	mkdir -p "$CUSTOM_PATH"
     +-	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
     +-	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
     ++	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
Signature · 1 line
gitgitgadget
author
#26 lore raw

PATCH v4 1/2 ci: drop quarantine removal for macos perforce binaries

From: Harald Nordgren <[email protected]>

The xattr -d call that strips com.apple.quarantine from the macOS perforce binaries has been doing nothing for years, and can simply go.

5ed9fc3fc8 (ci: prevent perforce from being quarantined, 2020-02-27) added that call 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 the xattr -d call as a precaution, while noting that it already does nothing on GitHub Actions.

A file downloaded with wget never gets com.apple.quarantine set in the first place, only Safari, Finder, and AirDrop apply that attribute through the LaunchServices quarantine API. The same holds on GitLab CI's macOS runners, so the call has been a silent no-op everywhere since 2022.

Signed-off-by Harald Nordgren <[email protected]>
---
 ci/install-dependencies.sh | 1 -
 1 file changed, 1 deletion(-)
ci/install-dependencies.sh +0 -1
index d57dce5663..203e8401cf 100755
@@ -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
Signature · 1 line
gitgitgadget
author
#27 lore raw

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

In reply to Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

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 <[email protected]>
---
 ci/install-dependencies.sh | 63 +++++++++++++-------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)
ci/install-dependencies.sh +22 -41
index 203e8401cf..d35c64102e 100755
@@ -93,23 +93,26 @@ 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"
-		}
+			"$P4WHENCE/bin.linux26x86_64/p4"
+		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" &&
+		     "$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"
-
-		wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit" &&
-		chmod a+x "$CUSTOM_PATH/jgit" ||
-		rm -f "$CUSTOM_PATH/jgit"
+			"git-lfs-$LINUX_GIT_LFS_VERSION/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"
+		echo "$(tput setaf 6)JGit Version$(tput sgr0)"
+		jgit version
 		;;
 	esac
 	;;

@@ -120,9 +123,13 @@ macos-*)
 	brew link --force gettext
 
 	mkdir -p "$CUSTOM_PATH"
-	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&
-	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
+	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"
Signature · 1 line
gitgitgadget
#28 lore raw

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

In reply to Harald Nordgren via GitGitGadget
Quoted 6 lines · 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 v4:

Quoted 9 lines

* Drops the && chaining between the wget/tar/chmod install steps for perforce, git-lfs, and JGit, since under set -e a failing command inside an && chain is exempt from aborting the script unless it's the last one in the chain, so the chain was silently letting an earlier failure fall through instead of stopping the build.
* Reworded the quarantine-removal commit to lead with the observation (the call does nothing and can go) before the history, fixed a dangling "this" reference, and added that the same holds on GitLab CI's macOS runners, not just GitHub Actions.

Thanks, this version looks good to me.

Patrick