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

Message

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

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/[e-mail]/
     
      ## 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