| Message ID | 20220423142559.32507-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d1c9195116beb18b2e8d9caf37d21289bb5f48b3 |
| Headers | show |
| Series | ci: avoid perforce/brew issues affecting macOS | expand |
On Sat, Apr 23, 2022 at 10:26 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote: > Perfoce's cask in brew is meant[1] to be used only by humans, so replace > its use from the CI with a scripted binary download which is less likely > to fail, as it is done in Linux. s/Perfoce/Perforce/ > Kept the logic together so it will be less likely to break when moved > around as on the fly code changes in this area are settled, at which > point it will also feasable to ammend it to avoid some of the hardcoded > values by using similar variables to the ones Linux does. s/Kept/Keep/ s/also/also be/ s/feasable/feasible/ s/ammend/amend/ > In that same line, a POSIX sh syntax is used instead of the similar one > used in Linux in preparation for an unrelated future change that might > change the shell currently configured for it. > > This change reintroduces the risk that the installed binaries might not > work because of being quarantined that was fixed with 5ed9fc3fc86 (ci: > prevent `perforce` from being quarantined, 2020-02-27) but fixing that > now was also punted for simplicity and since the affected cloud provider > is scheduled to be retired with an on the fly change, but should be > addressed if that other change is not integrated further. > > The discussion on the need to keep 2 radically different versions of > the binaries to be tested with Linux vs macOS or how to upgrade to > newer versions now that brew won't do that automatically for us has > been punted for now as well. On that line the now obsolete comment > about it in lib.sh was originally being updated by this change but > created conflicts as it is moved around by other on the fly changes, > so will be addressed independently as well. > > [1] https://github.com/Homebrew/homebrew-cask/pull/122347#discussion_r856026584 > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Hi Carlo, On Sat, 23 Apr 2022, Carlo Marcelo Arenas Belón wrote: > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh > index 41e9290fbdd..9da03350d09 100755 > --- a/ci/install-dependencies.sh > +++ b/ci/install-dependencies.sh > @@ -37,13 +37,14 @@ macos-latest) > test -z "$BREW_INSTALL_PACKAGES" || > brew install $BREW_INSTALL_PACKAGES > brew link --force gettext > - brew install --cask --no-quarantine perforce || { > - # Update the definitions and try again > - cask_repo="$(brew --repository)"/Library/Taps/homebrew/homebrew-cask && > - git -C "$cask_repo" pull --no-stat --ff-only && > - brew install --cask --no-quarantine perforce > - } || > - brew install homebrew/cask/perforce > + mkdir -p $HOME/bin > + ( > + cd $HOME/bin > + wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" && I vaguely recall that Gábor Szeder attempted something similar, but I _think_ that ultimately there were too many moving parts in that URL that we did not want to hardcode. For example, when opening a PR against `maint` or an even older topic branch, we are stuck with the exact code in that revision that obtains the perforce package. But since that server is outside our control, it can very well evolve to a state that is not amenable to hard-coding such a URL. While I don't think that we can solve this fully, I would prefer to keep the existing `brew install` calls but fall back to downloading from a hard-coded URL. That still would have the shortcoming of hard-coded `r21.2` and `1015`, but it should fail less often than the proposed patch. For the record, the recent problems stem from the fact that the package was cached on GitHub's build agents (I guess to avoid many identical downloads), and the cached package did not match what was recorded in the updated package definition. Which means that those `brew` errors are only transient, until a new VM image is built that caches the then-current perforce package. Ciao, Dscho > + tar -xf helix-core-server.tgz > + ) > + PATH="$PATH:${HOME}/bin" > + export PATH > > if test -n "$CC_PACKAGE" > then > -- > 2.36.0.266.g59f845bde02 > >
On Tue, Apr 26, 2022 at 8:55 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > On Sat, 23 Apr 2022, Carlo Marcelo Arenas Belón wrote: > > > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh > > index 41e9290fbdd..9da03350d09 100755 > > --- a/ci/install-dependencies.sh > > +++ b/ci/install-dependencies.sh > > @@ -37,13 +37,14 @@ macos-latest) > > test -z "$BREW_INSTALL_PACKAGES" || > > brew install $BREW_INSTALL_PACKAGES > > brew link --force gettext > > - brew install --cask --no-quarantine perforce || { > > - # Update the definitions and try again > > - cask_repo="$(brew --repository)"/Library/Taps/homebrew/homebrew-cask && > > - git -C "$cask_repo" pull --no-stat --ff-only && > > - brew install --cask --no-quarantine perforce > > - } || > > - brew install homebrew/cask/perforce > > + mkdir -p $HOME/bin > > + ( > > + cd $HOME/bin > > + wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" && > > I vaguely recall that Gábor Szeder attempted something similar, but I > _think_ that ultimately there were too many moving parts in that URL that > we did not want to hardcode. I can see that, and indeed that is also why those series still keep perforce installation as optional so it wouldn't block CI when it does. BTW most of the reasons why those values are hardcoded here is just to avoid conflicts with on the fly changes, and making it smarter and avoiding the hardcoding would be done as a prerequisite to making it mandatory again (if that is what is preferred) > While I don't think that we can solve this fully, I would prefer to keep > the existing `brew install` calls but fall back to downloading from a > hard-coded URL. Considering that the expectation from brew maintainers (as documented[1]) is that those mismatches would happen and should be solved manually and that perforce releases just do inplace binary pushes I still stand by my suggestion to avoid brew, but it might be useful as a fallback of the wget approach if that helps with your concerns above. > For the record, the recent problems stem from the fact that the package > was cached on GitHub's build agents (I guess to avoid many identical > downloads), and the cached package did not match what was recorded in the > updated package definition. Which means that those `brew` errors are only > transient, until a new VM image is built that caches the then-current > perforce package. FWIW as I updated the brew cask[2], I realized that livecheck pointed to an even newer release, so if we are adding brew back it might be worth adding yet another workaround to delete the cached file and try without a SHA as Ævar suggested. Carlo [1] https://docs.brew.sh/Common-Issues#cask---checksum-does-not-match [2] https://github.com/Homebrew/homebrew-cask/pull/122347
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 41e9290fbdd..9da03350d09 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -37,13 +37,14 @@ macos-latest) test -z "$BREW_INSTALL_PACKAGES" || brew install $BREW_INSTALL_PACKAGES brew link --force gettext - brew install --cask --no-quarantine perforce || { - # Update the definitions and try again - cask_repo="$(brew --repository)"/Library/Taps/homebrew/homebrew-cask && - git -C "$cask_repo" pull --no-stat --ff-only && - brew install --cask --no-quarantine perforce - } || - brew install homebrew/cask/perforce + mkdir -p $HOME/bin + ( + cd $HOME/bin + wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz" && + tar -xf helix-core-server.tgz + ) + PATH="$PATH:${HOME}/bin" + export PATH if test -n "$CC_PACKAGE" then
Perfoce's cask in brew is meant[1] to be used only by humans, so replace its use from the CI with a scripted binary download which is less likely to fail, as it is done in Linux. Kept the logic together so it will be less likely to break when moved around as on the fly code changes in this area are settled, at which point it will also feasable to ammend it to avoid some of the hardcoded values by using similar variables to the ones Linux does. In that same line, a POSIX sh syntax is used instead of the similar one used in Linux in preparation for an unrelated future change that might change the shell currently configured for it. This change reintroduces the risk that the installed binaries might not work because of being quarantined that was fixed with 5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27) but fixing that now was also punted for simplicity and since the affected cloud provider is scheduled to be retired with an on the fly change, but should be addressed if that other change is not integrated further. The discussion on the need to keep 2 radically different versions of the binaries to be tested with Linux vs macOS or how to upgrade to newer versions now that brew won't do that automatically for us has been punted for now as well. On that line the now obsolete comment about it in lib.sh was originally being updated by this change but created conflicts as it is moved around by other on the fly changes, so will be addressed independently as well. [1] https://github.com/Homebrew/homebrew-cask/pull/122347#discussion_r856026584 Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- ci/install-dependencies.sh | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)