[libvirt] [PATCH v3 0/6] Facilitate running libvirt builds via docker containers
For a while QEMU has provided simple make rules for building QEMU inside standard docker container environments. This provides an equivalent mechanism for libvirt inspired by QEMU's. QEMU actually builds the container images on developer's machines locally. Libvirt already hosts pre-built images on quay.io, so that is used directly as it is quicker to download them than to build them locally. This also ensures the container contents match what the live CI system is using, as opposed to building an image with newer packages. NB, there is a currently a test failure with this change to travis that we don't currently see. Current travis runs a privileged docker container, but with this change we now run unprivileged containers. This shows that the "virsh-snapshot" test is trying to create a file under $HOME, and docker blocks this. Obviously needs fixing before pushing. Changed in v3: - Lots of new variables & variable renames - Rename to top level Makefile.ci - Rename make targets to have @ separator for the image - Refactor macOS travis config Changed in v2: - Drop cross-compilation patches temporarily. The cross-compiler images are not yet built on quay.io and rather than creating and bulding many more images manually, I'm working on script to automate setup of quay.io images - Many changes to the make rules to make it possible to use them from Travis CI to match its currently testing setup - Modify Travis CI config to use the new make rules for consistency Daniel P. Berrangé (6): tests: don't abort in fopen(/proc/mounts) tests: add targets for building libvirt inside docker containers travis: convert ubuntu, centos & mingw builds to use new make rules travis: use declarative syntax for homebrew packages travis: remove display of test-suite.log from macOS travis: put macOS script inline in the macOS matrix entry .gitignore | 1 + .travis.yml | 78 +++++----------- Makefile.am | 2 + Makefile.ci | 206 ++++++++++++++++++++++++++++++++++++++++++ tests/vircgroupmock.c | 6 +- 5 files changed, 235 insertions(+), 58 deletions(-) create mode 100644 Makefile.ci -- 2.20.1
The mock fopen() function will abort if "/proc/mounts" is requested with "r" permissions and VIR_CGROUP_MOCK_FILENAME env var is not set. Unfortunately this is triggering by the libselinux library constructor when it tries to read /proc/mounts to find out if selinuxfs is mounted in an unusual place. This, however, only affects libselinux in Debian as that opens with "r", while in Fedora / RHEL it opens "re" and thus luckily never triggered the abort(), instead getting an EACCESS. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/vircgroupmock.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c index 06bd0a5f29..9c67a44b0d 100644 --- a/tests/vircgroupmock.c +++ b/tests/vircgroupmock.c @@ -460,8 +460,10 @@ FILE *fopen(const char *path, const char *mode) } if (type) { - if (!filename) - abort(); + if (!filename) { + errno = EACCES; + return NULL; + } if (virAsprintfQuiet(&filepath, "%s/vircgroupdata/%s.%s", abs_srcdir, filename, type) < 0) { abort(); -- 2.20.1
The Travis CI system uses docker containers for its build environment. These are pre-built and hosted under quay.io/libvirt so that developers can use them for reproducing problems locally. Getting the right docker command syntax to use them, however, is not entirely easy. This patch addresses that usability issue by introducing some make targets. To run a simple build (aka 'make all') using the Fedora 28 container: make ci-build@fedora-28 To also run unit tests make ci-check@fedora-28 This is just syntax sugar for calling the previous command with a custom make target make ci-build@fedora-28 MAKE_ARGS="check" To do a purely interactive build it is possible to request a shell make ci-shell@fedora-28 To do a mingw build, it is currently possible to use the fedora-rawhide and request a different configure script make ci-build@fedora-rawhide CONFIGURE=mingw32-configure It is also possible to do cross compiled builds via the Debian containers make ci-build@debian-9-cross-s390x In all cases the GIT source tree is cloned locally into a 'ci-tree/src' sub-directory which is then exposed to the container at '/src'. It is setup to facilitate VPATH build so the initial working directory is '/src/build'. An in source tree build can be requested instead by passing an empty string VPATH= arg to make. The make rules are kept in a standalone file that is included into the main Makefile.am, so that it is possible to run them without having to invoke autotools first. It is neccessary to disable the gnulib submodule commit check because this fails due to the way we have manually cloned submodule repos as primary git repos with their own .git directory, instead of letting git treat them as submodules in the top level .git directory. make[1]: Entering directory '/src/build' fatal: Not a valid object name origin fatal: run_command returned non-zero status for .gnulib . maint.mk: found non-public submodule commit make: *** [/src/maint.mk:1448: public-submodule-commit] Error 1 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .gitignore | 1 + Makefile.am | 2 + Makefile.ci | 206 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 209 insertions(+) create mode 100644 Makefile.ci diff --git a/.gitignore b/.gitignore index 3223cf1b76..a67a842294 100644 --- a/.gitignore +++ b/.gitignore @@ -45,6 +45,7 @@ /autom4te.cache /build-aux/* /build/ +/ci-tree/ /confdefs.h /config.cache /config.guess diff --git a/Makefile.am b/Makefile.am index 3c06e2619a..8add2f5d61 100644 --- a/Makefile.am +++ b/Makefile.am @@ -109,3 +109,5 @@ gen-AUTHORS: mv -f $(distdir)/AUTHORS-tmp $(distdir)/AUTHORS && \ rm -f all.list maint.list contrib.list; \ fi + +include Makefile.ci diff --git a/Makefile.ci b/Makefile.ci new file mode 100644 index 0000000000..5448fedd97 --- /dev/null +++ b/Makefile.ci @@ -0,0 +1,206 @@ +# -*- makefile -*- +# vim: filetype=make + +HERE = $(shell pwd) + +# Figure out name and path to this file. This isn't +# portable but we only care for modern GNU make +THIS_FILE = $(abspath $(lastword $(MAKEFILE_LIST))) + +# The directory holding content on the host that we will +# expose to the container. +SCRATCHDIR = $(HERE)/ci-tree + +TOP = $(shell git rev-parse --show-toplevel) + +# The directory holding the clo%%%%ne of the git repo that +# we will expose to the container +HOST_SRCDIR = $(SCRATCHDIR)/src + +# The directory holding the source inside the +# container. ie where we told docker to expose +# the $GIT/ci-tree directory from the host +CONT_SRCDIR = /src + +# Relative directory to perform the build in. This +# defaults to using a separate build dir, but can be +# set to empty string for an in-source tree build. +CONT_VPATH = build + +# The directory holding the build output inside the +# container. +CONT_BUILDDIR = $(CONT_SRCDIR)/$(CONT_VPATH) + +# Can be overridden with mingw{32,64}-configure if desired +CONFIGURE = $(CONT_SRCDIR)/configure + +# Default to using all possible CPUs +SMP = $(shell getconf _NPROCESSORS_ONLN) + +# Any extra arguments to pass to make +MAKE_ARGS = + +# Any extra arguments to pass to configure +CONFIGURE_ARGS = + +# Avoid pulling submodules over the network by locally +# cloning them +SUBMODULES = .gnulib src/keycodemapdb + +IMAGE_PREFIX = quay.io/libvirt/buildenv- +IMAGE_TAG = :master + +# We delete the virtual root after completion, set +# to 0 if you need to keep it around for debugging +CLEAN = 1 + +# We'll always freshly clone the virtual root each +# time in case it was not cleaned up before. Set +# to 1 if you want to try restarting a previously +# preserved env +REUSE = 0 + +# We need the container process to run with current host IDs +# so that it can access the passed in build directory +UID = $(shell id -u) +GID = $(shell id -g) + +# Docker doesn't require the IDs you run as to exist in +# the container's /etc/passwd & /etc/group files, but +# if they do not, then libvirt's 'make check' will fail +# many tests. +# +# We do not directly mount /etc/{passwd,group} as Docker +# is liable to mess with SELinux labelling which will +# then prevent the host accessing them. Copying them +# first is safer. +PWDB_MOUNTS = \ + --volume $(SCRATCHDIR)/group:/etc/group:ro,z \ + --volume $(SCRATCHDIR)/passwd:/etc/passwd:ro,z \ + $(NULL) + +# Docker containers can have very large ulimits +# for nofiles - as much as 1048576. This makes +# libvirt very slow at exec'ing programs. +ULIMIT_FILES = 1024 + +# Args to use when cloning a git repo. +# -c stop it complaining about checking out a random hash +# -q stop it displaying progress info for local clone +# --local ensure we don't actually copy files +GIT_ARGS = \ + -c advice.detachedHead=false \ + -q \ + --local \ + $(NULL) + +# Args to use when running the docker env +# --rm stop inactive containers getting left behind +# --user we execute as the same user & group account +# as dev so that file ownership matches host +# instead of root:root +# --volume to pass in the cloned git repo & config +# --workdir to set cwd to vpath build location +# --ulimit lower files limit for performance reasons +# --interactive +# --tty Ensure we have ability to Ctrl-C the build +DOCKER_ARGS = \ + --rm \ + --user $(UID):$(GID) \ + --interactive \ + --tty \ + $(PWDB_MOUNTS) \ + --volume $(HOST_SRCDIR):$(CONT_SRCDIR):z \ + --workdir $(CONT_SRCDIR) \ + --ulimit nofile=$(ULIMIT_FILES):$(ULIMIT_FILES) \ + $(NULL) + +check-docker: + @echo -n "Checking if Docker is available and running..." && \ + docker version 1>/dev/null && echo "yes" + +prepare-tree: + @if test "$(REUSE)" != "1" ; then \ + rm -rf ci-tree ; \ + fi + @if ! test -d ci-tree ; then \ + mkdir -p ci-tree/src; \ + cp /etc/passwd ci-tree; \ + cp /etc/group ci-tree; \ + echo "Cloning $(TOP) to $(HOST_SRCDIR)"; \ + git clone $(GIT_ARGS) $(TOP) $(HOST_SRCDIR) || exit 1; \ + for mod in $(SUBMODULES) ; \ + do \ + if test -d $(TOP)/$$mod ; \ + then \ + echo "Cloning $(TOP)/$$mod to $(HOST_SRCDIR)/$$mod"; \ + git clone $(GIT_ARGS) $(TOP)/$$mod $(HOST_SRCDIR)/$$mod || exit 1; \ + fi ; \ + done ; \ + else \ + test "$(CLEAN)" = "1" && rm -rf ci-tree || : ; \ + fi + +# $CONFIGURE_OPTS is a env that can optionally be set in the container, +# populated at build time from the Dockerfile. A typical use case would +# be to pass --host/--target args to trigger cross-compilation +# +# This can be augmented by make local args in $(CONFIGURE_ARGS) +ci-build@%: check-docker prepare-tree + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)$*$(IMAGE_TAG) \ + /bin/bash -c '\ + mkdir -p $(CONT_BUILDDIR) || exit 1 ; \ + cd $(CONT_BUILDDIR) ; \ + NOCONFIGURE=1 $(CONT_SRCDIR)/autogen.sh || exit 1 ; \ + $(CONFIGURE) $${CONFIGURE_OPTS} $(CONFIGURE_ARGS) ; \ + if test $$? != 0 ; \ + then \ + test -f config.log && cat config.log ; \ + exit 1 ; \ + fi; \ + find -name test-suite.log -delete ; \ + make -j$(SMP) $(MAKE_ARGS) ; \ + if test $$? != 0 ; then \ + LOGS=`find -name test-suite.log` ; \ + if test "$${LOGS}" != "" ; then \ + echo "=== LOG FILE(S) START ===" ; \ + cat $${LOGS} ; \ + echo "=== LOG FILE(S) END ===" ; \ + fi ; \ + exit 1 ;\ + fi' + @test "$(CLEAN)" = "1" && rm -rf ci-tree || : + +ci-check@%: + $(MAKE) -f $(THIS_FILE) ci-build@$* MAKE_ARGS="check gl_public_submodule_commit=" + +ci-shell@%: prepare-tree + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)$*$(IMAGE_TAG) /bin/bash + @test "$(CLEAN)" = "1" && rm -rf ci-tree || : + +ci-help: + @echo "Build libvirt inside docker containers used for CI" + @echo + @echo "Available targets:" + @echo + @echo " ci-build@\$$IMAGE - run a default 'make'" + @echo " ci-check@\$$IMAGE - run a 'make check'" + @echo " ci-shell@\$$IMAGE - run an interactive shell" + @echo + @echo "Available x86 container images:" + @echo + @echo " centos-7" + @echo " debian-8" + @echo " debian-9" + @echo " debian-sid" + @echo " fedora-28" + @echo " fedora-29" + @echo " fedora-rawhide" + @echo " ubuntu-16" + @echo " ubuntu-18" + @echo + @echo "Available make variables:" + @echo + @echo " CLEAN=0 - do not delete '$(SCRATCHDIR)' after completion" + @echo " REUSE=1 - re-use existing '$(SCRATCHDIR)' content" + @echo -- 2.20.1
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
The Travis CI system uses docker containers for its build environment. These are pre-built and hosted under quay.io/libvirt so that developers can use them for reproducing problems locally.
Throughout the commit message, s/docker/Docker/g [...]
To do a mingw build, it is currently possible to use the fedora-rawhide and request a different configure script
s/mingw/MinGW/ s/and/image and/ [...]
In all cases the GIT source tree is cloned locally into a 'ci-tree/src' sub-directory which is then exposed to the container at '/src'. It is setup to facilitate VPATH build so the initial working directory is '/src/build'. An in source tree build can be requested instead by passing an empty string VPATH= arg to make.
The part about the initial working directory being the VPATH build is no longer accurate. I also don't see a lot of value in spelling out the paths, since they are mostly an implementation detail: I would just write something like By default, a VPATH build will be performed; to request an in-tree build instead, pass VPATH= to make. In all cases, the git source tree is cloned locally to avoid unnecessary network access.
The make rules are kept in a standalone file that is included into the main Makefile.am, so that it is possible to run them without having to invoke autotools first.
It is neccessary to disable the gnulib submodule commit check because this fails due to the way we have manually cloned submodule repos as primary git repos with their own .git directory, instead of letting git treat them as submodules in the top level .git directory.
make[1]: Entering directory '/src/build' fatal: Not a valid object name origin fatal: run_command returned non-zero status for .gnulib . maint.mk: found non-public submodule commit make: *** [/src/maint.mk:1448: public-submodule-commit] Error 1
This last part is interesting for people looking at the code but not for users, so I'd leave it out of the commit message. [...]
+HERE = $(shell pwd) + +# Figure out name and path to this file. This isn't +# portable but we only care for modern GNU make +THIS_FILE = $(abspath $(lastword $(MAKEFILE_LIST))) + +# The directory holding content on the host that we will +# expose to the container. +SCRATCHDIR = $(HERE)/ci-tree
Since you don't use $(HERE) anywhere else, you could do SCRATCHDIR = $(shell pwd)/ci-tree here. [...]
+# The directory holding the clo%%%%ne of the git repo that
Not sure what happened with that comment ;) [...]
+# The directory holding the source inside the +# container. ie where we told docker to expose
Again for all messages displayed to the user and comments, s/docker/Docker/g
+# the $GIT/ci-tree directory from the host
This is actually $(HOST_SRCDIR). [...]
+# Relative directory to perform the build in. This +# defaults to using a separate build dir, but can be +# set to empty string for an in-source tree build. +CONT_VPATH = build
This should be called VPATH according to the commit message... VPATH also seems like a better name to expose to users, so I'd say change the code to follow the documentation rather than the other way around. [...]
+# Avoid pulling submodules over the network by locally +# cloning them +SUBMODULES = .gnulib src/keycodemapdb
I still very much don't like having this hardcoded instead of figured out at runtime by parsing the output of 'git submodule', but we can take care of that in a follow-up patch. [...]
+prepare-tree: + @if test "$(REUSE)" != "1" ; then \ + rm -rf ci-tree ; \ + fi + @if ! test -d ci-tree ; then \ + mkdir -p ci-tree/src; \ + cp /etc/passwd ci-tree; \ + cp /etc/group ci-tree; \
All instances of 'ci-tree' here, a few lines down, and also in the ci-build@% and ci-shell@% targets, should be changed to $(SCRATCHDIR). [...]
+ for mod in $(SUBMODULES) ; \ + do \ + if test -d $(TOP)/$$mod ; \ + then \ + echo "Cloning $(TOP)/$$mod to $(HOST_SRCDIR)/$$mod"; \ + git clone $(GIT_ARGS) $(TOP)/$$mod $(HOST_SRCDIR)/$$mod || exit 1; \ + fi ; \ + done ; \
You can rewrite this loop as for mod in $(SUBMODULES); \ do \ test -d $(TOP)/$$mod || continue; \ echo ... \ git clone ... \ done to reduce indentation. But I'm actually not sure what the check is there for: the list of submodules *will be* correct, especially once we move away from hardcoding it; even on a fresh clone with uninitialized submodules, which is a situation we need to handle better, the check will not help: $ make -f Makefile.ci ci-build@debian-9 Checking if Docker is available and running...Cloning /home/test/libvirt to /home/test/libvirt/ci-tree/src yes Cloning /home/test/libvirt/.gnulib to /home/test/libvirt/ci-tree/src/.gnulib fatal: repository '/home/test/libvirt/.gnulib' does not exist make: *** [Makefile.ci:124: prepare-tree] Error 1 So I'm thinking what we need to do instead is for mod in $(SUBMODULES); \ do \ git submodule update --init $(TOP)/$$mod || exit 1; \ echo ... \ git clone ... \ done which will do what we need and work on fresh clones too.
+ else \ + test "$(CLEAN)" = "1" && rm -rf ci-tree || : ; \
This branch seems incorrect: $(CLONE), as documented above, is supposed to control whether or not to remove $(SCRATCHDIR) *after* a build, but here we're removing it *before* the build, which also means... How would the container even run? And indeed: $ make -f Makefile.ci ci-shell@debian-9 CLEAN=0 Cloning /home/test/libvirt to /home/test/libvirt/ci-tree/src Cloning /home/test/libvirt/.gnulib to /home/test/libvirt/ci-tree/src/.gnulib Cloning /home/test/libvirt/src/keycodemapdb to /home/test/libvirt/ci-tree/src/src/keycodemapdb docker run --rm --user 1000:1000 --interactive --tty --volume /home/test/libvirt/ci-tree/group:/etc/group:ro,z --volume /home/test/libvirt/ci-tree/passwd:/etc/passwd:ro,z --volume /home/test/libvirt/ci-tree/src:/src:z --workdir /src --ulimit nofile=1024:1024 quay.io/libvirt/buildenv-debian-9:master /bin/bash test@14f00bd5a24a:/src$ exit $ make -f Makefile.ci ci-shell@debian-9 REUSE=1 docker run --rm --user 1000:1000 --interactive --tty --volume /home/test/libvirt/ci-tree/group:/etc/group:ro,z --volume /home/test/libvirt/ci-tree/passwd:/etc/passwd:ro,z --volume /home/test/libvirt/ci-tree/src:/src:z --workdir /src --ulimit nofile=1024:1024 quay.io/libvirt/buildenv-debian-9:master /bin/bash /usr/bin/docker-current: Error response from daemon: oci runtime error: container_linux.go:247: starting container process caused "process_linux.go:364: container init caused \"rootfs_linux.go:54: mounting \\\"/home/test/libvirt/ci-tree/group\\\" to rootfs \\\"/var/lib/docker/overlay2/388b4560961db1d446ee8621903ff33d29e51e30de3a9a661448ebdac0fd9d39/merged\\\" at \\\"/var/lib/docker/overlay2/388b4560961db1d446ee8621903ff33d29e51e30de3a9a661448ebdac0fd9d39/merged/etc/group\\\" caused \\\"not a directory\\\"\"" : Are you trying to mount a directory onto a file (or vice-versa)? Check if the specified host path exists and is the expected type. make: *** [Makefile.ci:176: ci-shell@debian-9] Error 125 Removing the else branch entirely makes this work. [...]
+ci-build@%: check-docker prepare-tree + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)$*$(IMAGE_TAG) \ + /bin/bash -c '\
There's nothing bash-specific in this script AFAICT, so you can use /bin/sh instead.
+ mkdir -p $(CONT_BUILDDIR) || exit 1 ; \ + cd $(CONT_BUILDDIR) ; \
This should also have || exit 1 just in case. [...]
+ find -name test-suite.log -delete ; \
I was today years old when I realized you can call find without giving it a directory to search in, in which case it will start from the current directory :)
+ make -j$(SMP) $(MAKE_ARGS) ; \
You should change this to make -j$(SMP) gl_public_submodule_commit= $(MAKE_ARGS) because the way it works right now, with the extra argument for gnulib being passed in the ci-check@% target, means something as reasonable as $ make ci-build@debian-9 MAKE_ARGS='check syntax-check' does not work. People running their own builds from a ci-shell@ will still have to add the argument themselves if they care about running the test suite, but there's really not much we can do there. [...]
+ci-shell@%: prepare-tree
This should depend on check-docker too. [...]
+ @echo "Available x86 container images:" + @echo + @echo " centos-7" + @echo " debian-8"
Not anymore!
+ @echo " debian-9" + @echo " debian-sid" + @echo " fedora-28" + @echo " fedora-29" + @echo " fedora-rawhide" + @echo " ubuntu-16"
Not anymore! We should figure out a way to fetch this dynamically. We can do that later, though. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 29, 2019 at 05:39:59PM +0100, Andrea Bolognani wrote:
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
The Travis CI system uses docker containers for its build environment. These are pre-built and hosted under quay.io/libvirt so that developers can use them for reproducing problems locally.
Throughout the commit message,
s/docker/Docker/g
[...]
To do a mingw build, it is currently possible to use the fedora-rawhide and request a different configure script
s/mingw/MinGW/ s/and/image and/
[...]
In all cases the GIT source tree is cloned locally into a 'ci-tree/src' sub-directory which is then exposed to the container at '/src'. It is setup to facilitate VPATH build so the initial working directory is '/src/build'. An in source tree build can be requested instead by passing an empty string VPATH= arg to make.
The part about the initial working directory being the VPATH build is no longer accurate. I also don't see a lot of value in spelling out the paths, since they are mostly an implementation detail: I would just write something like
By default, a VPATH build will be performed; to request an in-tree build instead, pass VPATH= to make. In all cases, the git source tree is cloned locally to avoid unnecessary network access.
The make rules are kept in a standalone file that is included into the main Makefile.am, so that it is possible to run them without having to invoke autotools first.
It is neccessary to disable the gnulib submodule commit check because this fails due to the way we have manually cloned submodule repos as primary git repos with their own .git directory, instead of letting git treat them as submodules in the top level .git directory.
make[1]: Entering directory '/src/build' fatal: Not a valid object name origin fatal: run_command returned non-zero status for .gnulib . maint.mk: found non-public submodule commit make: *** [/src/maint.mk:1448: public-submodule-commit] Error 1
This last part is interesting for people looking at the code but not for users, so I'd leave it out of the commit message.
It is important as it shows the maint.mk rule that is causing the problem we are fixing. Without that there's not enough context to undestand the problem.
+# Relative directory to perform the build in. This +# defaults to using a separate build dir, but can be +# set to empty string for an in-source tree build. +CONT_VPATH = build
This should be called VPATH according to the commit message...
VPATH also seems like a better name to expose to users, so I'd say change the code to follow the documentation rather than the other way around.
I did try using VPATH before posting this, but I hit an irritating problem. Automake already has a variable called "VPATH", so when we do 'include Makefile.ci' we break stuff :-( In fact this could potentially be a problem for other variables if we are not lucky. Two options I see - Prefix all variables in this file with "CI_" - Don't inmclude Makefile.ci from Makefile.am I'm tending towards the former, since it is still less verbose than adding '-f Makefile.ci' every time.
+ for mod in $(SUBMODULES) ; \ + do \ + if test -d $(TOP)/$$mod ; \ + then \ + echo "Cloning $(TOP)/$$mod to $(HOST_SRCDIR)/$$mod"; \ + git clone $(GIT_ARGS) $(TOP)/$$mod $(HOST_SRCDIR)/$$mod || exit 1; \ + fi ; \ + done ; \
You can rewrite this loop as
for mod in $(SUBMODULES); \ do \ test -d $(TOP)/$$mod || continue; \ echo ... \ git clone ... \ done
to reduce indentation.
But I'm actually not sure what the check is there for: the list of submodules *will be* correct, especially once we move away from hardcoding it; even on a fresh clone with uninitialized submodules, which is a situation we need to handle better, the check will not help:
It was supposed to test existance of $(TOP)/$$mod/.git actually. ie, we should only clone the submodule if it has actually been initialized. If it doesn't exist, we should just let the container bootstrap clone it as normal.
$ make -f Makefile.ci ci-build@debian-9 Checking if Docker is available and running...Cloning /home/test/libvirt to /home/test/libvirt/ci-tree/src yes Cloning /home/test/libvirt/.gnulib to /home/test/libvirt/ci-tree/src/.gnulib fatal: repository '/home/test/libvirt/.gnulib' does not exist make: *** [Makefile.ci:124: prepare-tree] Error 1
So I'm thinking what we need to do instead is
for mod in $(SUBMODULES); \ do \ git submodule update --init $(TOP)/$$mod || exit 1; \
IMHO, build rules must never touch the user's GIT repository state, so I don't want to run a submodule update.
echo ... \ git clone ... \ done
which will do what we need and work on fresh clones too.
+ else \ + test "$(CLEAN)" = "1" && rm -rf ci-tree || : ; \
This branch seems incorrect: $(CLONE), as documented above, is supposed to control whether or not to remove $(SCRATCHDIR) *after* a build, but here we're removing it *before* the build, which also means... How would the container even run? And indeed:
Yeah, left over cruft from earlier version
[...]
+ci-build@%: check-docker prepare-tree + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)$*$(IMAGE_TAG) \ + /bin/bash -c '\
There's nothing bash-specific in this script AFAICT, so you can use /bin/sh instead.
Since we have bash available I prefer to use that explicitly, so we don't have to worry about accidentally using some syntax that will break with dash in future.
+ mkdir -p $(CONT_BUILDDIR) || exit 1 ; \ + cd $(CONT_BUILDDIR) ; \
This should also have
|| exit 1
just in case.
That's impossible to reach, since we exit the line above if mkdir fails.
+ make -j$(SMP) $(MAKE_ARGS) ; \
You should change this to
make -j$(SMP) gl_public_submodule_commit= $(MAKE_ARGS)
because the way it works right now, with the extra argument for gnulib being passed in the ci-check@% target, means something as reasonable as
$ make ci-build@debian-9 MAKE_ARGS='check syntax-check'
does not work.
People running their own builds from a ci-shell@ will still have to add the argument themselves if they care about running the test suite, but there's really not much we can do there.
Oh, yes, that's nice Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, 2019-04-01 at 11:39 +0100, Daniel P. Berrangé wrote:
On Fri, Mar 29, 2019 at 05:39:59PM +0100, Andrea Bolognani wrote:
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
It is neccessary to disable the gnulib submodule commit check because this fails due to the way we have manually cloned submodule repos as primary git repos with their own .git directory, instead of letting git treat them as submodules in the top level .git directory.
make[1]: Entering directory '/src/build' fatal: Not a valid object name origin fatal: run_command returned non-zero status for .gnulib . maint.mk: found non-public submodule commit make: *** [/src/maint.mk:1448: public-submodule-commit] Error 1
This last part is interesting for people looking at the code but not for users, so I'd leave it out of the commit message.
It is important as it shows the maint.mk rule that is causing the problem we are fixing. Without that there's not enough context to undestand the problem.
My point is that this is really only interesting to people hacking on Makefile.ci, and it's just noise to all other developers going through the git log. So we should not drop the information, just move it to a comment inside Makefile.ci, possibly with the shell output snipped for brevity.
+# Relative directory to perform the build in. This +# defaults to using a separate build dir, but can be +# set to empty string for an in-source tree build. +CONT_VPATH = build
This should be called VPATH according to the commit message...
VPATH also seems like a better name to expose to users, so I'd say change the code to follow the documentation rather than the other way around.
I did try using VPATH before posting this, but I hit an irritating problem. Automake already has a variable called "VPATH", so when we do 'include Makefile.ci' we break stuff :-(
In fact this could potentially be a problem for other variables if we are not lucky.
Two options I see
- Prefix all variables in this file with "CI_" - Don't inmclude Makefile.ci from Makefile.am
I'm tending towards the former, since it is still less verbose than adding '-f Makefile.ci' every time.
Ouch. Yeah, the first option seems more palatable. [...]
But I'm actually not sure what the check is there for: the list of submodules *will be* correct, especially once we move away from hardcoding it; even on a fresh clone with uninitialized submodules, which is a situation we need to handle better, the check will not help:
It was supposed to test existance of $(TOP)/$$mod/.git actually. ie, we should only clone the submodule if it has actually been initialized. If it doesn't exist, we should just let the container bootstrap clone it as normal.
So I'm thinking what we need to do instead is
for mod in $(SUBMODULES); \ do \ git submodule update --init $(TOP)/$$mod || exit 1; \
IMHO, build rules must never touch the user's GIT repository state, so I don't want to run a submodule update.
Okay, this makes sense. Just make sure you test with '-f' rather than with '-d', because if the submodules have been initialized with 'git submodule update --init' then .git will be a regular file rather than a directory. And you can still use the shorter version I suggested :)
[...]
+ci-build@%: check-docker prepare-tree + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)$*$(IMAGE_TAG) \ + /bin/bash -c '\
There's nothing bash-specific in this script AFAICT, so you can use /bin/sh instead.
Since we have bash available I prefer to use that explicitly, so we don't have to worry about accidentally using some syntax that will break with dash in future.
Alright. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Apr 01, 2019 at 01:03:21PM +0200, Andrea Bolognani wrote:
On Mon, 2019-04-01 at 11:39 +0100, Daniel P. Berrangé wrote:
On Fri, Mar 29, 2019 at 05:39:59PM +0100, Andrea Bolognani wrote:
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
It is neccessary to disable the gnulib submodule commit check because this fails due to the way we have manually cloned submodule repos as primary git repos with their own .git directory, instead of letting git treat them as submodules in the top level .git directory.
make[1]: Entering directory '/src/build' fatal: Not a valid object name origin fatal: run_command returned non-zero status for .gnulib . maint.mk: found non-public submodule commit make: *** [/src/maint.mk:1448: public-submodule-commit] Error 1
This last part is interesting for people looking at the code but not for users, so I'd leave it out of the commit message.
It is important as it shows the maint.mk rule that is causing the problem we are fixing. Without that there's not enough context to undestand the problem.
My point is that this is really only interesting to people hacking on Makefile.ci, and it's just noise to all other developers going through the git log. So we should not drop the information, just move it to a comment inside Makefile.ci, possibly with the shell output snipped for brevity.
I really disagree. I want the commit messages to be more verbose so that details of what's being fixed are clearly visible. If you don't care about this detail fine, just ignore it, but that's not a reason to purge cut useful info out of the commit messages. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, 2019-04-03 at 11:44 +0100, Daniel P. Berrangé wrote:
On Mon, Apr 01, 2019 at 01:03:21PM +0200, Andrea Bolognani wrote:
On Mon, 2019-04-01 at 11:39 +0100, Daniel P. Berrangé wrote:
On Fri, Mar 29, 2019 at 05:39:59PM +0100, Andrea Bolognani wrote:
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
It is neccessary to disable the gnulib submodule commit check because this fails due to the way we have manually cloned submodule repos as primary git repos with their own .git directory, instead of letting git treat them as submodules in the top level .git directory.
make[1]: Entering directory '/src/build' fatal: Not a valid object name origin fatal: run_command returned non-zero status for .gnulib . maint.mk: found non-public submodule commit make: *** [/src/maint.mk:1448: public-submodule-commit] Error 1
This last part is interesting for people looking at the code but not for users, so I'd leave it out of the commit message.
It is important as it shows the maint.mk rule that is causing the problem we are fixing. Without that there's not enough context to undestand the problem.
My point is that this is really only interesting to people hacking on Makefile.ci, and it's just noise to all other developers going through the git log. So we should not drop the information, just move it to a comment inside Makefile.ci, possibly with the shell output snipped for brevity.
I really disagree. I want the commit messages to be more verbose so that details of what's being fixed are clearly visible. If you don't care about this detail fine, just ignore it, but that's not a reason to purge cut useful info out of the commit messages.
You're not really fixing anything with this commit, you're introducing a new feature and presenting its use and motivation behind it - up until the point where you spend a dozen lines to explain in detail why a certain implementation details is needed. This rationale should be in the code rather than in the commit message, so that the next person wondering what the gl_foo thingy is there for will be able to learn that without having to dig through the git history. At the same time, someone scrolling through the git history might be interested in the new cool feature you're adding, but will most likely not care about the implementation detail. It's also worth noting that you didn't include this detail in the commit messages for the first two revisions, so clearly you didn't deem it as critical until I asked about it during review ;) Anyway, if after reading the above you still want to keep it in that's fine: I think both my time and yours are better used on something else than arguing further about this :) -- Andrea Bolognani / Red Hat / Virtualization
Change the travis configuration to invoke the new cibuild-$IMAGE target instead of directly running docker. This guarantees that when a developer runs cibuild-$IMAGE locally, the container build setup is identical to that used in Travis, with exception of the host kernel and docker version. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 45 ++++++++++----------------------------------- 1 file changed, 10 insertions(+), 35 deletions(-) diff --git a/.travis.yml b/.travis.yml index 54de7dd7ad..6d04201e59 100644 --- a/.travis.yml +++ b/.travis.yml @@ -11,24 +11,30 @@ matrix: - docker env: - IMAGE="ubuntu-18" - - DOCKER_CMD="$LINUX_CMD" + - MAKE_ARGS="syntax-check distcheck" + script: + - make -f Makefile.ci ci-build@$IMAGE MAKE_ARGS="$MAKE_ARGS" - services: - docker env: - IMAGE="centos-7" - - DOCKER_CMD="$LINUX_CMD" + - MAKE_ARGS="syntax-check distcheck" + script: + - make -f Makefile.ci ci-build@$IMAGE MAKE_ARGS="$MAKE_ARGS" - services: - docker env: - IMAGE="fedora-rawhide" - MINGW="mingw32" - - DOCKER_CMD="$MINGW_CMD" + script: + - make -f Makefile.ci ci-build@$IMAGE CONFIGURE=$MINGW-configure - services: - docker env: - IMAGE="fedora-rawhide" - MINGW="mingw64" - - DOCKER_CMD="$MINGW_CMD" + script: + - make -f Makefile.ci ci-build@$IMAGE CONFIGURE=$MINGW-configure - compiler: clang language: c os: osx @@ -37,43 +43,12 @@ matrix: script: /bin/sh -xc "$MACOS_CMD" -script: - - docker run - -v $(pwd):/build - -w /build - -e VIR_TEST_DEBUG="$VIR_TEST_DEBUG" - -e MINGW="$MINGW" - "quay.io/libvirt/buildenv-$IMAGE:master" - /bin/sh -xc "$DOCKER_CMD" - git: submodules: true env: global: - VIR_TEST_DEBUG=1 - - LINUX_CMD=" - ./autogen.sh && - make -j3 syntax-check && - make -j3 distcheck || - ( - echo '=== LOG FILE(S) START ==='; - find -name test-suite.log | xargs cat; - echo '=== LOG FILE(S) END ==='; - exit 1 - ) - " - - MINGW_CMD=" - NOCONFIGURE=1 ./autogen.sh && - \$MINGW-configure && - make -j3 || - ( - echo '=== LOG FILE(S) START ==='; - find -name test-suite.log | xargs cat; - echo '=== LOG FILE(S) END ==='; - exit 1 - ) - " # We can't run 'distcheck' or 'syntax-check' because they fail on # macOS, but doing 'install' and 'dist' gives us some useful coverage - MACOS_CMD=" -- 2.20.1
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
Change the travis configuration to invoke the new cibuild-$IMAGE target instead of directly running docker. This guarantees that when a developer runs cibuild-$IMAGE locally, the container build setup is identical to that used in Travis, with exception of the host kernel and docker version.
It should be "Ubuntu", "CentOS", "MinGW", "Travis CI" and "Docker" everywhere in the commit message. [...]
+ script: + - make -f Makefile.ci ci-build@$IMAGE MAKE_ARGS="$MAKE_ARGS"
Two spaces before MAKE_ARGS. [...]
+ script: + - make -f Makefile.ci ci-build@$IMAGE MAKE_ARGS="$MAKE_ARGS"
Same. [...]
+ script: + - make -f Makefile.ci ci-build@$IMAGE CONFIGURE=$MINGW-configure
Please quote the value of CONFIGURE. [...]
+ script: + - make -f Makefile.ci ci-build@$IMAGE CONFIGURE=$MINGW-configure
Same. With all of the above fixed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Instead of running custom commands use the new declarative syntax for listing extra homebrew packages. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 6d04201e59..fe9bda82a9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,14 @@ branches: except: - /^.*-maint$/ +addons: + homebrew: + packages: + - ccache + - rpcgen + - xz + - yajl + matrix: include: - services: @@ -52,8 +60,6 @@ env: # We can't run 'distcheck' or 'syntax-check' because they fail on # macOS, but doing 'install' and 'dist' gives us some useful coverage - MACOS_CMD=" - brew update && - brew install ccache rpcgen xz yajl && ./autogen.sh --prefix=\$(pwd)/install-root && make -j3 && make -j3 install && -- 2.20.1
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
Instead of running custom commands use the new declarative syntax for listing extra homebrew packages.
s/homebrew/Homebrew/g in the commit message. [...]
+addons: + homebrew: + packages: + - ccache + - rpcgen + - xz + - yajl
According to the documentation[1], this is not equivalent to the code you're replacing since it will not run 'brew update' before installing packages. Updating the system before building sounds like a good idea to me, but I could be convinced otherwise :) Another thing I think is worth mentioning is that several times in the past we had to work around Homebrew issues, which is easy to do when you're calling it from the shell but not so when you use the declarative syntax. Switching back and forth is simple enough, though, so we can just do that if the need ever arises again. [1] https://docs.travis-ci.com/user/installing-dependencies/#installing-packages... -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Apr 01, 2019 at 10:08:45AM +0200, Andrea Bolognani wrote:
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
Instead of running custom commands use the new declarative syntax for listing extra homebrew packages.
s/homebrew/Homebrew/g in the commit message.
[...]
+addons: + homebrew: + packages: + - ccache + - rpcgen + - xz + - yajl
According to the documentation[1], this is not equivalent to the code you're replacing since it will not run 'brew update' before installing packages. Updating the system before building sounds like a good idea to me, but I could be convinced otherwise :)
I forgot this can be done by adding update: true Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, 2019-04-01 at 12:04 +0100, Daniel P. Berrangé wrote:
On Mon, Apr 01, 2019 at 10:08:45AM +0200, Andrea Bolognani wrote:
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
+addons: + homebrew: + packages: + - ccache + - rpcgen + - xz + - yajl
According to the documentation[1], this is not equivalent to the code you're replacing since it will not run 'brew update' before installing packages. Updating the system before building sounds like a good idea to me, but I could be convinced otherwise :)
I forgot this can be done by adding
update: true
Yeah, I saw that in the documentation. So do you think we should include that additional setting and keep updating the system before builds, as we have done until now? -- Andrea Bolognani / Red Hat / Virtualization
We are not running "make check" on macOS, so the commands to cat the test-suite.log are not useful. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/.travis.yml b/.travis.yml index fe9bda82a9..027fa0c67a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -63,14 +63,7 @@ env: ./autogen.sh --prefix=\$(pwd)/install-root && make -j3 && make -j3 install && - make -j3 dist || - ( - echo '=== LOG FILE(S) START ==='; - find -name test-suite.log | xargs cat; - echo '=== LOG FILE(S) END ==='; - exit 1 - ) - " + make -j3 dist" notifications: irc: -- 2.20.1
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
We are not running "make check" on macOS, so the commands to cat the test-suite.log are not useful.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/.travis.yml b/.travis.yml index 027fa0c67a..af17dc6276 100644 --- a/.travis.yml +++ b/.travis.yml @@ -48,23 +48,15 @@ matrix: os: osx env: - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/ccache/libexec:/usr/local/opt/rpcgen/bin:$PATH" + - VIR_TEST_DEBUG=1 script: - /bin/sh -xc "$MACOS_CMD" + # We can't run 'distcheck' or 'syntax-check' because they fail on + # macOS, but doing 'install' and 'dist' gives us some useful coverage + - ./autogen.sh --prefix=$(pwd)/install-root && make -j3 && make -j3 install && make -j3 dist git: submodules: true -env: - global: - - VIR_TEST_DEBUG=1 - # We can't run 'distcheck' or 'syntax-check' because they fail on - # macOS, but doing 'install' and 'dist' gives us some useful coverage - - MACOS_CMD=" - ./autogen.sh --prefix=\$(pwd)/install-root && - make -j3 && - make -j3 install && - make -j3 dist" - notifications: irc: # The channel name "irc.oftc.net#virt" is encrypted against libvirt/libvirt -- 2.20.1
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote: [...]
@@ -48,23 +48,15 @@ matrix: os: osx env: - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/ccache/libexec:/usr/local/opt/rpcgen/bin:$PATH" + - VIR_TEST_DEBUG=1
Since we're not running 'make check' on macOS, setting VIR_TEST_DEBUG in its environment is not particularly useful :) On the other hand, this made me realize that our new Makefile.ci does not set this environment variable itself, nor does it pass it to the container if it's already present. I think we can just set it unconditionally in Makefile.ci. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Apr 01, 2019 at 10:15:51AM +0200, Andrea Bolognani wrote:
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote: [...]
@@ -48,23 +48,15 @@ matrix: os: osx env: - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/ccache/libexec:/usr/local/opt/rpcgen/bin:$PATH" + - VIR_TEST_DEBUG=1
Since we're not running 'make check' on macOS, setting VIR_TEST_DEBUG in its environment is not particularly useful :)
Opps, should have been removed entirely in the patch that switches to use Makefile.ci for linux/mingw builds
On the other hand, this made me realize that our new Makefile.ci does not set this environment variable itself, nor does it pass it to the container if it's already present. I think we can just set it unconditionally in Makefile.ci.
Agreed. We can hardcode it safely even if not running the 'check' make target. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote: [...]
script: - /bin/sh -xc "$MACOS_CMD" + # We can't run 'distcheck' or 'syntax-check' because they fail on + # macOS, but doing 'install' and 'dist' gives us some useful coverage + - ./autogen.sh --prefix=$(pwd)/install-root && make -j3 && make -j3 install && make -j3 dist
One more thing: it would be nice if we could replace that '-j3' with something similar to the 'getconf _NPROCESSORS_ONLN' call we have in Makefile.ci for Linux builds. Of course it would be a separate change that we can do as a follow up, I'm just writing it down before I forget (again). -- Andrea Bolognani / Red Hat / Virtualization
On 3/27/19 12:10 PM, Daniel P. Berrangé wrote:
For a while QEMU has provided simple make rules for building QEMU inside standard docker container environments. This provides an equivalent mechanism for libvirt inspired by QEMU's.
QEMU actually builds the container images on developer's machines locally. Libvirt already hosts pre-built images on quay.io, so that is used directly as it is quicker to download them than to build them locally. This also ensures the container contents match what the live CI system is using, as opposed to building an image with newer packages.
NB, there is a currently a test failure with this change to travis that we don't currently see. Current travis runs a privileged docker container, but with this change we now run unprivileged containers. This shows that the "virsh-snapshot" test is trying to create a file under $HOME, and docker blocks this. Obviously needs fixing before pushing.
Patch for that fix has been posted. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Eric Blake