Re: [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job
On Wed, Jan 24, 2018 at 01:12:48PM +0100, SZEDER Gábor wrote: > >> -test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) && > [...] > 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. True. :) I didn't even look closely at whether the failures could be correlated. > >> +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. You're right. I was confusing it with this case: (one; two) || three in which we continue with "two" even if "one" fails. But it's only the left-hand side of the || that does that. -Peff
Re: [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job
On Tue, Jan 23, 2018 at 5:26 PM, Jeff King wrote:
> 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.
> 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 :)
Re: [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job
On Tue, Jan 23, 2018 at 11:26:33AM -0500, Jeff King wrote: > > +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. OK, nevermind on all this. I just read patch 4. :) -Peff
Re: [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job
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 {}.
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
[PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job
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. Enable 'set -e' for the whole script and for the commands executed under 'su' as well. While touching every line in the 'su' command block anyway, change their indentation to use a tab instead of spaces. Signed-off-by: SZEDER Gábor --- ci/run-linux32-build.sh | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh index 5a36a8d7c0..248183982b 100755 --- a/ci/run-linux32-build.sh +++ b/ci/run-linux32-build.sh @@ -6,29 +6,29 @@ # run-linux32-build.sh [host-user-id] # -set -x +set -ex # 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 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) && +HOST_UID=$1 +CI_USER=$USER +test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) # Build and test linux32 --32bit i386 su -m -l $CI_USER -c ' -set -x && -cd /usr/src/git && -ln -s /tmp/travis-cache/.prove t/.prove && -make --jobs=2 && -make --quiet test + set -ex + cd /usr/src/git + ln -s /tmp/travis-cache/.prove t/.prove + make --jobs=2 + make --quiet test ' -- 2.16.1.80.gc0eec9753d
