[libvirt] [PATCH] travis: switch to using Ubuntu 16.04 and 18.04

[Andrea Bolognani]

On Tue, 2018-06-05 at 18:47 +0100, Daniel P. Berrangé wrote:
> The container images provided by Travis only support Ubuntu 14.04,
> however, Travis has ability to run docker, which allows the build
> script to use arbitrary OS images. This takes advantage of that to
> convert the build over to Ubuntu 16.04 and 18.04

Neat! I was not aware that was possible. TIL :)

> This is using the official Ubuntu provided images and installing
> extra build deps required, as we previously did with Travis container
> images.

Everything after this doesn't belong in the commit message IMHO.

> With the switch to Docker though, this can be improved, by
> building custom Docker images with all the deps pre-installed which
> will cut down build time. This can be driven from the package lists
> in libvirt-jenkins-ci repo, to remove the duplication. This work
> for future improvement though, this just does the minimal conversion
> to match what we already do, but with newer distro.

We will probably want to do that, since installing packages takes
quite a bit of time and using Docker like this apparently causes
jobs to serialize, which makes shaving down time all the more
important.

I'm wondering if using Ubuntu 16.04 and 18.04 as base image is the
best option, however. Pavel is trying to get more hardware assigned
to us in the CentOS CI environment, and assuming that's successful
we will get those distributions added there as well for full-stack
testing; Travis CI is more useful for developers to smoke test
their patch series before posting, however, and with that in mind
I think it would be much better to run the build on the latest
CentOS, Fedora, Debian and Ubuntu to give reasonable coverage,
leaving up to CentOS CI to catch issues related to interactions
with other components or compatibility with older distros after
merge.

[...]
> +      before_install:

Not sure 'before_install' is the best choice here, I would probably
have gone with 'script' instead.

On the other hand, it seems like the distinction between job phases
is fairly blurry, so it probably doesn't matter after all.

> +        - docker pull berrange/test

I'm pretty sure we don't want this :)

> +        - docker run
> +            --privileged

Is '--privileged' really needed here? I'm not familiar enough
with Docker, but it feels like we shouldn't need it. Feel free to
point out exactly why I'm wrong :)

> +            -v `pwd`:/build

Please use $(pwd) instead of `pwd`.

> +            -w /build
> +            -e VIR_TEST_DEBUG="$VIR_TEST_DEBUG"
> +            -e PACKAGES="$PACKAGES"
> +            -e DISTCHECK_FLAGS="$DISTCHECK_FLAGS"
> +            ubuntu:18.04

It would be great to have the base image name in the environment
as well, so that in case the build fails on only one of the
Ubuntu versions you could easily tell from a quick look at the
build summary which one that is.

[...]
>        env:
> -        - PYTHON=$(which python3)

You got rid of this variable entirely. I think we won't lose any
coverage because configure will naturally pick python2 on 16.04
and python3 on 18.04 (I can't check because Travis CI decided to
only keep around a truncated log of my test run), but it was kinda
nice to have that explicitly in the summary. As long as you can
confirm the above is correct and no coverage is lost, though, I'm
okay with the removal.

> -        - DISTCHECK_CONFIGURE_FLAGS="--with-init-script=systemd"
> +        - DISTCHECK_FLAGS="--with-init-script=upstart"

You copied the wrong line here, we're going to test upstart twice.

I don't see a good reason to change the variable name, however:
let's just stick with matching the name expected by distcheck
itself.

[...]
>      - compiler: clang
> +      language: c
>        os: osx

You lost the ccache setting. That's probably okay because we
install the package and set up PATH explicitly anyway.

[...]
> +      after_failure:
> +        - echo '=== LOG FILE(S) START ==='
> +        - find -name test-suite.log | xargs cat
> +        - echo '=== LOG FILE(S) END ==='

This is simpler than the previous approach. I like it :)

[...]
> +    - DOCKER_CMD="
> +        apt-get update &&
> +        apt-get install -y \$PACKAGES &&
> +        ./autogen.sh --prefix=/build/install-root &&

Use

  --prefix=\$(pwd)/install-root

here for consistency with the macOS part.

> +        make -j 3 &&

No space between '-j' and '3'.

> +        make -j 3 syntax-check &&
> +        make -j 3 distcheck DISTCHECK_CONFIGURE_FLAGS=\$DISTCHECK_FLAGS ||

This won't work if we have more than a single flag, will it?

> +        (
> +          echo '=== LOG FILE(S) START ==='
> +          find -name test-suite.log | xargs cat
> +          echo '=== LOG FILE(S) END ==='
> +          exit 1
> +        )
> +      "
> +    - PACKAGES="

You lost the comment about this list being sorted alphabetically.

> +        augeas-tools
[...]
> +        zfs-fuse"

Put the closing quote on a separate line for neater future diffs.

-- 
Andrea Bolognani / Red Hat / Virtualization