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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jan 24, 2018, 12:12 UTC
Message-ID: <CAM0VKjkGEfL2j2uzRa9=N5jZFbMMKEHn=UX=thxTAMM_g8TB4g@mail.gmail.com>
In-Reply-To: <20180123162633.GE13068@sigill.intra.peff.net>

On Tue, Jan 23, 2018 at 5:26 PM, Jeff King <peff@peff.net> wrote:

Show 31 quoted lines

> On Mon, Jan 22, 2018 at 02:32:17PM +0100, SZEDER Gábor wrote:
>
>> 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:
>
>>  # 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...
>
>>  # 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 {}.

Indeed, the && chain is broken, I didn't noticed that.

Luckily it was broken in a way that it didn't lead to false successes: if installing dependencies fails, then the first && chain ensures that HOST_UID is not set, and then useradd will error out with "invalid user ID 'ci'", causing the second && chain to abort the script and thus breaking the build.

Will update the commit message accordingly.

Show 9 quoted lines

> 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.

No. It does abort the script, because it isn't part of a conditional. Try to run the script twice in the same container instance: in the second run the user already exists, useradd fails and the whole script aborts.

> 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.

I see you've already made it to patch 4, good :)