Hi Patrick,
On Fri, 25 Apr 2025, Patrick Steinhardt wrote:
> On Thu, Apr 24, 2025 at 04:10:47PM -0700, Junio C Hamano wrote:
> > The ci/install-dependencies.sh script used in a very early phase of
> > our CI jobs downloads Perforce, Git-LFS, and JGit, used for running
> > the test scripts. The test framework is prepared to properly skip
> > the tests that depend on these external software, but the CI script
> > is unnecessarily strict (due to its use of "set -e" in ci/lib.sh)
> > and fails the entire CI run before even starting to test the rest of
> > the system.
> >
> > Notice a failure to download to any of these external software, but
> > keep going. We need to be careful about cleaning after a failed
> > wget, as a later part of the script that does:
> >
> > 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
> >
> > will (surprise!) succeed running "type jgit", and then fail with
> > "jgit version", taking the whole thing down due to "set -e".
>
> Yeah, I think this is a sensible direction to go. It is unfortunate that
> this may lead to silent breakage of these dependencies unless somebody
> explicitly looks for those warnings. But that feels like the lesser evil
> compared to failing the whole pipeline.
>
> > Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> > ---
> > ci/install-dependencies.sh | 31 ++++++++++++++++++++++---------
> > 1 file changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> > index 0df74610d0..e51304c3b0 100755
> > --- a/ci/install-dependencies.sh
> > +++ b/ci/install-dependencies.sh
> > @@ -66,16 +66,29 @@ ubuntu-*|i386/ubuntu-*|debian-*)
> > mkdir --parents "$CUSTOM_PATH"
> >
> > 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"
> > -
> > - wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
> > + "$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 "P4 download (optional) failed"
> > + }
>
> I think it would be preferable to only handle failure of wget as chmod
> shouldn't ever fail if wget was successful. The same is true for the
> other downloads -- let's be as strict as possible but allow failure of
> those actions that depend on the network.
That is true. It would probably also make sense to mark the message as a
`::warning::` on GitHub (and the equivalent on GitLab), so that it is
shown a bit more prominently in the CI summary.
Further, as per Matthias Sohn's (i.e. the JGit maintainer's)
recommendation at
https://discord.com/channels/1042895022950994071/1364872237710184520/1364886912044765216,
the JGit download link in particular should probably be changed to
https://repo1.maven.org/maven2/org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh
(which would work around the CI failures as well and could take the
pressure off of working on more graceful dependency management in Git's
CI, but then, we don't need more time to discuss the patch in this here
thread because it already was fast-tracked to `master`).
Ciao,
Johannes
