On Thu, Nov 24 2022, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
>
> 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@xxxxxx>
> Signed-off-by: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
> ---
> 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.
