[PATCH] ci: make perforce installation optional in macOS

[PATCH] ci: make perforce installation optional in macOS

[Carlo Marcelo Arenas Belón]

Using brew to install perforce has several documented[1,2,3,4] edge
cases that make it fail periodically, so if problems were found while
installing it, just continue without it.

This means that until the problem is solved all perforce tests will be
skipped in macOS, but they are still most likely covered by the other
unaffected runs and will be covered again once the issue solves itself.

1 0eb3671ed96 (ci(osx): use new location of the `perforce` cask, 2019-10-23)
2 5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27)
3 3831132ace6 (ci/install-depends: attempt to fix "brew cask" stuff, 2021-01-14)
4 https://lore.kernel.org/git/cover-0.2-00000000000-20220421T124225Z-avarab@gmail.com/
---
This is based on master and can merge upwards except with seen where it
conflicts with the recently added ab/ci-osx-loosen-package-requirement
which it is meant to replace.

A successful sample run when merged with master available in :

  https://github.com/carenas/git/actions/runs/2204519374

 ci/install-dependencies.sh | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
wi
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dbcebad2fb2..53da1afd9f3 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -43,7 +43,7 @@ macos-latest)
 		git -C "$cask_repo" pull --no-stat --ff-only &&
 		brew install --cask --no-quarantine perforce
 	} ||
-	brew install homebrew/cask/perforce
+	brew install homebrew/cask/perforce || true
 
 	if test -n "$CC_PACKAGE"
 	then
-- 
2.36.0.266.g59f845bde02

Re: [PATCH] ci: make perforce installation optional in macOS

[Eric Sunshine]

On Thu, Apr 21, 2022 at 6:55 PM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> Using brew to install perforce has several documented[1,2,3,4] edge
> cases that make it fail periodically, so if problems were found while
> installing it, just continue without it.
>
> This means that until the problem is solved all perforce tests will be
> skipped in macOS, but they are still most likely covered by the other
> unaffected runs and will be covered again once the issue solves itself.
>
> 1 0eb3671ed96 (ci(osx): use new location of the `perforce` cask, 2019-10-23)
> 2 5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27)
> 3 3831132ace6 (ci/install-depends: attempt to fix "brew cask" stuff, 2021-01-14)
> 4 https://lore.kernel.org/git/cover-0.2-00000000000-20220421T124225Z-avarab@gmail.com/
> ---

Missing sign-off.

Re: [PATCH] ci: make perforce installation optional in macOS

[Eric Sunshine]

On Thu, Apr 21, 2022 at 6:55 PM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> Using brew to install perforce has several documented[1,2,3,4] edge
> cases that make it fail periodically, so if problems were found while
> installing it, just continue without it.
>
> This means that until the problem is solved all perforce tests will be
> skipped in macOS, but they are still most likely covered by the other
> unaffected runs and will be covered again once the issue solves itself.
>
> 1 0eb3671ed96 (ci(osx): use new location of the `perforce` cask, 2019-10-23)
> 2 5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27)
> 3 3831132ace6 (ci/install-depends: attempt to fix "brew cask" stuff, 2021-01-14)
> 4 https://lore.kernel.org/git/cover-0.2-00000000000-20220421T124225Z-avarab@gmail.com/
> ---
> This is based on master and can merge upwards except with seen where it
> conflicts with the recently added ab/ci-osx-loosen-package-requirement
> which it is meant to replace.
>
> A successful sample run when merged with master available in :
>
>   https://github.com/carenas/git/actions/runs/2204519374
>
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> @@ -43,7 +43,7 @@ macos-latest)
> -       brew install homebrew/cask/perforce
> +       brew install homebrew/cask/perforce || true

You do get a vaguely alarming error message[1] with the solution
implemented by this patch:

    ci/install-dependencies.sh: line 81: type: p4d: not found

However, that's an unrelated and existing minor mistake (outside of
the scope of this fix, probably). It should be using `command -v`
rather than `type` in the couple conditionals at the bottom of the
script.

[1]: https://github.com/carenas/git/runs/6119943232?check_suite_focus=true#step:3:116

[PATCH v2 0/2] ci: avoid perforce/brew issues affecting macOS

[Carlo Marcelo Arenas Belón]

This series is a simpler alternative to ab/ci-osx-loosen-package-requirement
which while it doesn't address the underlying issue, avoids the CI failures
in macOS as well.

For an example of a successful run with it merged with next see:

  https://github.com/carenas/git/actions/runs/2205005763

Carlo Marcelo Arenas Belón (2):
  ci: make failure to find perforce more user friendly
  ci: make perforce installation optional in macOS

 ci/install-dependencies.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.36.0.266.g59f845bde02

[PATCH v2 1/2] ci: make failure to find perforce more user friendly

[Carlo Marcelo Arenas Belón]

In preparation for a future change that will make perforce installation
optional in macOS, make sure that the check for it is done without
triggering scary looking errors and add a user friendly message instead.

Only one invocation of type is changed as that is what is only needed
for the expected future use case, and because `type` is recommended in
the CodingGuidelines, so changing that recommendation or a more complex
change has been specifically punted.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 ci/install-dependencies.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dbcebad2fb2..6de20108775 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -78,12 +78,14 @@ linux-gcc-default)
 	;;
 esac
 
-if type p4d >/dev/null && type p4 >/dev/null
+if command -v p4d >/dev/null && type p4 >/dev/null
 then
 	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
 	p4d -V | grep Rev.
 	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
 	p4 -V | grep Rev.
+else
+	echo "WARNING: perforce wasn't installed, see above for clues why"
 fi
 if type git-lfs >/dev/null
 then
-- 
2.36.0.266.g59f845bde02

Re: [PATCH v2 1/2] ci: make failure to find perforce more user friendly

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> In preparation for a future change that will make perforce installation
> optional in macOS, make sure that the check for it is done without
> triggering scary looking errors and add a user friendly message instead.
>
> Only one invocation of type is changed as that is what is only needed
> for the expected future use case, and because `type` is recommended in
> the CodingGuidelines, so changing that recommendation or a more complex
> change has been specifically punted.

This may be in the "POSIX may say this but the real world may not
work that way" territory.  As far as I can tell, "command -v" [*1*]
and "type" [*2*] both ought to give diagnostic messages to their
standard error stream, and they both should signal an error with
non-zero exit status.  It may be that the shell implementation you
have tried had "command -v" that is less noisy than "type" when
given a command that is not installed, but I wonder if the next
shell implementation you find has "command -v" that is as noisy and
scary as "type", in which case this patch amounts to a no-op.

I wonder if "type p4d >/dev/null 2>/dev/null" (or "command -v" with
the same) is a more futureproof fix.


[References]

*1* https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html
*2* https://pubs.opengroup.org/onlinepubs/9699919799/utilities/type.html

> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  ci/install-dependencies.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index dbcebad2fb2..6de20108775 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -78,12 +78,14 @@ linux-gcc-default)
>  	;;
>  esac
>  
> -if type p4d >/dev/null && type p4 >/dev/null
> +if command -v p4d >/dev/null && type p4 >/dev/null
>  then
>  	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
>  	p4d -V | grep Rev.
>  	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
>  	p4 -V | grep Rev.
> +else
> +	echo "WARNING: perforce wasn't installed, see above for clues why"
>  fi
>  if type git-lfs >/dev/null
>  then

Re: [PATCH v2 1/2] ci: make failure to find perforce more user friendly

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This may be in the "POSIX may say this but the real world may not
> work that way" territory.  As far as I can tell, "command -v" [*1*]
> and "type" [*2*] both ought to give diagnostic messages to their
> standard error stream, and they both should signal an error with
> non-zero exit status.  It may be that the shell implementation you
> have tried had "command -v" that is less noisy than "type" when
> given a command that is not installed, but I wonder if the next
> shell implementation you find has "command -v" that is as noisy and
> scary as "type", in which case this patch amounts to a no-op.
>
> I wonder if "type p4d >/dev/null 2>/dev/null" (or "command -v" with
> the same) is a more futureproof fix.

So, how about replacing it with something like this?

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
From: Carlo Marcelo Arenas Belón <carenas@gmail.com>

In preparation for a future change that will make perforce installation
optional in macOS, make sure that the check for it is done without
triggering scary looking errors and add a user friendly message instead.

All other existing uses of 'type <cmd>' in our shell scripts that
check the availability of a command <cmd> send both standard output
and error stream to /dev/null to squelch "<cmd> not found" diagnostic
output, but this script left the standard error stream shown.

Redirect it just like everybody else to squelch this error message that
we fully expect to see.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dbcebad2fb..e598dc28df 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -78,14 +78,16 @@ linux-gcc-default)
 	;;
 esac
 
-if type p4d >/dev/null && type p4 >/dev/null
+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 | grep Rev.
 	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
 	p4 -V | grep Rev.
+else
+	echo "WARNING: perforce wasn't installed, see above for clues why"
 fi
-if type git-lfs >/dev/null
+if type git-lfs >/dev/null 2>&1
 then
 	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
 	git-lfs version
-- 
2.36.0-184-gca3de164ba

Re: [PATCH v2 1/2] ci: make failure to find perforce more user friendly

[Carlo Arenas]

On Fri, Apr 22, 2022 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> So, how about replacing it with something like this?

Agree 100%.

It would theoretically make the issue raised by Ævar of not knowing
when perforce was skipped slightly worse
but getting that fixed would seem like something that could be done in
a follow up.

I have to also admit, with all the on the fly changes to these same
files, it might be wiser to wait until later anyway.

Carlo

Re: [PATCH v2 1/2] ci: make failure to find perforce more user friendly

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> On Fri, Apr 22, 2022 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> So, how about replacing it with something like this?
>
> Agree 100%.
>
> It would theoretically make the issue raised by Ævar of not knowing
> when perforce was skipped slightly worse
> but getting that fixed would seem like something that could be done in
> a follow up.
>
> I have to also admit, with all the on the fly changes to these same
> files, it might be wiser to wait until later anyway.

Actually, I was thinking about taking these two (possibly with
Ævar's "download with https://, we are in 21st century" change to
make them 3-patch series) and fast-tracking.

The homebrew specific "packagers can take a bit of time to catch up
and we may see hash mismatch" and other tweaks Ævar has can come on
top once we see the basic "any parts of tests can fail and the rest
can still be tested, why does it have to be world-stopping if we
fail to install p4?" working.

Thanks.

Re: [PATCH v2 1/2] ci: make failure to find perforce more user friendly

[Carlo Arenas]

On Fri, Apr 22, 2022 at 4:58 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Arenas <carenas@gmail.com> writes:
>
> > I have to also admit, with all the on the fly changes to these same
> > files, it might be wiser to wait until later anyway.
>
> Actually, I was thinking about taking these two (possibly with
> Ævar's "download with https://, we are in 21st century" change to
> make them 3-patch series) and fast-tracking.

Sounds like a good plan to me, and unlikely to introduce much pain to
all other on the fly changes, but I think it is worth mentioning that
some kind soul did fix the mismatch in homebrew already, so there is
not really an urgent need for fast-tracking right now either.

Carlo

[PATCH v2 2/2] ci: make perforce installation optional in macOS

[Carlo Marcelo Arenas Belón]

Using brew to install perforce has several documented[1,2,3,4] edge
cases that make it fail periodically, so if problems were found while
installing it, just continue without it.

This means that until the problem is solved all perforce tests will be
skipped in macOS, but they are still most likely covered by the other
unaffected runs and will be covered again once the issue solves itself.

1 0eb3671ed96 (ci(osx): use new location of the `perforce` cask, 2019-10-23)
2 5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27)
3 3831132ace6 (ci/install-depends: attempt to fix "brew cask" stuff, 2021-01-14)
4 https://lore.kernel.org/git/cover-0.2-00000000000-20220421T124225Z-avarab@gmail.com/

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 6de20108775..aca01a414ab 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -43,7 +43,7 @@ macos-latest)
 		git -C "$cask_repo" pull --no-stat --ff-only &&
 		brew install --cask --no-quarantine perforce
 	} ||
-	brew install homebrew/cask/perforce
+	brew install homebrew/cask/perforce || true
 
 	if test -n "$CC_PACKAGE"
 	then
-- 
2.36.0.266.g59f845bde02

[PATCH v3 0/4] ci: avoid perforce/brew issues affecting macOS

[Carlo Marcelo Arenas Belón]

This series is an alternative to ab/ci-osx-loosen-package-requirement
and includes the parts proposed on it in v2 that didn't conflict.

The first patch is just a slightly modified version from the one
proposed by Junio and that adds also a message if git-lfs wasn't found
just like we do now for perforce while making those messages go to stderr.

The second patch replaces the original proposal with a more complicated
one that removes the use of brew as suggested by Ævar, it is also a
little bigger than it really needs to be just to make sure it won't
conflict with on the fly changes, and for the same reason uses some
hardcoded values that would be worth fixing after.

The third patch is only needed to avoid regressions with Azure Pipelines
which might be deprecated as proposed in other on the fly changes, so it
might not be needed in seen, but is still needed if this progresses
further first.  It was made independent so it could be easy to revert if
Azure support is going away just like TravisCI did.

The last patch was taken from Ævar's v2 and is the only one that will
require changes in other core on the fly but the conflict resolution is
trivial, and more importantly doesn't conflict when merged to seen in
the current spot this branch uses.

For an example of a "successful" run with it merged on top of seen
(with the previous iteration reverted) see:

  https://github.com/carenas/git/runs/6140236148

Note that perforce no longer fails because someone fixed the issue
with their Cask but still most of the new code is excercised correctly.

This introduces a problem identified by Ævar of making the CI runs less
consistent, as they might be skipping some of the perforce integrations
if installing that fails again, but the likelyhood of that failing has
been reduced by removing the inherent brew flakyness, and so improving
the UI so that it will be more visible when that happens is probably
something that can be addressed later, once all other CI related changes
are settled, or we could reintroduce the hard dependency on installing
perforce for macOS (just like it is done in Linux)

Carlo Marcelo Arenas Belón (3):
  ci: make failure to find perforce more user friendly
  ci: avoid brew for installing perforce
  ci: reintroduce prevention from perforce being quarantined in macOS

Ævar Arnfjörð Bjarmason (1):
  CI: use https, not http to download binaries from perforce.com

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

-- 
2.36.0.266.g59f845bde02

[PATCH v3 1/4] ci: make failure to find perforce more user friendly

[Carlo Marcelo Arenas Belón]

In preparation for a future change that will make perforce installation
optional in macOS, make sure that the check for it is done without
triggering scary looking errors and add a user friendly message instead.

All other existing uses of 'type <cmd>' in our shell scripts that
check the availability of a command <cmd> send both standard output
and error stream to /dev/null to squelch "<cmd> not found" diagnostic
output, but this script left the standard error stream shown.

Redirect it just like everybody else to squelch this error message that
we fully expect to see.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 ci/install-dependencies.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dbcebad2fb2..41e9290fbdd 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -78,15 +78,19 @@ linux-gcc-default)
 	;;
 esac
 
-if type p4d >/dev/null && type p4 >/dev/null
+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 | grep Rev.
 	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
 	p4 -V | grep Rev.
+else
+	echo "WARNING: perforce wasn't installed, see above for clues why" >2
 fi
-if type git-lfs >/dev/null
+if type git-lfs >/dev/null 2>&1
 then
 	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
 	git-lfs version
+else
+	echo "WARNING: git-lfs wasn't installed, see above for clues why" >2
 fi
-- 
2.36.0.266.g59f845bde02

Re: [PATCH v3 1/4] ci: make failure to find perforce more user friendly

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> -if type p4d >/dev/null && type p4 >/dev/null
> +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 | grep Rev.
>  	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
>  	p4 -V | grep Rev.
> +else
> +	echo "WARNING: perforce wasn't installed, see above for clues why" >2
>  fi
> -if type git-lfs >/dev/null
> +if type git-lfs >/dev/null 2>&1
>  then
>  	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
>  	git-lfs version
> +else
> +	echo "WARNING: git-lfs wasn't installed, see above for clues why" >2
>  fi

NO!  Why do we want to create a file whose name is "2" here?

Good that I caught them before I merged them to 'next'.

Will locally amend and see if it fixes CI failure in 'seen'

Thanks.

[PATCH v3 2/4] ci: avoid brew for installing perforce

[Carlo Marcelo Arenas Belón]

Perfoce's cask in brew is meant[1] to be used only by humans, so replace
its use from the CI with a scripted binary download which is less likely
to fail, as it is done in Linux.

Kept the logic together so it will be less likely to break when moved
around as on the fly code changes in this area are settled, at which
point it will also feasable to ammend it to avoid some of the hardcoded
values by using similar variables to the ones Linux does.

In that same line, a POSIX sh syntax is used instead of the similar one
used in Linux in preparation for an unrelated future change that might
change the shell currently configured for it.

This change reintroduces the risk that the installed binaries might not
work because of being quarantined that was fixed with 5ed9fc3fc86 (ci:
prevent `perforce` from being quarantined, 2020-02-27) but fixing that
now was also punted for simplicity and since the affected cloud provider
is scheduled to be retired with an on the fly change, but should be
addressed if that other change is not integrated further.

The discussion on the need to keep 2 radically different versions of
the binaries to be tested with Linux vs macOS or how to upgrade to
newer versions now that brew won't do that automatically for us has
been punted for now as well.  On that line the now obsolete comment
about it in lib.sh was originally being updated by this change but
created conflicts as it is moved around by other on the fly changes,
so will be addressed independently as well.

[1] https://github.com/Homebrew/homebrew-cask/pull/122347#discussion_r856026584

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 ci/install-dependencies.sh | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 41e9290fbdd..9da03350d09 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -37,13 +37,14 @@ macos-latest)
 	test -z "$BREW_INSTALL_PACKAGES" ||
 	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
-	brew install --cask --no-quarantine perforce || {
-		# Update the definitions and try again
-		cask_repo="$(brew --repository)"/Library/Taps/homebrew/homebrew-cask &&
-		git -C "$cask_repo" pull --no-stat --ff-only &&
-		brew install --cask --no-quarantine perforce
-	} ||
-	brew install homebrew/cask/perforce
+	mkdir -p $HOME/bin
+	(
+		cd $HOME/bin
+		wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" &&
+		tar -xf helix-core-server.tgz
+	)
+	PATH="$PATH:${HOME}/bin"
+	export PATH
 
 	if test -n "$CC_PACKAGE"
 	then
-- 
2.36.0.266.g59f845bde02

Re: [PATCH v3 2/4] ci: avoid brew for installing perforce

[Eric Sunshine]

On Sat, Apr 23, 2022 at 10:26 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> Perfoce's cask in brew is meant[1] to be used only by humans, so replace
> its use from the CI with a scripted binary download which is less likely
> to fail, as it is done in Linux.

s/Perfoce/Perforce/

> Kept the logic together so it will be less likely to break when moved
> around as on the fly code changes in this area are settled, at which
> point it will also feasable to ammend it to avoid some of the hardcoded
> values by using similar variables to the ones Linux does.

s/Kept/Keep/
s/also/also be/
s/feasable/feasible/
s/ammend/amend/

> In that same line, a POSIX sh syntax is used instead of the similar one
> used in Linux in preparation for an unrelated future change that might
> change the shell currently configured for it.
>
> This change reintroduces the risk that the installed binaries might not
> work because of being quarantined that was fixed with 5ed9fc3fc86 (ci:
> prevent `perforce` from being quarantined, 2020-02-27) but fixing that
> now was also punted for simplicity and since the affected cloud provider
> is scheduled to be retired with an on the fly change, but should be
> addressed if that other change is not integrated further.
>
> The discussion on the need to keep 2 radically different versions of
> the binaries to be tested with Linux vs macOS or how to upgrade to
> newer versions now that brew won't do that automatically for us has
> been punted for now as well.  On that line the now obsolete comment
> about it in lib.sh was originally being updated by this change but
> created conflicts as it is moved around by other on the fly changes,
> so will be addressed independently as well.
>
> [1] https://github.com/Homebrew/homebrew-cask/pull/122347#discussion_r856026584
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>

Re: [PATCH v3 2/4] ci: avoid brew for installing perforce

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2241 bytes --]

Hi Carlo,

On Sat, 23 Apr 2022, Carlo Marcelo Arenas Belón wrote:

> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 41e9290fbdd..9da03350d09 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -37,13 +37,14 @@ macos-latest)
>  	test -z "$BREW_INSTALL_PACKAGES" ||
>  	brew install $BREW_INSTALL_PACKAGES
>  	brew link --force gettext
> -	brew install --cask --no-quarantine perforce || {
> -		# Update the definitions and try again
> -		cask_repo="$(brew --repository)"/Library/Taps/homebrew/homebrew-cask &&
> -		git -C "$cask_repo" pull --no-stat --ff-only &&
> -		brew install --cask --no-quarantine perforce
> -	} ||
> -	brew install homebrew/cask/perforce
> +	mkdir -p $HOME/bin
> +	(
> +		cd $HOME/bin
> +		wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" &&

I vaguely recall that Gábor Szeder attempted something similar, but I
_think_ that ultimately there were too many moving parts in that URL that
we did not want to hardcode.

For example, when opening a PR against `maint` or an even older topic
branch, we are stuck with the exact code in that revision that obtains
the perforce package. But since that server is outside our control, it can
very well evolve to a state that is not amenable to hard-coding such a
URL.

While I don't think that we can solve this fully, I would prefer to keep
the existing `brew install` calls but fall back to downloading from a
hard-coded URL. That still would have the shortcoming of hard-coded
`r21.2` and `1015`, but it should fail less often than the proposed patch.

For the record, the recent problems stem from the fact that the package
was cached on GitHub's build agents (I guess to avoid many identical
downloads), and the cached package did not match what was recorded in the
updated package definition. Which means that those `brew` errors are only
transient, until a new VM image is built that caches the then-current
perforce package.

Ciao,
Dscho

> +		tar -xf helix-core-server.tgz
> +	)
> +	PATH="$PATH:${HOME}/bin"
> +	export PATH
>
>  	if test -n "$CC_PACKAGE"
>  	then
> --
> 2.36.0.266.g59f845bde02
>
>

Re: [PATCH v3 2/4] ci: avoid brew for installing perforce

[Carlo Arenas]

On Tue, Apr 26, 2022 at 8:55 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Sat, 23 Apr 2022, Carlo Marcelo Arenas Belón wrote:
>
> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> > index 41e9290fbdd..9da03350d09 100755
> > --- a/ci/install-dependencies.sh
> > +++ b/ci/install-dependencies.sh
> > @@ -37,13 +37,14 @@ macos-latest)
> >       test -z "$BREW_INSTALL_PACKAGES" ||
> >       brew install $BREW_INSTALL_PACKAGES
> >       brew link --force gettext
> > -     brew install --cask --no-quarantine perforce || {
> > -             # Update the definitions and try again
> > -             cask_repo="$(brew --repository)"/Library/Taps/homebrew/homebrew-cask &&
> > -             git -C "$cask_repo" pull --no-stat --ff-only &&
> > -             brew install --cask --no-quarantine perforce
> > -     } ||
> > -     brew install homebrew/cask/perforce
> > +     mkdir -p $HOME/bin
> > +     (
> > +             cd $HOME/bin
> > +             wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" &&
>
> I vaguely recall that Gábor Szeder attempted something similar, but I
> _think_ that ultimately there were too many moving parts in that URL that
> we did not want to hardcode.

I can see that, and indeed that is also why those series still keep
perforce installation as optional so it wouldn't block CI when it does.

BTW most of the reasons why those values are hardcoded here
is just to avoid conflicts with on the fly changes, and making it smarter
and avoiding the hardcoding would be done as a prerequisite to making
it mandatory again (if that is what is preferred)

> While I don't think that we can solve this fully, I would prefer to keep
> the existing `brew install` calls but fall back to downloading from a
> hard-coded URL.

Considering that the expectation from brew maintainers (as documented[1])
is that those mismatches would happen and should be solved manually
and that perforce releases just do inplace binary pushes I still stand by my
suggestion to avoid brew, but it might be useful as a fallback of the wget
approach if that helps with your concerns above.

> For the record, the recent problems stem from the fact that the package
> was cached on GitHub's build agents (I guess to avoid many identical
> downloads), and the cached package did not match what was recorded in the
> updated package definition. Which means that those `brew` errors are only
> transient, until a new VM image is built that caches the then-current
> perforce package.

FWIW as I updated the brew cask[2], I realized that livecheck pointed to an even
newer release, so if we are adding brew back it might be worth adding
yet another
workaround to delete the cached file and try without a SHA as Ævar suggested.

Carlo

[1] https://docs.brew.sh/Common-Issues#cask---checksum-does-not-match
[2] https://github.com/Homebrew/homebrew-cask/pull/122347

[PATCH v3 3/4] ci: reintroduce prevention from perforce being quarantined in macOS

[Carlo Marcelo Arenas Belón]

5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27)
introduces this prevention for brew, but brew has been removed in a
previous commit, so reintroduce an equivalent option to avoid a possible
regression.

This doesn't affect github actions (as configure now) and is therefore
done silently to avoid any possible scary irrelevant messages.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 ci/install-dependencies.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 9da03350d09..46a23a33dbd 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -41,7 +41,8 @@ macos-latest)
 	(
 		cd $HOME/bin
 		wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" &&
-		tar -xf helix-core-server.tgz
+		tar -xf helix-core-server.tgz &&
+		sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
 	)
 	PATH="$PATH:${HOME}/bin"
 	export PATH
-- 
2.36.0.266.g59f845bde02

Re: [PATCH v3 3/4] ci: reintroduce prevention from perforce being quarantined in macOS

[Eric Sunshine]

On Sat, Apr 23, 2022 at 10:26 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> 5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27)
> introduces this prevention for brew, but brew has been removed in a
> previous commit, so reintroduce an equivalent option to avoid a possible
> regression.
>
> This doesn't affect github actions (as configure now) and is therefore
> done silently to avoid any possible scary irrelevant messages.

s/configure/configured/

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> @@ -41,7 +41,8 @@ macos-latest)
>         (
>                 cd $HOME/bin
>                 wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" &&
> -               tar -xf helix-core-server.tgz
> +               tar -xf helix-core-server.tgz &&
> +               sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
>         )
>         PATH="$PATH:${HOME}/bin"
>         export PATH

I don't see the point of the `|| true` since the code doesn't care
about the eventual outcome of the &&-chain. (The &&-chain itself makes
sense, though, to avoid executing commands pointlessly if an earlier
step failed.) May not be worth a reroll, though.

[PATCH v3 4/4] CI: use https, not http to download binaries from perforce.com

[Carlo Marcelo Arenas Belón]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Since 522354d70f4 (Add Travis CI support, 2015-11-27) the CI has used
http://filehost.perforce.com/perforce/ to download binaries from
filehost.perforce.com, they were then moved to this script in
657343a602e (travis-ci: move Travis CI code into dedicated scripts,
2017-09-10).

Let's use https instead for good measure. I don't think we need to
worry about the DNS or network between the GitHub CI and perforce.com
being MitM'd, but using https gives us extra validation of the payload
at least, and is one less thing to worry about when checking where
else we rely on non-TLS'd http connections.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 46a23a33dbd..a2d3578f4e3 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -5,7 +5,7 @@
 
 . ${0%/*}/lib.sh
 
-P4WHENCE=http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
+P4WHENCE=https://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
 UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
-- 
2.36.0.266.g59f845bde02

[PATCH v4 0/4] ci: avoid perforce/brew issues affecting macOS

[Junio C Hamano]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3824 bytes --]

We see macOS CI job failures from time to time when the version of
p4 brew expects is slightly older than what is offered at Perforce's
download site.  Instead of failing to run the tests altogether, just
skip "git-p4" tests, like we do for tests that depends on any other
optional packages, when this happens.

This is essentially unchanged from the v3 posted by Carlo on April
23rd, with a few fix-ups that have been cooking on top squashed in.

The changes from the previous round (i.e. the "fix-ups" that have
been floating on top, which got squashed in) are:

 * The first step has a fix to a couple of wrong redirections that
   created a junk file whose name is "2", instead of sending the
   messages to the standard error.

 * The last step makes two places that we download Perforce packages
   to use https://cdist2.perforce.com/ (earlier one of the places
   used that, the other https://filehost.perforce.com/).

I am sending this out as a final "complain now, or this will go to
'next' soonish" warning.  The "What's cooking" report is getting
crowded with too many topics marked as "Expecting a reroll", and I'm
trying to do easier ones myself to see how much reduction we can
make.

Carlo Marcelo Arenas Belón (3):
  ci: make failure to find perforce more user friendly
  ci: avoid brew for installing perforce
  ci: reintroduce prevention from perforce being quarantined in macOS

Ævar Arnfjörð Bjarmason (1):
  ci: use https, not http to download binaries from perforce.com

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


1:  5730a749bc ! 1:  024459486c ci: make failure to find perforce more user friendly
    @@ ci/install-dependencies.sh: linux-gcc-default)
      	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
      	p4 -V | grep Rev.
     +else
    -+	echo "WARNING: perforce wasn't installed, see above for clues why" >2
    ++	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
      fi
     -if type git-lfs >/dev/null
     +if type git-lfs >/dev/null 2>&1
    @@ ci/install-dependencies.sh: linux-gcc-default)
      	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
      	git-lfs version
     +else
    -+	echo "WARNING: git-lfs wasn't installed, see above for clues why" >2
    ++	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
      fi
2:  8cd96645ae = 2:  8717dbe8d9 ci: avoid brew for installing perforce
3:  1f06d0ba06 = 3:  6a4f085d63 ci: reintroduce prevention from perforce being quarantined in macOS
4:  6bf267b995 ! 4:  5be72d9150 ci: use https, not http to download binaries from perforce.com
    @@ Commit message
         at least, and is one less thing to worry about when checking where
         else we rely on non-TLS'd http connections.
     
    +    Also, use the same download site at perforce.com for Linux and macOS
    +    tarballs for consistency.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## ci/install-dependencies.sh ##
    @@ ci/install-dependencies.sh
      . ${0%/*}/lib.sh
      
     -P4WHENCE=http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
    -+P4WHENCE=https://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
    ++P4WHENCE=https://cdist2.perforce.com/perforce/r$LINUX_P4_VERSION
      LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
      UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
       tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl



-- 
2.36.1-338-g1c7f76a54c

[PATCH v4 1/4] ci: make failure to find perforce more user friendly

[Junio C Hamano]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>

In preparation for a future change that will make perforce installation
optional in macOS, make sure that the check for it is done without
triggering scary looking errors and add a user friendly message instead.

All other existing uses of 'type <cmd>' in our shell scripts that
check the availability of a command <cmd> send both standard output
and error stream to /dev/null to squelch "<cmd> not found" diagnostic
output, but this script left the standard error stream shown.

Redirect it just like everybody else to squelch this error message that
we fully expect to see.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dbcebad2fb..fa620f8fcd 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -78,15 +78,19 @@ linux-gcc-default)
 	;;
 esac
 
-if type p4d >/dev/null && type p4 >/dev/null
+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 | grep Rev.
 	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
 	p4 -V | grep Rev.
+else
+	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
 fi
-if type git-lfs >/dev/null
+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
-- 
2.36.1-338-g1c7f76a54c

[PATCH v4 2/4] ci: avoid brew for installing perforce

[Junio C Hamano]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2833 bytes --]

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>

Perfoce's cask in brew is meant[1] to be used only by humans, so replace
its use from the CI with a scripted binary download which is less likely
to fail, as it is done in Linux.

Kept the logic together so it will be less likely to break when moved
around as on the fly code changes in this area are settled, at which
point it will also feasable to ammend it to avoid some of the hardcoded
values by using similar variables to the ones Linux does.

In that same line, a POSIX sh syntax is used instead of the similar one
used in Linux in preparation for an unrelated future change that might
change the shell currently configured for it.

This change reintroduces the risk that the installed binaries might not
work because of being quarantined that was fixed with 5ed9fc3fc86 (ci:
prevent `perforce` from being quarantined, 2020-02-27) but fixing that
now was also punted for simplicity and since the affected cloud provider
is scheduled to be retired with an on the fly change, but should be
addressed if that other change is not integrated further.

The discussion on the need to keep 2 radically different versions of
the binaries to be tested with Linux vs macOS or how to upgrade to
newer versions now that brew won't do that automatically for us has
been punted for now as well.  On that line the now obsolete comment
about it in lib.sh was originally being updated by this change but
created conflicts as it is moved around by other on the fly changes,
so will be addressed independently as well.

[1] https://github.com/Homebrew/homebrew-cask/pull/122347#discussion_r856026584

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index fa620f8fcd..8e796fa669 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -37,13 +37,14 @@ macos-latest)
 	test -z "$BREW_INSTALL_PACKAGES" ||
 	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
-	brew install --cask --no-quarantine perforce || {
-		# Update the definitions and try again
-		cask_repo="$(brew --repository)"/Library/Taps/homebrew/homebrew-cask &&
-		git -C "$cask_repo" pull --no-stat --ff-only &&
-		brew install --cask --no-quarantine perforce
-	} ||
-	brew install homebrew/cask/perforce
+	mkdir -p $HOME/bin
+	(
+		cd $HOME/bin
+		wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" &&
+		tar -xf helix-core-server.tgz
+	)
+	PATH="$PATH:${HOME}/bin"
+	export PATH
 
 	if test -n "$CC_PACKAGE"
 	then
-- 
2.36.1-338-g1c7f76a54c

[PATCH v4 3/4] ci: reintroduce prevention from perforce being quarantined in macOS

[Junio C Hamano]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>

5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27)
introduces this prevention for brew, but brew has been removed in a
previous commit, so reintroduce an equivalent option to avoid a possible
regression.

This doesn't affect github actions (as configure now) and is therefore
done silently to avoid any possible scary irrelevant messages.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 8e796fa669..c150bce2d9 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -41,7 +41,8 @@ macos-latest)
 	(
 		cd $HOME/bin
 		wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" &&
-		tar -xf helix-core-server.tgz
+		tar -xf helix-core-server.tgz &&
+		sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
 	)
 	PATH="$PATH:${HOME}/bin"
 	export PATH
-- 
2.36.1-338-g1c7f76a54c

[PATCH v4 4/4] ci: use https, not http to download binaries from perforce.com

[Junio C Hamano]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Since 522354d70f4 (Add Travis CI support, 2015-11-27) the CI has used
http://filehost.perforce.com/perforce/ to download binaries from
filehost.perforce.com, they were then moved to this script in
657343a602e (travis-ci: move Travis CI code into dedicated scripts,
2017-09-10).

Let's use https instead for good measure. I don't think we need to
worry about the DNS or network between the GitHub CI and perforce.com
being MitM'd, but using https gives us extra validation of the payload
at least, and is one less thing to worry about when checking where
else we rely on non-TLS'd http connections.

Also, use the same download site at perforce.com for Linux and macOS
tarballs for consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index c150bce2d9..107757a1fe 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -5,7 +5,7 @@
 
 . ${0%/*}/lib.sh
 
-P4WHENCE=http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
+P4WHENCE=https://cdist2.perforce.com/perforce/r$LINUX_P4_VERSION
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
 UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
-- 
2.36.1-338-g1c7f76a54c