All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org, avarab@gmail.com, sunshine@sunshineco.com
Subject: Re: [PATCH v2 1/2] ci: make failure to find perforce more user friendly
Date: Thu, 21 Apr 2022 22:49:16 -0700	[thread overview]
Message-ID: <xmqqo80td5wj.fsf@gitster.g> (raw)
In-Reply-To: <20220422013911.7646-2-carenas@gmail.com> ("Carlo Marcelo Arenas =?utf-8?Q?Bel=C3=B3n=22's?= message of "Thu, 21 Apr 2022 18:39:10 -0700")

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

  reply	other threads:[~2022-04-22  5:49 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-21 22:55 [PATCH] ci: make perforce installation optional in macOS Carlo Marcelo Arenas Belón
2022-04-21 22:58 ` Eric Sunshine
2022-04-21 23:05 ` Eric Sunshine
2022-04-22  1:39 ` [PATCH v2 0/2] ci: avoid perforce/brew issues affecting macOS Carlo Marcelo Arenas Belón
2022-04-22  1:39   ` [PATCH v2 1/2] ci: make failure to find perforce more user friendly Carlo Marcelo Arenas Belón
2022-04-22  5:49     ` Junio C Hamano [this message]
2022-04-22 22:23       ` Junio C Hamano
2022-04-22 23:13         ` Carlo Arenas
2022-04-22 23:58           ` Junio C Hamano
2022-04-23  0:37             ` Carlo Arenas
2022-04-22  1:39   ` [PATCH v2 2/2] ci: make perforce installation optional in macOS Carlo Marcelo Arenas Belón
2022-04-23 14:25   ` [PATCH v3 0/4] ci: avoid perforce/brew issues affecting macOS Carlo Marcelo Arenas Belón
2022-04-23 14:25     ` [PATCH v3 1/4] ci: make failure to find perforce more user friendly Carlo Marcelo Arenas Belón
2022-04-26  1:12       ` Junio C Hamano
2022-04-23 14:25     ` [PATCH v3 2/4] ci: avoid brew for installing perforce Carlo Marcelo Arenas Belón
2022-04-24  6:43       ` Eric Sunshine
2022-04-26 15:55       ` Johannes Schindelin
2022-04-26 17:07         ` Carlo Arenas
2022-04-23 14:25     ` [PATCH v3 3/4] ci: reintroduce prevention from perforce being quarantined in macOS Carlo Marcelo Arenas Belón
2022-04-24  6:47       ` Eric Sunshine
2022-04-23 14:25     ` [PATCH v3 4/4] CI: use https, not http to download binaries from perforce.com Carlo Marcelo Arenas Belón
2022-05-12 22:39     ` [PATCH v4 0/4] ci: avoid perforce/brew issues affecting macOS Junio C Hamano
2022-05-12 22:39       ` [PATCH v4 1/4] ci: make failure to find perforce more user friendly Junio C Hamano
2022-05-12 22:39       ` [PATCH v4 2/4] ci: avoid brew for installing perforce Junio C Hamano
2022-05-12 22:39       ` [PATCH v4 3/4] ci: reintroduce prevention from perforce being quarantined in macOS Junio C Hamano
2022-05-12 22:39       ` [PATCH v4 4/4] ci: use https, not http to download binaries from perforce.com Junio C Hamano

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=xmqqo80td5wj.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=sunshine@sunshineco.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.