On Thu, Apr 21 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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
