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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2018-06-06 at 17:24 +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
> 
> This is using the official Ubuntu provided images and installing
> extra build deps required, as we previously did with Travis container
> images.

As mentioned during the review of v1, this bit:

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

doesn't belong to the commit message in my opinion.

[...]
> +    - services:
> +        - docker
>        env:
> +        - IMAGE_NAME=ubuntu:18.04

This can be just IMAGE or even OS.

IMAGE is probably better because it sorts very early, thus
ensuring it will be displayed prominently in the build summary.

> +        - PYTHON=python3

Since we're not actually passing this to configure after all (and
neither could we, due to the fact that $PYTHON needs to be a full
path and we set the variable outside of the container where Python
could be installed in a different location than inside it) it
doesn't really make a lot of sense to have this.

Let's just get rid of it and rely on configure picking up python3
on modern operating systems and python2 otherwise, same way we
handle the issue on CentOS CI.

[...]
> +      before_script:

I thought we agreed on using script instead of before_script here?

> +        - docker run
> +            --privileged
> +            -v $(pwd):/build
> +            -w /build
> +            -e VIR_TEST_DEBUG="$VIR_TEST_DEBUG"
> +            -e PACKAGES="$PACKAGES"
> +            -e DISTCHECK_FLAGS="$DISTCHECK_CONFIGURE_FLAGS"

We should keep the names of variables consistent between the host
and container environment.

> +            $IMAGE_NAME

$IMAGE_NAME (or rather $IMAGE) should be quoted.

> +            /bin/sh -c "$DOCKER_CMD"

Something I didn't spot the first time around: using

  /bin/sh -xc "$DOCKER_CMD"

here will be very helpful to figure out exactly what is happening
during the build, so please add that.

[...]
> +    - services:
> +        - docker
> +      env:
> +        - IMAGE_NAME=ubuntu:16.04
> +        - PYTHON=python2
> +        - DISTCHECK_CONFIGURE_FLAGS="--with-init-script=upstart"
> +      before_script:
> +        - docker pull berrange/test

You forgot to delete this one :)

[...]
>          - brew update
>          - brew upgrade
>          - brew install python ccache rpcgen yajl
> +      before_script:
> +        - ./autogen.sh --prefix=$(pwd)/install-root

Another bit I missed the first time around (sorry): before these
changes, we took advantage of the before_script/script split to
avoid typing out the autogen.sh call twice, but that's no longer
relevant now, so you can simplify things by folding it into script.

[...]
>  env:
>    global:
>      - VIR_TEST_DEBUG=1
> +    - DOCKER_CMD="
> +        apt-get update &&
> +        apt-get install -y \$PACKAGES &&
> +        ./autogen.sh --prefix=$(pwd)/install-root &&

You didn't escape '$(pwd)' correctly here, so it will be expanded
in the host rather than inside the container.

That said, we don't actually call 'make install' for builds
happening inside Docker since 'make distcheck' works there, so you
might as well drop --prefix altogether.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux