patchci: make perforce installation optional in macOS

Using brew to install perforce has several documented[1,2,3,4] edge cases that make it fail periodically, so if problems were found while installing it, just continue without it.

This means that until the problem is solved all perforce tests will be skipped in macOS, but they are still most likely covered by the other unaffected runs and will be covered again once the issue solves itself.

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 https://lore.kernel.org/git/cover-0.2-00000000000-20220421T124225Z-avarab@gmail.com/ --- This is based on master and can merge upwards except with seen where it conflicts with the recently added ab/ci-osx-loosen-package-requirement which it is meant to replace.

A successful sample run when merged with master available in :

  https://github.com/carenas/git/actions/runs/2204519374

 ci/install-dependencies.sh | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
wi

On Thu, Apr 21, 2022 at 6:55 PM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

Missing sign-off.

On Thu, Apr 21, 2022 at 6:55 PM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

You do get a vaguely alarming error message[1] with the solution implemented by this patch:

    ci/install-dependencies.sh: line 81: type: p4d: not found

However, that's an unrelated and existing minor mistake (outside of the scope of this fix, probably). It should be using `command -v` rather than `type` in the couple conditionals at the bottom of the script.

[1]: https://github.com/carenas/git/runs/6119943232?check_suite_focus=true#step:3:116

This series is a simpler alternative to ab/ci-osx-loosen-package-requirement which while it doesn't address the underlying issue, avoids the CI failures in macOS as well.

For an example of a successful run with it merged with next see:

  https://github.com/carenas/git/actions/runs/2205005763

Carlo Marcelo Arenas Belón (2):
  ci: make failure to find perforce more user friendly
  ci: make perforce installation optional in macOS

 ci/install-dependencies.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

In preparation for a future change that will make perforce installation optional in macOS, make sure that the check for it is done without triggering scary looking errors and add a user friendly message instead.

Only one invocation of type is changed as that is what is only needed for the expected future use case, and because `type` is recommended in the CodingGuidelines, so changing that recommendation or a more complex change has been specifically punted.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 ci/install-dependencies.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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

This may be in the "POSIX may say this but the real world may not work that way" territory. As far as I can tell, "command -v" [*1*] and "type" [*2*] both ought to give diagnostic messages to their standard error stream, and they both should signal an error with non-zero exit status. It may be that the shell implementation you have tried had "command -v" that is less noisy than "type" when given a command that is not installed, but I wonder if the next shell implementation you find has "command -v" that is as noisy and scary as "type", in which case this patch amounts to a no-op.

I wonder if "type p4d >/dev/null 2>/dev/null" (or "command -v" with the same) is a more futureproof fix.

[References]

*1* https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html *2* https://pubs.opengroup.org/onlinepubs/9699919799/utilities/type.html

Junio C Hamano <gitster@pobox.com> writes:

So, how about replacing it with something like this?

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
From: Carlo Marcelo Arenas Belón <carenas@gmail.com>

In preparation for a future change that will make perforce installation optional in macOS, make sure that the check for it is done without triggering scary looking errors and add a user friendly message instead.

All other existing uses of 'type <cmd>' in our shell scripts that check the availability of a command <cmd> send both standard output and error stream to /dev/null to squelch "<cmd> not found" diagnostic output, but this script left the standard error stream shown.

Redirect it just like everybody else to squelch this error message that we fully expect to see.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On Fri, Apr 22, 2022 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote:

Agree 100%.

It would theoretically make the issue raised by Ævar of not knowing when perforce was skipped slightly worse but getting that fixed would seem like something that could be done in a follow up.

I have to also admit, with all the on the fly changes to these same files, it might be wiser to wait until later anyway.

Carlo

Carlo Arenas <carenas@gmail.com> writes:

Actually, I was thinking about taking these two (possibly with Ævar's "download with https://, we are in 21st century" change to make them 3-patch series) and fast-tracking.

The homebrew specific "packagers can take a bit of time to catch up and we may see hash mismatch" and other tweaks Ævar has can come on top once we see the basic "any parts of tests can fail and the rest can still be tested, why does it have to be world-stopping if we fail to install p4?" working.

Thanks.

On Fri, Apr 22, 2022 at 4:58 PM Junio C Hamano <gitster@pobox.com> wrote:

Sounds like a good plan to me, and unlikely to introduce much pain to all other on the fly changes, but I think it is worth mentioning that some kind soul did fix the mismatch in homebrew already, so there is not really an urgent need for fast-tracking right now either.

Carlo

Using brew to install perforce has several documented[1,2,3,4] edge cases that make it fail periodically, so if problems were found while installing it, just continue without it.

This means that until the problem is solved all perforce tests will be skipped in macOS, but they are still most likely covered by the other unaffected runs and will be covered again once the issue solves itself.

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 https://lore.kernel.org/git/cover-0.2-00000000000-20220421T124225Z-avarab@gmail.com/

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This series is an alternative to ab/ci-osx-loosen-package-requirement and includes the parts proposed on it in v2 that didn't conflict.

The first patch is just a slightly modified version from the one proposed by Junio and that adds also a message if git-lfs wasn't found just like we do now for perforce while making those messages go to stderr.

The second patch replaces the original proposal with a more complicated one that removes the use of brew as suggested by Ævar, it is also a little bigger than it really needs to be just to make sure it won't conflict with on the fly changes, and for the same reason uses some hardcoded values that would be worth fixing after.

The third patch is only needed to avoid regressions with Azure Pipelines which might be deprecated as proposed in other on the fly changes, so it might not be needed in seen, but is still needed if this progresses further first. It was made independent so it could be easy to revert if Azure support is going away just like TravisCI did.

The last patch was taken from Ævar's v2 and is the only one that will require changes in other core on the fly but the conflict resolution is trivial, and more importantly doesn't conflict when merged to seen in the current spot this branch uses.

For an example of a "successful" run with it merged on top of seen (with the previous iteration reverted) see:

  https://github.com/carenas/git/runs/6140236148

Note that perforce no longer fails because someone fixed the issue with their Cask but still most of the new code is excercised correctly.

This introduces a problem identified by Ævar of making the CI runs less consistent, as they might be skipping some of the perforce integrations if installing that fails again, but the likelyhood of that failing has been reduced by removing the inherent brew flakyness, and so improving the UI so that it will be more visible when that happens is probably something that can be addressed later, once all other CI related changes are settled, or we could reintroduce the hard dependency on installing perforce for macOS (just like it is done in Linux)

Carlo Marcelo Arenas Belón (3):
  ci: make failure to find perforce more user friendly
  ci: avoid brew for installing perforce
  ci: reintroduce prevention from perforce being quarantined in macOS

Ævar Arnfjörð Bjarmason (1):
  CI: use https, not http to download binaries from perforce.com

 ci/install-dependencies.sh | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

In preparation for a future change that will make perforce installation optional in macOS, make sure that the check for it is done without triggering scary looking errors and add a user friendly message instead.

All other existing uses of 'type <cmd>' in our shell scripts that check the availability of a command <cmd> send both standard output and error stream to /dev/null to squelch "<cmd> not found" diagnostic output, but this script left the standard error stream shown.

Redirect it just like everybody else to squelch this error message that we fully expect to see.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 ci/install-dependencies.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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

NO!  Why do we want to create a file whose name is "2" here?

Good that I caught them before I merged them to 'next'.

Will locally amend and see if it fixes CI failure in 'seen'

Thanks.

5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27) introduces this prevention for brew, but brew has been removed in a previous commit, so reintroduce an equivalent option to avoid a possible regression.

This doesn't affect github actions (as configure now) and is therefore done silently to avoid any possible scary irrelevant messages.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 ci/install-dependencies.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Sat, Apr 23, 2022 at 10:26 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

s/configure/configured/

I don't see the point of the `|| true` since the code doesn't care about the eventual outcome of the &&-chain. (The &&-chain itself makes sense, though, to avoid executing commands pointlessly if an earlier step failed.) May not be worth a reroll, though.

From: Ævar Arnfjörð Bjarmason <avarab@gmail.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(-)

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

On Sat, Apr 23, 2022 at 10:26 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

s/Perfoce/Perforce/

s/Kept/Keep/ s/also/also be/ s/feasable/feasible/ s/ammend/amend/

Hi Carlo,

On Sat, 23 Apr 2022, Carlo Marcelo Arenas Belón wrote:

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

On Tue, Apr 26, 2022 at 8:55 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

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)

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.

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

We see macOS CI job failures from time to time when the version of p4 brew expects is slightly older than what is offered at Perforce's download site. Instead of failing to run the tests altogether, just skip "git-p4" tests, like we do for tests that depends on any other optional packages, when this happens.

This is essentially unchanged from the v3 posted by Carlo on April 23rd, with a few fix-ups that have been cooking on top squashed in.

The changes from the previous round (i.e. the "fix-ups" that have been floating on top, which got squashed in) are:

 * The first step has a fix to a couple of wrong redirections that
   created a junk file whose name is "2", instead of sending the
   messages to the standard error.

 * The last step makes two places that we download Perforce packages
   to use https://cdist2.perforce.com/ (earlier one of the places
   used that, the other https://filehost.perforce.com/).

I am sending this out as a final "complain now, or this will go to 'next' soonish" warning. The "What's cooking" report is getting crowded with too many topics marked as "Expecting a reroll", and I'm trying to do easier ones myself to see how much reduction we can make.

Carlo Marcelo Arenas Belón (3):
  ci: make failure to find perforce more user friendly
  ci: avoid brew for installing perforce
  ci: reintroduce prevention from perforce being quarantined in macOS

Ævar Arnfjörð Bjarmason (1):
  ci: use https, not http to download binaries from perforce.com

 ci/install-dependencies.sh | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

1:  5730a749bc ! 1:  024459486c ci: make failure to find perforce more user friendly
    @@ ci/install-dependencies.sh: linux-gcc-default)
      	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
      	p4 -V | grep Rev.
     +else
    -+	echo "WARNING: perforce wasn't installed, see above for clues why" >2
    ++	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
      fi
     -if type git-lfs >/dev/null
     +if type git-lfs >/dev/null 2>&1
    @@ ci/install-dependencies.sh: linux-gcc-default)
      	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
      	git-lfs version
     +else
    -+	echo "WARNING: git-lfs wasn't installed, see above for clues why" >2
    ++	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
      fi
2:  8cd96645ae = 2:  8717dbe8d9 ci: avoid brew for installing perforce
3:  1f06d0ba06 = 3:  6a4f085d63 ci: reintroduce prevention from perforce being quarantined in macOS
4:  6bf267b995 ! 4:  5be72d9150 ci: use https, not http to download binaries from perforce.com
    @@ Commit message
         at least, and is one less thing to worry about when checking where
         else we rely on non-TLS'd http connections.
     
    +    Also, use the same download site at perforce.com for Linux and macOS
    +    tarballs for consistency.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## ci/install-dependencies.sh ##
    @@ ci/install-dependencies.sh
      . ${0%/*}/lib.sh
      
     -P4WHENCE=http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
    -+P4WHENCE=https://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
    ++P4WHENCE=https://cdist2.perforce.com/perforce/r$LINUX_P4_VERSION
      LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
      UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
       tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl

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

In preparation for a future change that will make perforce installation optional in macOS, make sure that the check for it is done without triggering scary looking errors and add a user friendly message instead.

All other existing uses of 'type <cmd>' in our shell scripts that check the availability of a command <cmd> send both standard output and error stream to /dev/null to squelch "<cmd> not found" diagnostic output, but this script left the standard error stream shown.

Redirect it just like everybody else to squelch this error message that we fully expect to see.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

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

5ed9fc3fc86 (ci: prevent `perforce` from being quarantined, 2020-02-27) introduces this prevention for brew, but brew has been removed in a previous commit, so reintroduce an equivalent option to avoid a possible regression.

This doesn't affect github actions (as configure now) and is therefore done silently to avoid any possible scary irrelevant messages.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.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.

Also, use the same download site at perforce.com for Linux and macOS tarballs for consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)