| Message ID | 20221124153934.12470-3-worldhello.net@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix broken CI on newer github-actions runner image | expand |
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.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> 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. > ... > 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 > ... > 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. Yup, makes sense. Thanks, both, for working on this topic.
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