From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Eric Sunshine" <sunshine@sunshineco.com>,
"Derrick Stolee" <stolee@gmail.com>,
"SZEDER Gábor" <szeder.dev@gmail.com>,
"Taylor Blau" <me@ttaylorr.com>,
"Johannes Schindelin" <johannes.schindelin@gmx.de>,
"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2 0/3] CI: don't fail OSX tests due to brew v.s. perforce.com mis-match
Date: Fri, 22 Apr 2022 11:07:36 +0200 [thread overview]
Message-ID: <cover-v2-0.3-00000000000-20220422T085958Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.2-00000000000-20220421T124225Z-avarab@gmail.com>
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
next prev parent reply other threads:[~2022-04-22 9:14 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-21 12:53 [PATCH 0/2] CI: don't fail OSX tests due to brew v.s. perforce.com mis-match Ævar Arnfjörð Bjarmason
2022-04-21 12:53 ` [PATCH 1/2] CI: run "brew install perforce" without past workarounds Ævar Arnfjörð Bjarmason
2022-04-21 12:53 ` [PATCH 2/2] CI: don't care about SHA256 mismatch on upstream "perforce" package Ævar Arnfjörð Bjarmason
2022-04-21 20:47 ` Junio C Hamano
2022-04-21 21:08 ` Ævar Arnfjörð Bjarmason
2022-04-21 21:29 ` Eric Sunshine
2022-04-21 21:38 ` Junio C Hamano
2022-04-21 21:48 ` Eric Sunshine
2022-04-21 22:41 ` Junio C Hamano
2022-04-22 9:22 ` Ævar Arnfjörð Bjarmason
2022-04-21 21:30 ` [PATCH 0/2] CI: don't fail OSX tests due to brew v.s. perforce.com mis-matchy Carlo Marcelo Arenas Belón
2022-04-21 21:39 ` Junio C Hamano
2022-04-22 9:21 ` Ævar Arnfjörð Bjarmason
2022-04-21 21:57 ` [PATCH 0/2] CI: don't fail OSX tests due to brew v.s. perforce.com mis-match Junio C Hamano
2022-04-22 9:07 ` Ævar Arnfjörð Bjarmason [this message]
2022-04-22 9:07 ` [PATCH v2 1/3] CI: run "brew install perforce" without past workarounds Ævar Arnfjörð Bjarmason
2022-04-22 9:52 ` Carlo Arenas
2022-04-22 18:46 ` Ævar Arnfjörð Bjarmason
2022-04-22 9:07 ` [PATCH v2 2/3] CI: don't care about SHA256 mismatch on upstream "perforce" package Ævar Arnfjörð Bjarmason
2022-04-22 11:15 ` Carlo Arenas
2022-04-22 9:07 ` [PATCH v2 3/3] CI: use https, not http to download binaries from perforce.com Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover-v2-0.3-00000000000-20220422T085958Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=carenas@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=me@ttaylorr.com \
--cc=stolee@gmail.com \
--cc=sunshine@sunshineco.com \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).