Travis: Build tests on Ubuntu 18.04 with docker

ken2812221 commented at 10:18 am on May 11, 2018: contributor

Compile and run tests on Ubuntu 18.04 docker.

fanquake added the label Tests on May 11, 2018

ken2812221 force-pushed on May 11, 2018

ken2812221 renamed this:
~~[WIP] Travis: Build tests on Ubuntu 18.04 with docker~~
Travis: Build tests on Ubuntu 18.04 with docker
on May 11, 2018

ken2812221 force-pushed on May 11, 2018

MarcoFalke commented at 2:42 pm on May 11, 2018: member

I can’t see any downside, so Concept ACK.

promag commented at 2:45 pm on May 11, 2018: member

Can you elaborate the motivation behind this change?

in .travis.yml:49 in 585d16cc80 outdated

52 install:
53-    - if [ -n "$DPKG_ADD_ARCH" ]; then sudo dpkg --add-architecture "$DPKG_ADD_ARCH" ; fi
54-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get update; fi
55-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get install --no-install-recommends --no-upgrade -qq $PACKAGES; fi
56+    - env | grep -vE '^(LC_.*|LANG)=' > /tmp/env
57+    - DOCKER_ID=$(docker run --privileged -idtv $TRAVIS_BUILD_DIR:$TRAVIS_BUILD_DIR -v $HOME/.ccache:$HOME/.ccache -w $TRAVIS_BUILD_DIR --env-file /tmp/env  ubuntu:18.04)

promag commented at 2:46 pm on May 11, 2018:

Remove double space.

ken2812221 commented at 3:03 pm on May 11, 2018:

Done.

promag commented at 2:53 pm on May 11, 2018: member

Concept ACK.

Maybe it should also cache docker layers? Similar to http://circleci.com/docs/1.0/docker/#caching-docker-layers.

Edit: seems to take around 30 to 40 seconds in pulling images and installing extra packages.

MarcoFalke commented at 2:54 pm on May 11, 2018: member

@promag I presume the motivation is that gitian will (soon) use bionic, so having a similar configuration run on travis isn’t that bad. Also, it untangles us a bit from the travis environments and changes, as we use the vanilla bionic docker. So reproducing issues could potentially be easier or harder because it uses docker …

I have no strong opinion based on that, but since the new qt “requires” bionic for windows cross builds, I am slightly in favor of this change.

ken2812221 commented at 2:57 pm on May 11, 2018: contributor

Can you elaborate the motivation behind this change?

We will bump our gitian build system to Ubuntu 18.04 since 0.17, so I think we might need to have the same build environment as the gitian build. Also, once we’ve merged the back-compat code, we could run the executable both on 14.04 and 18.04 to test if the back-compat code would work.

jamesob commented at 2:59 pm on May 11, 2018: member

This will likely fix the build on #12873 by (inadvertently) using a more recent version of SSL that doesn’t cause the deadlock warning system to flare up.

The job duration times don’t seem adversely affected by this change.

Concept ACK

ken2812221 force-pushed on May 11, 2018

MarcoFalke commented at 3:15 pm on May 11, 2018: member

Please remove the [WIP] from the commit title when this is ready for review.

ken2812221 force-pushed on May 11, 2018

in .travis.yml:49 in bea3a72427 outdated

52 install:
53-    - if [ -n "$DPKG_ADD_ARCH" ]; then sudo dpkg --add-architecture "$DPKG_ADD_ARCH" ; fi
54-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get update; fi
55-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get install --no-install-recommends --no-upgrade -qq $PACKAGES; fi
56+    - env | grep -vE '^(LC_.*|LANG)=' > /tmp/env
57+    - DOCKER_ID=$(docker run --privileged -idtv $TRAVIS_BUILD_DIR:$TRAVIS_BUILD_DIR -v $HOME/.ccache:$HOME/.ccache -w $TRAVIS_BUILD_DIR --env-file /tmp/env ubuntu:18.04)

jamesob commented at 3:44 pm on May 11, 2018:

Small thing, but could we add

0- DOCKER_EXEC="docker exec ${DOCKER_ID}"

to cut down on some of the boilerplate below?

ken2812221 commented at 4:04 pm on May 11, 2018:

Sure

ken2812221 force-pushed on May 11, 2018

jamesob commented at 4:38 pm on May 11, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/13215/commits/9e7924c0cb0995f8df6cca5d04b576e5b56038a9

ken2812221 commented at 0:36 am on May 12, 2018: contributor

Maybe it should also cache docker layers? Similar to http://circleci.com/docs/1.0/docker/#caching-docker-layers.

I’ve tested this, docker save took more time than apt install, so we don’t need to cache it.

promag commented at 9:41 am on May 12, 2018: member

@ken2812221 indeed in travis it’s fast.

utACK 9e7924c.

laanwj commented at 1:54 pm on May 14, 2018: member

utACK 9e7924c0cb0995f8df6cca5d04b576e5b56038a9

laanwj requested review from theuni on May 14, 2018

in .travis.yml:48 in 9e7924c0cb outdated

51     - export PATH=$(echo $PATH | tr ':' "\n" | sed '/\/opt\/python/d' | tr "\n" ":" | sed "s|::|:|g")
52 install:
53-    - if [ -n "$DPKG_ADD_ARCH" ]; then sudo dpkg --add-architecture "$DPKG_ADD_ARCH" ; fi
54-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get update; fi
55-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get install --no-install-recommends --no-upgrade -qq $PACKAGES; fi
56+    - env | grep -vE '^(LC_.*|LANG)=' > /tmp/env

MarcoFalke commented at 1:36 am on May 17, 2018:

nit: I’d prefer if we only write the environment variables that are specified in the yaml (global env and job env). Currently this also exports a lot of travis specific junk, which shouldn’t be necessary. No strong opinion, though, as I am not aware how to achieve this.

in .travis.yml:49 in 9e7924c0cb outdated

52 install:
53-    - if [ -n "$DPKG_ADD_ARCH" ]; then sudo dpkg --add-architecture "$DPKG_ADD_ARCH" ; fi
54-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get update; fi
55-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get install --no-install-recommends --no-upgrade -qq $PACKAGES; fi
56+    - env | grep -vE '^(LC_.*|LANG)=' > /tmp/env
57+    - DOCKER_ID=$(docker run --privileged -idtv $TRAVIS_BUILD_DIR:$TRAVIS_BUILD_DIR -v $HOME/.ccache:$HOME/.ccache -w $TRAVIS_BUILD_DIR --env-file /tmp/env ubuntu:18.04)

MarcoFalke commented at 1:40 am on May 17, 2018:

nits:

Please replace the -v with the more verbose --mount type=bind,src=....,dst=...., which is suggested in the docker docs
You overwrite the CCACHE_DIR some lines above, but don’t use that value here. Any reason for that?
Please replace the privliledged flag with just --cap-add SYS_ADMIN and add a comment why this is required or make it specific to the windows build.

in .travis.yml:76 in 9e7924c0cb outdated

88-    - ./configure --cache-file=../config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( cat config.log && false)
89-    - make $MAKEJOBS $GOAL || ( echo "Build failure. Verbose build follows." && make $GOAL V=1 ; false )
90-    - export LD_LIBRARY_PATH=$TRAVIS_BUILD_DIR/depends/$HOST/lib
91-    - if [ "$RUN_TESTS" = "true" ]; then travis_wait 50 make $MAKEJOBS check VERBOSE=1; fi
92+    - $DOCKER_EXEC bash -c "cd build/bitcoin-$HOST && ./configure --cache-file=../config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG" || ( cat config.log && false)
93+    - $DOCKER_EXEC bash -c "cd build/bitcoin-$HOST && make $MAKEJOBS $GOAL" || ( echo "Build failure. Verbose build follows." && make $GOAL V=1 ; false )

MarcoFalke commented at 2:46 am on May 17, 2018:

This fails, since it is executed on the host, not via DOCKER_EXEC.

MarcoFalke commented at 2:47 am on May 17, 2018: member

Tested ACK 9e7924c0cb0995f8df6cca5d04b576e5b56038a9 (after fixing the failure)

Also left a couple of nits in the process, you can safely ignore them.

ken2812221 force-pushed on May 17, 2018

in .travis.yml:48 in eeeb9f2a09 outdated

51     - export PATH=$(echo $PATH | tr ':' "\n" | sed '/\/opt\/python/d' | tr "\n" ":" | sed "s|::|:|g")
52 install:
53-    - if [ -n "$DPKG_ADD_ARCH" ]; then sudo dpkg --add-architecture "$DPKG_ADD_ARCH" ; fi
54-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get update; fi
55-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get install --no-install-recommends --no-upgrade -qq $PACKAGES; fi
56+    - env | grep -E '^(CCACHE_|WINEDEBUG|DISPLAY)' > /tmp/env

MarcoFalke commented at 3:06 am on May 17, 2018:

I assume BOOST_TEST_RANDOM and DISPLAY needs to be passed down as well?

ken2812221 force-pushed on May 17, 2018

MarcoFalke commented at 3:35 am on May 17, 2018: member

re-ACK 09b26751e0

theuni commented at 3:55 am on May 17, 2018: member

Playing around with this now.

in .travis.yml:49 in 09b26751e0 outdated

52 install:
53-    - if [ -n "$DPKG_ADD_ARCH" ]; then sudo dpkg --add-architecture "$DPKG_ADD_ARCH" ; fi
54-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get update; fi
55-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get install --no-install-recommends --no-upgrade -qq $PACKAGES; fi
56+    - env | grep -E '^(CCACHE_|WINEDEBUG|DISPLAY|BOOST_TEST_RANDOM)' | tee /tmp/env
57+    - if [[ $HOST = *-mingw32 ]]; then DOCKER_ADMIN="--cap-add SYS_ADMIN"; fi

theuni commented at 5:46 am on May 17, 2018:

Why is this needed?

ken2812221 commented at 5:53 am on May 17, 2018:

In order to set wine-binfmt

in .travis.yml:52 in 09b26751e0 outdated

55-    - if [ -n "$PACKAGES" ]; then travis_retry sudo apt-get install --no-install-recommends --no-upgrade -qq $PACKAGES; fi
56+    - env | grep -E '^(CCACHE_|WINEDEBUG|DISPLAY|BOOST_TEST_RANDOM)' | tee /tmp/env
57+    - if [[ $HOST = *-mingw32 ]]; then DOCKER_ADMIN="--cap-add SYS_ADMIN"; fi
58+    - DOCKER_ID=$(docker run $DOCKER_ADMIN -idt --mount type=bind,src=$TRAVIS_BUILD_DIR,dst=$TRAVIS_BUILD_DIR --mount type=bind,src=$CCACHE_DIR,dst=$CCACHE_DIR -w $TRAVIS_BUILD_DIR --env-file /tmp/env ubuntu:18.04)
59+    - DOCKER_EXEC="docker exec $DOCKER_ID"
60+    - if [ -n "$DPKG_ADD_ARCH" ]; then $DOCKER_EXEC dpkg --add-architecture "$DPKG_ADD_ARCH" ; fi

theuni commented at 6:02 am on May 17, 2018:

Is this still needed? IIRC it was an artifact of needing a non-default wine.

ken2812221 commented at 6:08 am on May 17, 2018:

Yes, this is needed. wine32 is only available on i386 platform. Here is the link: https://packages.ubuntu.com/bionic/wine32

in .travis.yml:75 in 09b26751e0 outdated

90-    - make $MAKEJOBS $GOAL || ( echo "Build failure. Verbose build follows." && make $GOAL V=1 ; false )
91-    - export LD_LIBRARY_PATH=$TRAVIS_BUILD_DIR/depends/$HOST/lib
92-    - if [ "$RUN_TESTS" = "true" ]; then travis_wait 50 make $MAKEJOBS check VERBOSE=1; fi
93+    - $DOCKER_EXEC bash -c "cd build/bitcoin-$HOST && ./configure --cache-file=../config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG" || ( cat config.log && false)
94+    - $DOCKER_EXEC bash -c "cd build/bitcoin-$HOST && make $MAKEJOBS $GOAL" || ( echo "Build failure. Verbose build follows." && $DOCKER_EXEC bash -c "cd build/bitcoin-$HOST && make $GOAL V=1" ; false )
95+    - if [ "$RUN_TESTS" = "true" ]; then $DOCKER_EXEC bash -c "cd build/bitcoin-$HOST && LD_LIBRARY_PATH=$TRAVIS_BUILD_DIR/depends/$HOST/lib make $MAKEJOBS check VERBOSE=1"; fi

theuni commented at 6:14 am on May 17, 2018:

This dropped the “travis_wait 50”, but I guess we’re ok without it now?

ken2812221 commented at 6:18 am on May 17, 2018:

travis_wait is incompitable with docker. This is a known issue.

sipa commented at 5:06 pm on May 21, 2018:

I think we may not need this travis_wait anymore, due to several improvements to the unit tests’ runtimes.

MarcoFalke commented at 5:19 pm on May 21, 2018:

@sipa is correct. See #12970 (comment) for background. The current run time (on the windows jobs) in this pull:

i686-w64-mingw32: 245.895077045 s x86_64-w64-mingw32: 95.965838938 s

theuni commented at 6:33 am on May 17, 2018: member

Thanks for addressing my questions. utACK. @laanwj I’d like to have a chance to look at the qt/gitian changes as well. Mind giving me until the end of this week to get to them? Ideally we’d get all of those in at once so everything remains in sync.

in .travis.yml:54 in 09b26751e0 outdated

57+    - if [[ $HOST = *-mingw32 ]]; then DOCKER_ADMIN="--cap-add SYS_ADMIN"; fi
58+    - DOCKER_ID=$(docker run $DOCKER_ADMIN -idt --mount type=bind,src=$TRAVIS_BUILD_DIR,dst=$TRAVIS_BUILD_DIR --mount type=bind,src=$CCACHE_DIR,dst=$CCACHE_DIR -w $TRAVIS_BUILD_DIR --env-file /tmp/env ubuntu:18.04)
59+    - DOCKER_EXEC="docker exec $DOCKER_ID"
60+    - if [ -n "$DPKG_ADD_ARCH" ]; then $DOCKER_EXEC dpkg --add-architecture "$DPKG_ADD_ARCH" ; fi
61+    - travis_retry $DOCKER_EXEC apt update
62+    - travis_retry $DOCKER_EXEC apt install --no-install-recommends --no-upgrade -qq -y $PACKAGES $DOCKER_PACKAGES

MarcoFalke commented at 4:33 pm on May 17, 2018:

I incorrectly assumed that apt is an alias for apt-get and asked you to change it. This was wrong, so feel free to change it back to apt-get. Sorry.

ken2812221 commented at 4:44 pm on May 17, 2018:

Never mind

ken2812221 force-pushed on May 17, 2018

ken2812221 commented at 4:44 pm on May 17, 2018: contributor

Update: also expose CONFIG_SHELL into docker

ken2812221 force-pushed on May 17, 2018

jamesob commented at 3:12 pm on May 21, 2018: member

re-utACK https://github.com/bitcoin/bitcoin/pull/13215/commits/67cfb4537ae7ca360f1b0cfb7e625aff9b59d8d7

in .travis.yml:68 in 67cfb4537a outdated

78     - OUTDIR=$BASE_OUTDIR/$TRAVIS_PULL_REQUEST/$TRAVIS_JOB_NUMBER-$HOST
79     - BITCOIN_CONFIG_ALL="--disable-dependency-tracking --prefix=$TRAVIS_BUILD_DIR/depends/$HOST --bindir=$OUTDIR/bin --libdir=$OUTDIR/lib"
80-    - if [ -z "$NO_DEPENDS" ]; then ccache --max-size=$CCACHE_SIZE; fi
81-    - test -n "$USE_SHELL" && eval '"$USE_SHELL" -c "./autogen.sh"' || ./autogen.sh
82+    - if [ -z "$NO_DEPENDS" ]; then $DOCKER_EXEC ccache --max-size=$CCACHE_SIZE; fi
83+    - test -n "$USE_SHELL" && $DOCKER_EXEC "$USE_SHELL" -c "./autogen.sh" || $DOCKER_EXEC ./autogen.sh

MarcoFalke commented at 3:26 pm on May 21, 2018:

nit: Could reuse CONFIG_SHELL as below and skip exporting USE_SHELL or is there a reason to have two separate env vars with the same value?

ken2812221 commented at 3:42 pm on May 21, 2018:

Done

MarcoFalke commented at 3:27 pm on May 21, 2018: member

re-ACK 67cfb4537a

ken2812221 force-pushed on May 21, 2018

jnewbery commented at 4:55 pm on May 21, 2018: member

Concept ACK.

Build 4 here: https://travis-ci.org/bitcoin/bitcoin/jobs/380317076 took 36 minutes, which seems to be much longer than normal. I don’t know whether that’s caused by the changes in this PR or whether it’s just natural variance.

ken2812221 commented at 5:02 pm on May 21, 2018: contributor

@jnewbery Because I just modify the enviroment variable on that one and travis read caches based on enviroment variable. So it needs to recompile everything.

in .travis.yml:72 in 938385a071 outdated

84     - mkdir build && cd build
85-    - ../configure --cache-file=config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( cat config.log && false)
86-    - make distdir VERSION=$HOST
87+    - $DOCKER_EXEC bash -c "cd build && ../configure --cache-file=config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG" || ( cat config.log && false)
88+    - $DOCKER_EXEC bash -c "cd build && make distdir VERSION=$HOST"
89     - cd bitcoin-$HOST

jnewbery commented at 5:35 pm on May 21, 2018:

Can we remove this now?

MarcoFalke commented at 8:04 pm on May 21, 2018:

I think we mirror the pwd for the travis vm and the docker container. Otherwise a cat config.log on the travis vm wouldn’t work, I presume.

MarcoFalke commented at 8:06 pm on May 21, 2018:

Oh yeah, it seems this one can be removed. Though, no harm in leaving it in, I guess.

ken2812221 commented at 1:10 am on May 22, 2018:

No, cat config.log below would not work then.

jnewbery commented at 2:11 pm on May 22, 2018:

ah, makes sense. Thanks

ken2812221 commented at 5:29 am on May 24, 2018: contributor

How about add DOCKER_EXEC () { docker exec $DOCKER_ID bash -c "cd $PWD && $*"; } as a bash funtion, so we don’t need to cd all the time.

ken2812221 force-pushed on May 24, 2018

MarcoFalke commented at 9:46 pm on May 24, 2018: member

re-utACK a182c9d

Travis: Build tests on Ubuntu 18.04 with docker 59e9688eda

in .travis.yml:68 in a182c9db63 outdated

78     - OUTDIR=$BASE_OUTDIR/$TRAVIS_PULL_REQUEST/$TRAVIS_JOB_NUMBER-$HOST
79     - BITCOIN_CONFIG_ALL="--disable-dependency-tracking --prefix=$TRAVIS_BUILD_DIR/depends/$HOST --bindir=$OUTDIR/bin --libdir=$OUTDIR/lib"
80-    - if [ -z "$NO_DEPENDS" ]; then ccache --max-size=$CCACHE_SIZE; fi
81-    - test -n "$USE_SHELL" && eval '"$USE_SHELL" -c "./autogen.sh"' || ./autogen.sh
82+    - if [ -z "$NO_DEPENDS" ]; then DOCKER_EXEC ccache --max-size=$CCACHE_SIZE; fi
83+    - test -n "$CONFIG_SHELL" && DOCKER_EXEC "CONFIG_SHELL" -c "./autogen.sh" || DOCKER_EXEC ./autogen.sh

MarcoFalke commented at 9:48 pm on May 24, 2018:

bash: CONFIG_SHELL: command not found

ken2812221 commented at 3:14 am on May 25, 2018:

Fixed.

ken2812221 force-pushed on May 25, 2018

MarcoFalke commented at 4:51 pm on May 29, 2018: member

re-utACK 59e9688eda4c4b01ee1713625632cd766c1a7ca9

jamesob approved

jamesob commented at 5:11 pm on May 29, 2018: member

re-utACK https://github.com/bitcoin/bitcoin/pull/13215/commits/59e9688eda4c4b01ee1713625632cd766c1a7ca9

jamesob approved

jamesob commented at 5:11 pm on May 29, 2018: member

re-utACK https://github.com/bitcoin/bitcoin/pull/13215/commits/59e9688eda4c4b01ee1713625632cd766c1a7ca9

laanwj merged this on May 29, 2018

laanwj closed this on May 29, 2018

laanwj referenced this in commit f8a29ca823 on May 29, 2018

ken2812221 deleted the branch on May 29, 2018

MarcoFalke locked this on Sep 8, 2021

Travis: Build tests on Ubuntu 18.04 with docker #13215