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
