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

Commit Message

Ævar Arnfjörð Bjarmason April 21, 2022, 12:53 p.m. UTC

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(-)

Comments

Junio C Hamano April 21, 2022, 8:47 p.m. UTC | #1

Ævar Arnfjörð Bjarmason  <[e-mail]> 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

Ævar Arnfjörð Bjarmason April 21, 2022, 9:08 p.m. UTC | #2

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

> Ævar Arnfjörð Bjarmason  <[e-mail]> 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

Eric Sunshine April 21, 2022, 9:29 p.m. UTC | #3

On Thu, Apr 21, 2022 at 5:10 PM Ævar Arnfjörð Bjarmason
<[e-mail]> wrote:
> On Thu, Apr 21 2022, Junio C Hamano wrote:
> > Ævar Arnfjörð Bjarmason  <[e-mail]> 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 ...

Junio C Hamano April 21, 2022, 9:38 p.m. UTC | #4

Eric Sunshine <[e-mail]> 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
 	}

Eric Sunshine April 21, 2022, 9:48 p.m. UTC | #5

On Thu, Apr 21, 2022 at 5:38 PM Junio C Hamano <[e-mail]> wrote:
> Eric Sunshine <[e-mail]> 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.

Junio C Hamano April 21, 2022, 10:41 p.m. UTC | #6

Eric Sunshine <[e-mail]> writes:

> On Thu, Apr 21, 2022 at 5:38 PM Junio C Hamano <[e-mail]> wrote:
>> Eric Sunshine <[e-mail]> 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.

Ævar Arnfjörð Bjarmason April 22, 2022, 9:22 a.m. UTC | #7

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

> Eric Sunshine <[e-mail]> writes:
>
>> On Thu, Apr 21, 2022 at 5:38 PM Junio C Hamano <[e-mail]> wrote:
>>> Eric Sunshine <[e-mail]> 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/[e-mail]/

Patch

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