| Message ID | 20220422013911.7646-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ci: avoid perforce/brew issues affecting macOS | expand |
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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. 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 > 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(-) > > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh > index dbcebad2fb2..6de20108775 100755 > --- a/ci/install-dependencies.sh > +++ b/ci/install-dependencies.sh > @@ -78,12 +78,14 @@ linux-gcc-default) > ;; > esac > > -if type p4d >/dev/null && type p4 >/dev/null > +if command -v p4d >/dev/null && type p4 >/dev/null > then > echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)" > p4d -V | grep Rev. > 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" > fi > if type git-lfs >/dev/null > then
Junio C Hamano <gitster@pobox.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. 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(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index dbcebad2fb..e598dc28df 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -78,14 +78,16 @@ linux-gcc-default) ;; esac -if type p4d >/dev/null && type p4 >/dev/null +if type p4d >/dev/null 2>&1 && type p4 >/dev/null 2>&1 then echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)" p4d -V | grep Rev. 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" fi -if type git-lfs >/dev/null +if type git-lfs >/dev/null 2>&1 then echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)" git-lfs version
On Fri, Apr 22, 2022 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote: > > So, how about replacing it with something like this? 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: > On Fri, Apr 22, 2022 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> So, how about replacing it with something like this? > > 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. 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: > > Carlo Arenas <carenas@gmail.com> writes: > > > 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. > > 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. 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
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index dbcebad2fb2..6de20108775 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -78,12 +78,14 @@ linux-gcc-default) ;; esac -if type p4d >/dev/null && type p4 >/dev/null +if command -v p4d >/dev/null && type p4 >/dev/null then echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)" p4d -V | grep Rev. 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" fi if type git-lfs >/dev/null then
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(-)