From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jiang Xin <worldhello.net@gmail.com>
Cc: Git List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
Johannes Schindelin <johannes.schindelin@gmx.de>,
Jiang Xin <zhiyou.jx@alibaba-inc.com>
Subject: Re: [PATCH v3 2/4] ci: show error message of "p4 -V"
Date: Thu, 24 Nov 2022 17:10:12 +0100 [thread overview]
Message-ID: <221124.86wn7knx1x.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20221124153934.12470-3-worldhello.net@gmail.com>
On Thu, Nov 24 2022, Jiang Xin wrote:
> 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.
next prev parent reply other threads:[~2022-11-24 16:23 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-23 15:02 [PATCH 0/2] Use fixed github-actions runner image Jiang Xin
2022-11-23 15:02 ` [PATCH 1/2] github-actions: run gcc-8 on ubuntu-20.04 image Jiang Xin
2022-11-24 8:11 ` Johannes Schindelin
2022-11-23 15:02 ` [PATCH 2/2] ci: upgrade version of p4 Jiang Xin
2022-11-24 8:16 ` Johannes Schindelin
2022-11-24 8:41 ` Johannes Schindelin
2022-11-24 8:54 ` Johannes Schindelin
2022-11-24 9:17 ` Jiang Xin
2022-11-24 9:41 ` Johannes Schindelin
2022-11-24 9:15 ` Jiang Xin
2022-11-24 8:18 ` [PATCH 0/2] Use fixed github-actions runner image Johannes Schindelin
2022-11-24 9:05 ` [PATCH v2 0/3] Fix broken CI on newer " Jiang Xin
2022-11-24 9:44 ` Johannes Schindelin
2022-11-24 10:48 ` Johannes Schindelin
2022-11-24 11:23 ` Jiang Xin
2022-11-24 12:28 ` python 2 EOL (was: [PATCH v2 0/3] Fix broken CI on newer github-actions runner image) Ævar Arnfjörð Bjarmason
2022-11-25 7:11 ` python 2 EOL Junio C Hamano
2022-11-24 15:39 ` [PATCH v3 0/4] Fix broken CI on newer github-actions runner image Jiang Xin
2022-11-25 9:59 ` [PATCH v4 " Jiang Xin
2022-11-25 9:59 ` [PATCH v4 1/4] github-actions: run gcc-8 on ubuntu-20.04 image Jiang Xin
2022-11-27 0:24 ` Junio C Hamano
2022-11-25 9:59 ` [PATCH v4 2/4] ci: remove the pipe after "p4 -V" to cache errors Jiang Xin
2022-11-27 0:24 ` Junio C Hamano
2022-11-27 9:14 ` Jiang Xin
2022-11-25 9:59 ` [PATCH v4 3/4] ci: p4 on Linux has the same version as on macOS Jiang Xin
2022-11-27 0:28 ` Junio C Hamano
2022-11-25 9:59 ` [PATCH v4 4/4] ci: install python on ubuntu Jiang Xin
2022-11-27 0:30 ` Junio C Hamano
2022-11-27 9:01 ` Jiang Xin
2022-11-27 23:36 ` Junio C Hamano
2022-11-24 15:39 ` [PATCH v3 1/4] github-actions: run gcc-8 on ubuntu-20.04 image Jiang Xin
2022-11-24 16:29 ` Ævar Arnfjörð Bjarmason
2022-11-24 15:39 ` [PATCH v3 2/4] ci: show error message of "p4 -V" Jiang Xin
2022-11-24 16:10 ` Ævar Arnfjörð Bjarmason [this message]
2022-11-25 4:48 ` Junio C Hamano
2022-11-24 15:39 ` [PATCH v3 3/4] ci: p4 on Linux has the same version as on macOS Jiang Xin
2022-11-24 15:39 ` [PATCH v3 4/4] ci: install python on ubuntu Jiang Xin
2022-11-24 9:05 ` [PATCH v2 1/3] github-actions: run gcc-8 on ubuntu-20.04 image Jiang Xin
2022-11-24 10:46 ` Ævar Arnfjörð Bjarmason
2022-11-25 7:21 ` Junio C Hamano
2022-11-24 9:05 ` [PATCH v2 2/3] ci: upgrade version of p4 to 21.2 Jiang Xin
2022-11-24 10:55 ` Ævar Arnfjörð Bjarmason
2022-11-24 12:56 ` Jiang Xin
2022-11-24 9:05 ` [PATCH v2 3/3] ci: install python on ubuntu Jiang Xin
2022-11-24 11:02 ` Ævar Arnfjörð Bjarmason
2022-11-24 11:37 ` Jiang Xin
2022-11-24 12:23 ` Ævar Arnfjörð Bjarmason
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=221124.86wn7knx1x.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=worldhello.net@gmail.com \
--cc=zhiyou.jx@alibaba-inc.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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).