Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
> > On 02 Mar 2017, at 12:24, Johannes Schindelin <johannes.schinde...@gmx.de>
> > wrote:
> >
> > On Thu, 2 Mar 2017, Lars Schneider wrote:
> >
> >> The patch looks good to me in general but I want to propose the
> >> following changes:
> >
> > I know you are using your script to generate this mail, but I would
> > have liked to see v2 in the subject ;-)
>
> Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in my
> email client. Now I am wondering... is the next version v2 or v3 :D
Since there was no v2, the next one should *definitely* be v2... ;-)
> >> (1) Move all the docker magic into a dedicated file
> >> "ci/run-linux-32-build.sh" This way people should be able to run this
> >> build on their local machines without TravisCI. However, I haven't
> >> tested this.
> >
> > I considered this, but there is serious overlap: the `docker pull`
> > call and the `docker run` call *have* to refer to the same image. It's
> > very easy for them to get out of sync if you have that information in
> > two files. Maybe make that an option of the script, defaulting to
> > daald/ubuntu32:xenial?
>
> Right. I missed that. How about something like that?
>
> before_install:
> - ci/run-linux32-build.sh --pull-container
> before_script:
> script: ci/run-linux32-build.sh
I'd prefer
before_install:
- docker pull daald/ubuntu32:xenial
before_script:
script: ci/run-linux32-build.sh daald/ubuntu32:xenial
> > BTW speaking of Docker: it would be nicer if there was a Docker image
> > that already had the build-essentials installed, to save on startup
> > time. But I did not find any that was reasonably up-to-date.
>
> True. But installing everything just takes a minute and we don't need to
> maintain anything...
And when there are network problems (like there were on Tuesday, right
when I developed the first v1 of this patch) then we have another set of
problems that make Travis fail. Even if the code in the PR or branch is
actually good. I'd like to avoid false positives, if possible.
> >> +set -e
> >
> > Is this really necessary? I really like to avoid `set -e`, in
> > particular when we do pretty much everything in && chains anyway.
>
> Agreed, not really necessary here as we just invoke one command. Out of
> curiosity: Why do you try to avoid it? I set it by default in all my
> scripts.
I try to avoid it because it encourages a style that omits helpful error
messages.
> >> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
> >> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
> >> +
> >> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
> >> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
> >> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
> >> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
> >> +
> >> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
> >> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
> >> +
> >> +sudo docker run \
> >> + --interactive --volume "${PWD}:/usr/src/git" \
> >> + daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
> >
> > Hmm. Since it is a script now, it would be more readable this way, I
> > think:
> >
> > sudo docker run --volume "${PWD}:/usr/src/git"
> > "${1:-daald/ubuntu32:xenial}" \
> > linux32 --32bit i386 sh -c '
> > : update packages first &&
> > apt update >/dev/null &&
> > apt install -y build-essential libcurl4-openssl-dev libssl-dev \
> > libexpat-dev gettext python >/dev/null &&
> >
> > : now build and test &&
> > cd /usr/src/git &&
> > DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
> > GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
> > GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
> > GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
> > make -j2 test
> > '
>
> That looks better! I'll try it!
Thanks!
Dscho
