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