Re: [PATCH v1] Travis: also test on 32-bit Linux

Hi Lars,


On Thu, 2 Mar 2017, Lars Schneider wrote:

quoted

On 02 Mar 2017, at 12:24, Johannes Schindelin [off-list ref] wrote:

On Thu, 2 Mar 2017, Lars Schneider wrote:

quoted

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

quoted

quoted

(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

quoted

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.

quoted

quoted

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

quoted

quoted

+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