Re: [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job

From: Jeff King <peff@peff.net>
Date: Jan 23, 2018, 16:26 UTC
Message-ID: <20180123162633.GE13068@sigill.intra.peff.net>
In-Reply-To: <20180122133220.18587-3-szeder.dev@gmail.com>

On Mon, Jan 22, 2018 at 02:32:17PM +0100, SZEDER Gábor wrote:

Show 5 quoted lines

> All 'ci/*' scripts use 'set -e' to break the build job if a command
> fails, except 'ci/run-linux32-build.sh' which relies on the && chain
> to do the same.  This inconsistency among the 'ci/*' scripts is asking
> for trouble: I forgot about the && chain more than once while working
> on this patch series.

I think this actually fixes a bug:

Show 7 quoted lines

>  # Update packages to the latest available versions
>  linux32 --32bit i386 sh -c '
>      apt update >/dev/null &&
>      apt install -y build-essential libcurl4-openssl-dev libssl-dev \
>  	libexpat-dev gettext python >/dev/null
> -' &&
> +'

If this step failed, then...

Show 8 quoted lines

>  # If this script runs inside a docker container, then all commands are
>  # usually executed as root. Consequently, the host user might not be
>  # able to access the test output files.
>  # If a host user id is given, then create a user "ci" with the host user
>  # id to make everything accessible to the host user.
> -HOST_UID=$1 &&
> -CI_USER=$USER &&
> -test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) &&

We'd have triggered the right-hand side of this "||", and run the rest of the script. The whole "||" block should have been inside {}.

But after your patch, it should be fine with "set -e". Although...

> +HOST_UID=$1
> +CI_USER=$USER
> +test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER)

If this "useradd" step fails, we wouldn't abort the script, because it's part of a conditional. You'd need a manual "|| exit 1" at the end of this line. Or to use a real "if" block.

Reading this line, I'm also slightly confused by setting CI_USER in the subshell, and then only using it once. Should it be respecting the outer CI_USER setting? If not, should it just hard-code "ci" on the useradd command-line? But that has nothing to do with your patch here.

-Peff