[PATCH 0/2] CI: don't fail OSX tests due to brew v.s. perforce.com mis-match

[PATCH 0/2] CI: don't fail OSX tests due to brew v.s. perforce.com mis-match

[Ævar Arnfjörð Bjarmason]

Junio: Despite modifying CI stuff this series merges cleanly with
"seen", and has no semantic conflicts with any outstanding CI changes.

For the past days we've again had CI failures due to "brew install"
detecting a SHA-256 mismatch when trying to install the perforce
package[1]. E.g. "seen" is now failing: https://github.com/git/git/runs/6104156856?check_suite_focus=true

This occurrence of this issue will no doubt be fixed within a few days
as the homebrew-cask repository is updated, i.e. this recipe:
https://github.com/Homebrew/homebrew-cask/commits/master/Casks/perforce.rb

But for our CI usage being this anal about the check isn't worth it,
here's a passing CI run where we simply forced the installation:
https://github.com/avar/git/runs/6092916675?check_suite_focus=true#step:3:87
and subsequently passed the git-p4 tests:
https://github.com/avar/git/runs/6092916675?check_suite_focus=true#step:4:1678

Ævar Arnfjörð Bjarmason (2):
  CI: run "brew install perforce" without past workarounds
  CI: don't care about SHA256 mismatch on upstream "perforce" package

 ci/install-dependencies.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

-- 
2.36.0.893.g80a51c675f6

[PATCH 1/2] CI: run "brew install perforce" without past workarounds

[Ævar Arnfjörð Bjarmason]

Remove the alternating between --no-quarantine, --cask and fallback
"git pull" updating of the "perforce" package.

As can be seen in [1], [2] and [3] these were workarounds for various
past CI issues. Running "brew install perforce" works now in GitHub
CI, so there's no need to alternate between package names, and the
"git pull" method was a workaround for some staleness issue on the
Azure pipelines removed in [4].

We do have a really common issue with this failing, but that's
unrelated to any of those past fixes, and removing these old
workarounds makes dealing with that a lot easier.

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. 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)

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

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dbcebad2fb2..82fa87f97af 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -37,13 +37,7 @@ 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
+	brew install perforce
 
 	if test -n "$CC_PACKAGE"
 	then
-- 
2.36.0.893.g80a51c675f6

[PATCH 2/2] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Ævar Arnfjörð Bjarmason]

As can be seen in the commit history[1] of the upstream perforce.rb in
homebrew-cask the upstream perforce package URL and its SHA-256 are
aren't a unique pair. The upstream will start publishing an updated
package at the same URL as the previous version, causing the CI to
routinely fail with errors like:

	==> Downloading https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz
	Error: SHA256 mismatch
	Expected: ffc757b9d4d0629b2594e2777edfb18097825e29c70d8f33a444c7482d622806
	  Actual: 37bc306f0bdfd1d63cfcea113ada132d96f89d53cbb20c282735d51d06223054

Once someone gets around to updating the perforce.rb the failure of
git's CI will be cleared up, but in the meantime all osx-{gcc,clang}
jobs will encounter hard failures.

Let's not be so anal about this and fallback to a "sha256 :no_check"
on failure. We are already downloading arbitrary binaries from
perforce's servers, and the point of doing so is to run the
t/*-git-p4-*.sh tests, not to validate the chain of custody between
perforce.com and the homebrew-cask repository.

In the obscure (but unlikely to ever happen) that the failure is
specifically because perforce.com published a bad updated package, and
it a failure that their testing wouldn't have caught, but whoever's
updating the homebrew SHA-256 recipe would have caught, we will have a
failure in our p4 tests that we wouldn't have otherwise had.

But I think that's so unlikely that we don't need to worry about it,
whereas seeing failures due to the homebrew recipe lagging upstream is
a real issue. E.g. "seen"'s latest push-out has such a failure: [3]

Note: It's probably possible to embed this "sed" one-liner directly in
the HOMEBREW_EDITOR variable, i.e.:

    HOMEBREW_EDITOR='...' brew edit perforce

But my attempts to do so were unsuccessful, particularly since I don't
have access to a Mac OS X machine other than via by round-tripping
through the CI. This version of getting the path via --print-path
works, and is arguably easier to reason about and debug than a cute
one-liner.

1. https://github.com/Homebrew/homebrew-cask/commits/master/Casks/perforce.rb
2. https://docs.brew.sh/Cask-Cookbook#required-stanzas
3. https://github.com/git/git/runs/6104156856?check_suite_focus=true
---
 ci/install-dependencies.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 82fa87f97af..540deab4488 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -37,7 +37,13 @@ macos-latest)
 	test -z "$BREW_INSTALL_PACKAGES" ||
 	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
-	brew install perforce
+	brew install perforce || {
+		echo Installing perforce failed, assuming and working around SHA256 mismatch >&2 &&
+
+		path=$(brew edit --print-path perforce) &&
+		sed -i -e 's/\(sha256.\).*/\1:no_check/' "$path" &&
+		brew install perforce
+	}
 
 	if test -n "$CC_PACKAGE"
 	then
-- 
2.36.0.893.g80a51c675f6

Re: [PATCH 2/2] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Junio C Hamano]

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

> In the obscure (but unlikely to ever happen) that the failure is
> specifically because perforce.com published a bad updated package, and
> it a failure that their testing wouldn't have caught, but whoever's
> updating the homebrew SHA-256 recipe would have caught, we will have a
> failure in our p4 tests that we wouldn't have otherwise had.

Or DNS the CI site consults is tainted and we got a bad package from
a fake perforce.com?

> @@ -37,7 +37,13 @@ macos-latest)
>  	test -z "$BREW_INSTALL_PACKAGES" ||
>  	brew install $BREW_INSTALL_PACKAGES
>  	brew link --force gettext
> -	brew install perforce
> +	brew install perforce || {
> +		echo Installing perforce failed, assuming and working around SHA256 mismatch >&2 &&

I had to read this three times before realizing what you are
"assuming".  You suspect without having a way to verify that SHA-256
mismatch was the reason why the attempt to install failed, and
working it around.  Makes sense.

What does it buy us to do this only as a fallback?  If we munged the
$path to disable sha256 checking before the initial "brew install",
we would install it happily if the package is the correct one, and
if it is not a kosher one, we'd install it anyway.

Is it so that we can tell if we had the checksum mismatch or not?
It is unfortunate that no_check is the only "special" value for the
field (I would have loved to use "warn_only" if it were available).

> +
> +		path=$(brew edit --print-path perforce) &&
> +		sed -i -e 's/\(sha256.\).*/\1:no_check/' "$path" &&

"sed -i" is not POSIX and without macOS box I do not know if it
works there.  FreeBSD sed manual seems to indicate they have

	sed -i <extension>

In our current codebase, "sed -i" appears to be used only in vcxproj
part of config.mak.uname

I would usually have said that "I'd rather see us not to use it
here, to prevent others from copying and pasting it, if it can be
helped", but this is very much macOS specific part of an obscure
corner of the source tree, so as long as we are sure it works there,
and if it is too cumbersome to avoid editing in-place, I'd let it
go.

Ah, no, I'd say we should NOT use "sed -i" here, not in the form in
this patch, after seeing

  https://unix.stackexchange.com/questions/401905/bsd-sed-vs-gnu-sed-and-i

but that is 4-year old information, so...

> +		brew install perforce
> +	}
>  
>  	if test -n "$CC_PACKAGE"
>  	then

Re: [PATCH 2/2] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 21 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> In the obscure (but unlikely to ever happen) that the failure is
>> specifically because perforce.com published a bad updated package, and
>> it a failure that their testing wouldn't have caught, but whoever's
>> updating the homebrew SHA-256 recipe would have caught, we will have a
>> failure in our p4 tests that we wouldn't have otherwise had.
>
> Or DNS the CI site consults is tainted and we got a bad package from
> a fake perforce.com?

Yeah, or any number of other things, all probably too obscure to worry about.

>> @@ -37,7 +37,13 @@ macos-latest)
>>  	test -z "$BREW_INSTALL_PACKAGES" ||
>>  	brew install $BREW_INSTALL_PACKAGES
>>  	brew link --force gettext
>> -	brew install perforce
>> +	brew install perforce || {
>> +		echo Installing perforce failed, assuming and working around SHA256 mismatch >&2 &&
>
> I had to read this three times before realizing what you are
> "assuming".  You suspect without having a way to verify that SHA-256
> mismatch was the reason why the attempt to install failed, and
> working it around.  Makes sense.
>
> What does it buy us to do this only as a fallback?  If we munged the
> $path to disable sha256 checking before the initial "brew install",
> we would install it happily if the package is the correct one, and
> if it is not a kosher one, we'd install it anyway.
>
> Is it so that we can tell if we had the checksum mismatch or not?
> It is unfortunate that no_check is the only "special" value for the
> field (I would have loved to use "warn_only" if it were available).

Yes, just to be able to tell that we tried, and overrode it. If anything
goes wrong we'll be able to see that we did that, in case it caused any
fallout.

>> +
>> +		path=$(brew edit --print-path perforce) &&
>> +		sed -i -e 's/\(sha256.\).*/\1:no_check/' "$path" &&
>
> "sed -i" is not POSIX and without macOS box I do not know if it
> works there.  FreeBSD sed manual seems to indicate they have
>
> 	sed -i <extension>
>
> In our current codebase, "sed -i" appears to be used only in vcxproj
> part of config.mak.uname
>
> I would usually have said that "I'd rather see us not to use it
> here, to prevent others from copying and pasting it, if it can be
> helped", but this is very much macOS specific part of an obscure
> corner of the source tree, so as long as we are sure it works there,
> and if it is too cumbersome to avoid editing in-place, I'd let it
> go.
>
> Ah, no, I'd say we should NOT use "sed -i" here, not in the form in
> this patch, after seeing
>
>   https://unix.stackexchange.com/questions/401905/bsd-sed-vs-gnu-sed-and-i
>
> but that is 4-year old information, so...

It works on the OSX we use now:
https://github.com/avar/git/runs/6092916612?check_suite_focus=true

I think it's fine to keep it, but we could also use "perl -pi -e" here,
or a rename dance...

>> +		brew install perforce
>> +	}
>>  
>>  	if test -n "$CC_PACKAGE"
>>  	then

Re: [PATCH 2/2] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Eric Sunshine]

On Thu, Apr 21, 2022 at 5:10 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Thu, Apr 21 2022, Junio C Hamano wrote:
> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> >> +            sed -i -e 's/\(sha256.\).*/\1:no_check/' "$path" &&
> >
> > "sed -i" is not POSIX and without macOS box I do not know if it
> > works there.  FreeBSD sed manual seems to indicate they have
> >
> >       sed -i <extension>
> >
> > Ah, no, I'd say we should NOT use "sed -i" here, not in the form in
> > this patch, after seeing
> >   https://unix.stackexchange.com/questions/401905/bsd-sed-vs-gnu-sed-and-i
> > but that is 4-year old information, so...
>
> It works on the OSX we use now:
> https://github.com/avar/git/runs/6092916612?check_suite_focus=true

On my end-of-life'd 10.13 macOS, `sed` (which is the FreeBSD version)
requires an argument after `-i`, thus it's taking `-e` as the
extension name for the backup file, and then uses the raw
's/\(sha256.\).*/\1:no_check/' as the `sed` command. So, it's working,
but only by accident since `-e` is optional when invoking `sed`.

> I think it's fine to keep it, but we could also use "perl -pi -e" here,
> or a rename dance...

Although it's working (by accident), it's rather misleading since it
looks like `-e` is introducing the `sed` command, even though it
really isn't (the `-e` is being consumed by the `-i` option). Any of
the following would likely be less confusing (in no particular order
of preference):

    * sed -i .bak -e '...' "$path"
    * rename dance
    * perl -pi -e ...

Re: [PATCH 2/2] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> really isn't (the `-e` is being consumed by the `-i` option).

That makes sense.  So after all 4-year old random finding on the
internet had some value in pregventing a new breakage to sneak into
our codebase.  Good ;-)

> Any of
> the following would likely be less confusing (in no particular order
> of preference):
>
>     * sed -i .bak -e '...' "$path"
>     * rename dance
>     * perl -pi -e ...

That order happens to match my preference, but if the first one
comes with a comment to dissuade readers to copy-and-paste the
construct to other places in our code, that would be even better.

Perhaps something along this line.

 ci/install-dependencies.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git c/ci/install-dependencies.sh w/ci/install-dependencies.sh
index 540deab448..2a5ee34246 100755
--- c/ci/install-dependencies.sh
+++ w/ci/install-dependencies.sh
@@ -41,7 +41,12 @@ macos-latest)
 		echo Installing perforce failed, assuming and working around SHA256 mismatch >&2 &&
 
 		path=$(brew edit --print-path perforce) &&
-		sed -i -e 's/\(sha256.\).*/\1:no_check/' "$path" &&
+
+		# we do not do this unconditionally because we want
+		# to know that we are falling back.  Do not copy this
+		# use of 'sed -i .bak' elsewhere---it does not work with
+		# other implementations of "sed".
+		sed -i .bak -e 's/\(sha256.\).*/\1:no_check/' "$path" &&
 		brew install perforce
 	}
 

Re: [PATCH 2/2] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Eric Sunshine]

On Thu, Apr 21, 2022 at 5:38 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Any of
> > the following would likely be less confusing (in no particular order
> > of preference):
> >
> >     * sed -i .bak -e '...' "$path"
> >     * rename dance
> >     * perl -pi -e ...
>
> That order happens to match my preference, but if the first one
> comes with a comment to dissuade readers to copy-and-paste the
> construct to other places in our code, that would be even better.

Bikeshedding: I think I would prefer the rename-dance over a lengthy
comment meant to dissuade people from copying this non-portable usage,
especially since people often fail to read comments. The rename-dance
idiom, on the other hand, can be cargo-culted without harm.

Re: [PATCH 2/2] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Apr 21, 2022 at 5:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > Any of
>> > the following would likely be less confusing (in no particular order
>> > of preference):
>> >
>> >     * sed -i .bak -e '...' "$path"
>> >     * rename dance
>> >     * perl -pi -e ...
>>
>> That order happens to match my preference, but if the first one
>> comes with a comment to dissuade readers to copy-and-paste the
>> construct to other places in our code, that would be even better.
>
> Bikeshedding: I think I would prefer the rename-dance over a lengthy
> comment meant to dissuade people from copying this non-portable usage,
> especially since people often fail to read comments. The rename-dance
> idiom, on the other hand, can be cargo-culted without harm.

Yeah, that is fine, too.

Re: [PATCH 2/2] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 21 2022, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Thu, Apr 21, 2022 at 5:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> > Any of
>>> > the following would likely be less confusing (in no particular order
>>> > of preference):
>>> >
>>> >     * sed -i .bak -e '...' "$path"
>>> >     * rename dance
>>> >     * perl -pi -e ...
>>>
>>> That order happens to match my preference, but if the first one
>>> comes with a comment to dissuade readers to copy-and-paste the
>>> construct to other places in our code, that would be even better.
>>
>> Bikeshedding: I think I would prefer the rename-dance over a lengthy
>> comment meant to dissuade people from copying this non-portable usage,
>> especially since people often fail to read comments. The rename-dance
>> idiom, on the other hand, can be cargo-culted without harm.
>
> Yeah, that is fine, too.

I just used the rename dance in the updated v3:
https://lore.kernel.org/git/cover-v2-0.3-00000000000-20220422T085958Z-avarab@gmail.com/

Re: [PATCH 0/2] CI: don't fail OSX tests due to brew v.s. perforce.com mis-matchy

[Carlo Marcelo Arenas Belón]

On Thu, Apr 21, 2022 at 02:53:50PM +0200, Ævar Arnfjörð Bjarmason wrote:
> For the past days we've again had CI failures due to "brew install"
> detecting a SHA-256 mismatch when trying to install the perforce

Since the only reason why that is a concern is because it aborts the
rest of the run and is a recurring problem, wouldn't it be better to
tell the script to continue regardless and therefore skip all perforce
tests?

Sure, there is a window where that integration could be broken which
will be only visible once the perforce cask gets fixed and perforce
installs again, but wouldn't that be less intrusive and overall safer
than the currently proposed change?

Carlo

Re: [PATCH 0/2] CI: don't fail OSX tests due to brew v.s. perforce.com mis-matchy

[Junio C Hamano]

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

> On Thu, Apr 21, 2022 at 02:53:50PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> For the past days we've again had CI failures due to "brew install"
>> detecting a SHA-256 mismatch when trying to install the perforce
>
> Since the only reason why that is a concern is because it aborts the
> rest of the run and is a recurring problem, wouldn't it be better to
> tell the script to continue regardless and therefore skip all perforce
> tests?
>
> Sure, there is a window where that integration could be broken which
> will be only visible once the perforce cask gets fixed and perforce
> installs again, but wouldn't that be less intrusive and overall safer
> than the currently proposed change?

Good suggestion.  Care to come up with an alternative patch (or
two)?

Thanks.

Re: [PATCH 0/2] CI: don't fail OSX tests due to brew v.s. perforce.com mis-matchy

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 21 2022, Carlo Marcelo Arenas Belón wrote:

> On Thu, Apr 21, 2022 at 02:53:50PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> For the past days we've again had CI failures due to "brew install"
>> detecting a SHA-256 mismatch when trying to install the perforce
>
> Since the only reason why that is a concern is because it aborts the
> rest of the run and is a recurring problem, wouldn't it be better to
> tell the script to continue regardless and therefore skip all perforce
> tests?
>
> Sure, there is a window where that integration could be broken which
> will be only visible once the perforce cask gets fixed and perforce
> installs again, but wouldn't that be less intrusive and overall safer
> than the currently proposed change?

I tried to answer all of this in the updated v3 CL. Thanks!:
https://lore.kernel.org/git/cover-v2-0.3-00000000000-20220422T085958Z-avarab@gmail.com/

Re: [PATCH 0/2] CI: don't fail OSX tests due to brew v.s. perforce.com mis-match

[Junio C Hamano]

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

> Junio: Despite modifying CI stuff this series merges cleanly with
> "seen", and has no semantic conflicts with any outstanding CI changes.
>
> For the past days we've again had CI failures due to "brew install"
> detecting a SHA-256 mismatch when trying to install the perforce
> package[1]. E.g. "seen" is now failing: https://github.com/git/git/runs/6104156856?check_suite_focus=true

Yup, it was something I've been disturbed by and planning to ping
folks about if it continued.

> This occurrence of this issue will no doubt be fixed within a few days
> as the homebrew-cask repository is updated, i.e. this recipe:
> https://github.com/Homebrew/homebrew-cask/commits/master/Casks/perforce.rb
>
> But for our CI usage being this anal about the check isn't worth it,
> here's a passing CI run where we simply forced the installation:
> ...
> Ævar Arnfjörð Bjarmason (2):
>   CI: run "brew install perforce" without past workarounds
>   CI: don't care about SHA256 mismatch on upstream "perforce" package

I dunno.  Does it open us to a new attack vector in some way?

[PATCH v2 0/3] CI: don't fail OSX tests due to brew v.s. perforce.com mis-match

[Ævar Arnfjörð Bjarmason]

I think this re-roll should address outstanding feedback with the v1.

I also added an extra patch to change the existing http:// download of
the perforce binaries to use https://, which should further make the
case that we can do without any homebrew-specific sanity checking in
this area: It's what we're already doing for the ubuntu-latest jobs.

I think a sensible follow-up to this would be to skip invoking
homebrew entirely, we can simply:

    wget https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz

And then do the exact same thing we're doing to get the linux p4
binaries.

A CI job showing this series working (including the working OSX fallback):
https://github.com/avar/git/runs/6125736258?check_suite_focus=true#step:3:83

In addition to that the wget via https for the linux perforce package works:
https://github.com/avar/git/runs/6125736067?check_suite_focus=true#step:3:273

Ævar Arnfjörð Bjarmason (3):
  CI: run "brew install perforce" without past workarounds
  CI: don't care about SHA256 mismatch on upstream "perforce" package
  CI: use https, not http to download binaries from perforce.com

 ci/install-dependencies.sh | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Range-diff against v1:
1:  dcedf03c2d7 = 1:  dcedf03c2d7 CI: run "brew install perforce" without past workarounds
2:  28208bac859 ! 2:  2805e89623c CI: don't care about SHA256 mismatch on upstream "perforce" package
    @@ Commit message
         jobs will encounter hard failures.
     
         Let's not be so anal about this and fallback to a "sha256 :no_check"
    -    on failure. We are already downloading arbitrary binaries from
    -    perforce's servers, and the point of doing so is to run the
    -    t/*-git-p4-*.sh tests, not to validate the chain of custody between
    -    perforce.com and the homebrew-cask repository.
    +    on failure. For the "ubuntu-latest" jobs just a few lines earlier we
    +    "wget" and chmod binaries from http://filehost.perforce.com (no
    +    https!).
     
    -    In the obscure (but unlikely to ever happen) that the failure is
    -    specifically because perforce.com published a bad updated package, and
    -    it a failure that their testing wouldn't have caught, but whoever's
    -    updating the homebrew SHA-256 recipe would have caught, we will have a
    -    failure in our p4 tests that we wouldn't have otherwise had.
    +    We're already trusting the DNS in the CI to do the right thing here,
    +    and for there not to be any MitM attacks between the CI and
    +    filehost.perforce.com. Even if someone managed to get in the middle
    +    the worst they could do is run arbitrary code in the CI environment.
     
    -    But I think that's so unlikely that we don't need to worry about it,
    -    whereas seeing failures due to the homebrew recipe lagging upstream is
    -    a real issue. E.g. "seen"'s latest push-out has such a failure: [3]
    +    The only thing we were getting out of the SHA-256 check was to serve
    +    as a canary that the homebrew-cask repository itself was drifting out
    +    of date. It seems sensible to just cut it out as a middle-man
    +    entirely (as the ubuntu-latest jobs do). We want to run the
    +    t/*-git-p4-*.sh tests, not to validate the chain of custody between
    +    perforce.com and the homebrew-cask repository.
     
    -    Note: It's probably possible to embed this "sed" one-liner directly in
    -    the HOMEBREW_EDITOR variable, i.e.:
    +    See [2] for the "no_check" syntax, and [3] for an example of a failure
    +    in "seen" before this change.
     
    -        HOMEBREW_EDITOR='...' brew edit perforce
    +    It's been suggested as an alternative [4] to simply disable the p4
    +    tests if we can't install the package from homebrew. While I don't
    +    really care, I think that would be strictly worse. Before this change
    +    we've either run the p4 tests or failed, and we'll still fail now if
    +    we can't run the p4 tests.
     
    -    But my attempts to do so were unsuccessful, particularly since I don't
    -    have access to a Mac OS X machine other than via by round-tripping
    -    through the CI. This version of getting the path via --print-path
    -    works, and is arguably easier to reason about and debug than a cute
    -    one-liner.
    +    Whereas skipping them entirely as [4] does is introducing the caveat
    +    that our test coverage in these jobs today might be different than the
    +    coverage tomorrow. If we do want to introduce such dynamic runs to CI
    +    I think they should use the relevant "needs" or "if" features, so that
    +    the UX can make it obvious what was or wasn't dynamically skipped.
     
         1. https://github.com/Homebrew/homebrew-cask/commits/master/Casks/perforce.rb
         2. https://docs.brew.sh/Cask-Cookbook#required-stanzas
         3. https://github.com/git/git/runs/6104156856?check_suite_focus=true
    +    4. https://lore.kernel.org/git/20220421225515.6316-1-carenas@gmail.com/
     
      ## ci/install-dependencies.sh ##
     @@ ci/install-dependencies.sh: macos-latest)
    @@ ci/install-dependencies.sh: macos-latest)
     +		echo Installing perforce failed, assuming and working around SHA256 mismatch >&2 &&
     +
     +		path=$(brew edit --print-path perforce) &&
    -+		sed -i -e 's/\(sha256.\).*/\1:no_check/' "$path" &&
    ++		sed 's/\(sha256.\).*/\1:no_check/' <"$path" >"$path".tmp &&
    ++		mv "$path".tmp "$path" &&
     +		brew install perforce
     +	}
      
-:  ----------- > 3:  3fdd54aa8df CI: use https, not http to download binaries from perforce.com
-- 
2.36.0.879.g56a83971f3f

[PATCH v2 1/3] CI: run "brew install perforce" without past workarounds

[Ævar Arnfjörð Bjarmason]

Remove the alternating between --no-quarantine, --cask and fallback
"git pull" updating of the "perforce" package.

As can be seen in [1], [2] and [3] these were workarounds for various
past CI issues. Running "brew install perforce" works now in GitHub
CI, so there's no need to alternate between package names, and the
"git pull" method was a workaround for some staleness issue on the
Azure pipelines removed in [4].

We do have a really common issue with this failing, but that's
unrelated to any of those past fixes, and removing these old
workarounds makes dealing with that a lot easier.

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. 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11)

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

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dbcebad2fb2..82fa87f97af 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -37,13 +37,7 @@ 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
+	brew install perforce
 
 	if test -n "$CC_PACKAGE"
 	then
-- 
2.36.0.879.g56a83971f3f

Re: [PATCH v2 1/3] CI: run "brew install perforce" without past workarounds

[Carlo Arenas]

On Fri, Apr 22, 2022 at 2:07 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index dbcebad2fb2..82fa87f97af 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -37,13 +37,7 @@ 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
> +       brew install perforce

While this might work under the current VM configuration used by
github actions, is definitely not the usual configuration in macOS
installations and therefore likely to break if run locally (as some
other on the fly changes attempt to suggest)

keeping the "--no-quarantine" makes for a less likely to fail option
(since SIP is enabled by default), and therefore I am also concerned
that by removing all these other (learned the hard way) workarounds we
might be making this more fragile for the future as well.

instead of this rewrite of the brew interface logic, removing brew as
you suggested would be probably better.

Carlo

Re: [PATCH v2 1/3] CI: run "brew install perforce" without past workarounds

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 22 2022, Carlo Arenas wrote:

> On Fri, Apr 22, 2022 at 2:07 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
>> index dbcebad2fb2..82fa87f97af 100755
>> --- a/ci/install-dependencies.sh
>> +++ b/ci/install-dependencies.sh
>> @@ -37,13 +37,7 @@ 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
>> +       brew install perforce
>
> While this might work under the current VM configuration used by
> github actions, is definitely not the usual configuration in macOS
> installations and therefore likely to break if run locally (as some
> other on the fly changes attempt to suggest)
>
> keeping the "--no-quarantine" makes for a less likely to fail option
> (since SIP is enabled by default), and therefore I am also concerned
> that by removing all these other (learned the hard way) workarounds we
> might be making this more fragile for the future as well.

It works with the current CI, and keeping those fallbacks would have
meant turning this into some for-loop where we track which command
variant failed exactly, then retrying that with the SHA-256 munging.

> instead of this rewrite of the brew interface logic, removing brew as
> you suggested would be probably better.

I won't have time to pursue that in the near future, sorry. Especially
as I've got no OSX box, so any changes to this series mean long painful
bouncing around against the GitHub CI.

Anyway. I'd be happy for you to pick this up, whether that means
re-rolling my series with your suggested changes or taking yours over
mine I really don't care, I just want to not see those OSX CI errors
anymore.

As noted I have a mild preference out of general principle of having the
CI at least somewhat deterministic, i.e. to do this series where if we
can't get p4 at all we'll fail.

But I'd be fine either way, i.e. your series is also fine by
me. Whatever stops these errors from happening whenever perforce.com
updates the packages & the brew recipes haven't been bumped yet...

Thanks!

[PATCH v2 2/3] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Ævar Arnfjörð Bjarmason]

As can be seen in the commit history[1] of the upstream perforce.rb in
homebrew-cask the upstream perforce package URL and its SHA-256 are
aren't a unique pair. The upstream will start publishing an updated
package at the same URL as the previous version, causing the CI to
routinely fail with errors like:

	==> Downloading https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz
	Error: SHA256 mismatch
	Expected: ffc757b9d4d0629b2594e2777edfb18097825e29c70d8f33a444c7482d622806
	  Actual: 37bc306f0bdfd1d63cfcea113ada132d96f89d53cbb20c282735d51d06223054

Once someone gets around to updating the perforce.rb the failure of
git's CI will be cleared up, but in the meantime all osx-{gcc,clang}
jobs will encounter hard failures.

Let's not be so anal about this and fallback to a "sha256 :no_check"
on failure. For the "ubuntu-latest" jobs just a few lines earlier we
"wget" and chmod binaries from http://filehost.perforce.com (no
https!).

We're already trusting the DNS in the CI to do the right thing here,
and for there not to be any MitM attacks between the CI and
filehost.perforce.com. Even if someone managed to get in the middle
the worst they could do is run arbitrary code in the CI environment.

The only thing we were getting out of the SHA-256 check was to serve
as a canary that the homebrew-cask repository itself was drifting out
of date. It seems sensible to just cut it out as a middle-man
entirely (as the ubuntu-latest jobs do). We want to run the
t/*-git-p4-*.sh tests, not to validate the chain of custody between
perforce.com and the homebrew-cask repository.

See [2] for the "no_check" syntax, and [3] for an example of a failure
in "seen" before this change.

It's been suggested as an alternative [4] to simply disable the p4
tests if we can't install the package from homebrew. While I don't
really care, I think that would be strictly worse. Before this change
we've either run the p4 tests or failed, and we'll still fail now if
we can't run the p4 tests.

Whereas skipping them entirely as [4] does is introducing the caveat
that our test coverage in these jobs today might be different than the
coverage tomorrow. If we do want to introduce such dynamic runs to CI
I think they should use the relevant "needs" or "if" features, so that
the UX can make it obvious what was or wasn't dynamically skipped.

1. https://github.com/Homebrew/homebrew-cask/commits/master/Casks/perforce.rb
2. https://docs.brew.sh/Cask-Cookbook#required-stanzas
3. https://github.com/git/git/runs/6104156856?check_suite_focus=true
4. https://lore.kernel.org/git/20220421225515.6316-1-carenas@gmail.com/
---
 ci/install-dependencies.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 82fa87f97af..cab6e04a358 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -37,7 +37,14 @@ macos-latest)
 	test -z "$BREW_INSTALL_PACKAGES" ||
 	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
-	brew install perforce
+	brew install perforce || {
+		echo Installing perforce failed, assuming and working around SHA256 mismatch >&2 &&
+
+		path=$(brew edit --print-path perforce) &&
+		sed 's/\(sha256.\).*/\1:no_check/' <"$path" >"$path".tmp &&
+		mv "$path".tmp "$path" &&
+		brew install perforce
+	}
 
 	if test -n "$CC_PACKAGE"
 	then
-- 
2.36.0.879.g56a83971f3f

Re: [PATCH v2 2/3] CI: don't care about SHA256 mismatch on upstream "perforce" package

[Carlo Arenas]

> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 82fa87f97af..cab6e04a358 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -37,7 +37,14 @@ macos-latest)
>         test -z "$BREW_INSTALL_PACKAGES" ||
>         brew install $BREW_INSTALL_PACKAGES
>         brew link --force gettext
> -       brew install perforce
> +       brew install perforce || {
> +               echo Installing perforce failed, assuming and working around SHA256 mismatch >&2 &&
> +
> +               path=$(brew edit --print-path perforce) &&
> +               sed 's/\(sha256.\).*/\1:no_check/' <"$path" >"$path".tmp &&
> +               mv "$path".tmp "$path" &&
> +               brew install perforce

brew install $path

seems to be a safer way to ensure that brew will use the locally modified cask

Carlo

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

[Ævar Arnfjörð Bjarmason]

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 cab6e04a358..00154a8ef45 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.879.g56a83971f3f