Re: [PATCH v3 2/4] ci: show error message of "p4 -V"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 24, 2022, 16:10 UTC
Message-ID: <221124.86wn7knx1x.gmgdl@evledraar.gmail.com>
In-Reply-To: <20221124153934.12470-3-worldhello.net@gmail.com>

On Thu, Nov 24 2022, Jiang Xin wrote:

Show 31 quoted lines

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> When installing p4 as a dependency, we used to pipe output of "p4 -V" to
> validate the installation, but this would hide potential errors of p4.
> E.g.: A broken p4 installation fails to run.
>
> Add some instructions to show errors of command "p4 -V", so we can see
> why the command output doesn't match.
>
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  ci/install-dependencies.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index f639263a62..291e49bdde 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -83,9 +83,9 @@ esac
>  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.
> +	p4d -V | grep Rev. || { echo >&2 "p4d: bad version"; p4d -V; exit 1; }
>  	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
> -	p4 -V | grep Rev.
> +	p4 -V | grep Rev. || { echo >&2 "p4: bad version"; p4 -V; exit 1; }
>  else
>  	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
>  fi

I think it makes sense to detect this, but the specific remedy makes no sense to me.

I.e. as your CL's CI output shows the reason we want this is because you had a p4d (and p4?) that segfaulted. I can reproduce that locally as:

	wget --quiet https://cdist2.perforce.com/perforce/r16.2/bin.linux26x86_64/p4d -O p4d; chmod +x p4d; ./p4d
	Segmentation fault

But we had the initial command piped into Rev, so we wouldn't show that, just

	$ wget --quiet https://cdist2.perforce.com/perforce/r16.2/bin.linux26x86_64/p4d -O p4d; chmod +x p4d; ./p4d | grep Rev; echo $?
	1

So we want to show that it segfaults, but how it is useful once that command fails to pretend that it's a "bad version?" The issue is that the command can't show the version at all.

	$ { echo >&2 "p4d: bad version"; ./p4d -V; exit 1; }
	p4d: bad version
	Segmentation fault
	exit

I think this should just be a plain:

	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
        p4d -V
        echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
	p4 -V

As we're already under "set -e" (in ci/lib.sh).

I.e. I don't see why we need to work at making the output less verbose, and then if we fail run the command again.

If we really want to make it less verbose and avoid hiding segfault
messages perhaps we could filter out the stuff we don't care about?:
	
	$ ./p4d -V | sed -ne '/[Cc]opyright/d; /[Ll]icense/d; /http/d; /product/d; p'
	Perforce - The Fast Software Configuration Management System.
	Version of OpenSSL Libraries: OpenSSL 1.1.1q  5 Jul 2022
	Version of OpenLDAP Libraries: 2.4.59
	Version of Cyrus SASL Libraries: 2.1.27
	Using the 'SmartHeap' memory manager.
	Rev. P4D/LINUX26X86_64/2021.1/2313999 (2022/07/15).
	
But I don't see the point of that effort, we won't look at these trace
logs unless something fails, so just including the raw output seems most
sensible.