patch, 2 partsCI: don't fail OSX tests due to brew v.s. perforce.com mis-match

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

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

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

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

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

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

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

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

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

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.

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

On Thu, Apr 21, 2022 at 5:10 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

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

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

On Thu, Apr 21, 2022 at 5:38 PM Junio C Hamano <gitster@pobox.com> wrote:

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.

Eric Sunshine <sunshine@sunshineco.com> writes:

Yeah, that is fine, too.

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

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

On Thu, Apr 21, 2022 at 02:53:50PM +0200, Ævar Arnfjörð Bjarmason wrote:

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

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

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

Thanks.

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

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/

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

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

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

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

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

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

brew install $path

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

Carlo

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

On Fri, Apr 22, 2022 at 2:07 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

On Fri, Apr 22 2022, Carlo Arenas wrote:

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.

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!