[libvirt] [PATCH v2 0/2] 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. 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é (2): tests: add targets for building libvirt inside docker containers travis: convert ubuntu, centos & mingw builds to use new make rules .gitignore | 1 + .travis.yml | 48 +++--------- Makefile.am | 2 + tests/Makefile.ci.inc | 174 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 187 insertions(+), 38 deletions(-) create mode 100644 tests/Makefile.ci.inc -- 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 containr: make cibuild-fedora-28 To also run unit tests make cicheck-fedora-28 This is just syntax sugar for calling the previous command with a custom make target make cibuild-fedora-28 MAKE_ARGS="check" To do a purely interactive build it is possible to request a shell make cishell-fedora-28 To do a mingw build, it is currently possible to use the fedora-rawhide and request a different configure script make cibuild-fedora-rawhide CONFIGURE=mingw32-configure In all cases the GIT source tree is cloned locally into a 'citree/src' sub-directory which is then exposed to the container at '/build'. It is setup to facilitate VPATH build so the initial working directory is '/build/vpath'. An in source tree build can be requested instead by passing VPATHDIR= SRCDIR=. args 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. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .gitignore | 1 + Makefile.am | 2 + tests/Makefile.ci.inc | 174 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 tests/Makefile.ci.inc diff --git a/.gitignore b/.gitignore index 3303eed411..a30882d72b 100644 --- a/.gitignore +++ b/.gitignore @@ -46,6 +46,7 @@ /autom4te.cache /build-aux/* /build/ +/citree/ /confdefs.h /config.cache /config.guess diff --git a/Makefile.am b/Makefile.am index 709064c6a6..0c8ab13ebc 100644 --- a/Makefile.am +++ b/Makefile.am @@ -123,3 +123,5 @@ gen-AUTHORS: mv -f $(distdir)/AUTHORS-tmp $(distdir)/AUTHORS && \ rm -f all.list maint.list contrib.list; \ fi + +include tests/Makefile.ci.inc diff --git a/tests/Makefile.ci.inc b/tests/Makefile.ci.inc new file mode 100644 index 0000000000..7d521326cc --- /dev/null +++ b/tests/Makefile.ci.inc @@ -0,0 +1,174 @@ +# -*- makefile -*- + +HERE = $(shell pwd) +TOP = $(shell git rev-parse --show-toplevel) + +# Run in a separate build directory. Set to empty +# for a in-source tree build, but note SRCDIR must +# also be set to a corresponding relative path +VPATHDIR = vpath +SRCDIR = .. + +# Can be overridden with mingw{32,64}-configure if desired +CONFIGURE = $(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 0 if you want to try restarting a previously +# preserved env +RECLONE = 1 + +# 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 +PWDB_MOUNTS = \ + --volume $(HERE)/citree/group:/etc/group:ro,z \ + --volume $(HERE)/citree/passwd:/etc/passwd:ro,z + +# 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 +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 $(HERE)/citree/src:/build:z \ + --workdir /build/$(VPATHDIR) \ + --ulimit nofile=$(ULIMIT_FILES):$(ULIMIT_FILES) \ + $(NULL) + +checkdocker: + @echo -n "Checking if docker is available and running..." && \ + docker version 1>/dev/null && echo "yes" + +preparetree: + @if test "$(RECLONE)" = "1" ; then \ + rm -rf citree ; \ + fi + @if ! test -d citree ; then \ + mkdir -p citree/src; \ + cp /etc/passwd citree; \ + cp /etc/group citree; \ + echo "Cloning $(TOP) to $(HERE)/citree/root"; \ + git clone $(GIT_ARGS) $(TOP) $(HERE)/citree/src || exit 1; \ + for mod in $(SUBMODULES) ; \ + do \ + if test -d $(TOP)/$$mod ; \ + then \ + echo "Cloning $(TOP)/$$mod to $(HERE)/citree/$$mod"; \ + git clone $(GIT_ARGS) $(TOP)/$$mod $(HERE)/citree/src/$$mod || exit 1; \ + fi ; \ + done ; \ + mkdir -p citree/src/$(VPATHDIR) ; \ + else \ + test "$(CLEAN)" = "1" && rm -rf citree || : ; \ + 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) +cibuild-%: checkdocker preparetree + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)-$*$(IMAGE_TAG) \ + /bin/bash -c '\ + NOCONFIGURE=1 $(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 citree || : + +cicheck-%: + $(MAKE) cibuild-$* MAKE_ARGS="check gl_public_submodule_commit=" + +cishell-%: preparetree + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)-$*$(IMAGE_TAG) /bin/bash + @test "$(CLEAN)" = "1" && rm -rf citree || : + +cihelp: + @echo "Build libvirt inside docker containers used for CI" + @echo + @echo "Available targets:" + @echo + @echo " cibuild-\$$IMAGE - run a default 'make'" + @echo " cicheck-\$$IMAGE - run a 'make check'" + @echo " cishell-\$$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 '$(HERE)/citree' after completion" + @echo " RECLONE=0 - re-use existing '$(HERE)/citree' content" + @echo -- 2.20.1
On Wed, 2019-03-06 at 09:34 +0000, Daniel P. Berrangé wrote: [...]
+++ b/.gitignore @@ -46,6 +46,7 @@ /autom4te.cache /build-aux/* /build/ +/citree/
Bikeshedding: I would prefer if we used /ci-tree/ instead, and same for the various ci-build, ci-shell, ci-... make targets. [...]
+++ b/tests/Makefile.ci.inc
I think this file belongs in the top-level rather than in tests/, given that you can use it to perform a build that goes nowhere near our test suite or even just spawn a shell inside the container. Additionally, while it's true that we include the file from Makefile.am, we also use it in a stand-alone fashion as part of all the Travis CI jobs, so I would argue the .inc suffix is not warranted.
@@ -0,0 +1,174 @@ +# -*- makefile -*-
This is pretty useful! We should also add # vim: filetype=make for those of us who prefer That Other Text Editor™ (and probably introduce the same magic comments in the various Makefile.inc.am and friends as well). [...]
+# Run in a separate build directory. Set to empty +# for a in-source tree build, but note SRCDIR must +# also be set to a corresponding relative path +VPATHDIR = vpath +SRCDIR = ..
Do we even care about in-source builds? It's usually VPATH builds that will end up broken by mistake because someone was not careful enough when tweaking the build system... Either way, since we have full control over the way local directories will show up inside the container, I would define these as SRCDIR = /build VPATHDIR = $(SRCDIR)/vpath that way people will only ever have to override VPATHDIR. Now for the bikeshedding :) I would use BUILDDIR rather than VPATHDIR because that matches the name used by autotools for the same concept, and also because having to set VPATHDIR when you *do not* want a VPATH build just seems off. Additionally, having SRCDIR pointing to /build also strikes me as fairly odd... Personally, I'd do SRCDIR = /libvirt BUILDDIR = $(SRCDIR)/build instead. [...]
+# Avoid pulling submodules over the network by locally +# cloning them +SUBMODULES = .gnulib src/keycodemapdb
We should not hardcode this list, and figure it out dynamically by calling 'git submodule' and parsing its output instead. [...]
+# We'll always freshly clone the virtual root each +# time in case it was not cleaned up before. Set +# to 0 if you want to try restarting a previously +# preserved env +RECLONE = 1
Bikeshedding: I'd call this REUSE, with of course the default being 0 and all the logic involving it being flipped. [...]
+# 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 +PWDB_MOUNTS = \ + --volume $(HERE)/citree/group:/etc/group:ro,z \ + --volume $(HERE)/citree/passwd:/etc/passwd:ro,z
Why do we copy the files under /citree before handing them over to Docker? Shouldn't we be able to mount them directly off /etc? You're also not using $(NULL) to finish of the list here [...]
+# Args to use when cloning a git repo
It would be nice if you briefly documented these the same way you do for Docker arguments.
+GIT_ARGS = \ + -c advice.detachedHead=false \ + -q \
Why quiet? We're pretty verbose throughout the rest of the process.
+ --local \
According to the documentation, --local is implied when cloning from a local path rather than a URL. Not that it hurts to keep it around. [...]
+# 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
Adding this little reference sheet was a brilliant idea, thanks! :)
+DOCKER_ARGS = \ + --rm \ + --user $(UID):$(GID) \ + --interactive \ + --tty \ + $(PWDB_MOUNTS) \ + --volume $(HERE)/citree/src:/build:z \ + --workdir /build/$(VPATHDIR) \
So based on the comments above this would turn into --volume $(HERE)/citree/src:$(SRCDIR):z \ --workdir $(BUILDDIR) \ I just noticed that you're using "$(HERE)/citree" a whole lot: I think it would make sense to define CI_TREE = $(HERE)/citree earlier in the file and just use that instead. [...]
+checkdocker:
s/docker/-docker/
+ @echo -n "Checking if docker is available and running..." && \
s/docker/Docker/ s/.../... / [...]
+preparetree:
s/tree/-tree/
+ @if test "$(RECLONE)" = "1" ; then \ + rm -rf citree ; \ + fi + @if ! test -d citree ; then \ + mkdir -p citree/src; \ + cp /etc/passwd citree; \ + cp /etc/group citree; \
You can use $(CI_TREE) everywhere here.
+ echo "Cloning $(TOP) to $(HERE)/citree/root"; \
You're actually cloning to $(CI_TREE)/src here... And since this is another path that you're using a lot, I would define HOST_SRCDIR = $(CI_TREE)/src and use that instead, to avoid this kind of error.
+ git clone $(GIT_ARGS) $(TOP) $(HERE)/citree/src || exit 1; \ + for mod in $(SUBMODULES) ; \ + do \ + if test -d $(TOP)/$$mod ; \ + then \ + echo "Cloning $(TOP)/$$mod to $(HERE)/citree/$$mod"; \
The target is wrong here as well, it should be $(HOST_SRCDIR)/$$mod.
+ git clone $(GIT_ARGS) $(TOP)/$$mod $(HERE)/citree/src/$$mod || exit 1; \ + fi ; \ + done ; \ + mkdir -p citree/src/$(VPATHDIR) ; \
Oh, you're using $(VPATHDIR) assuming it's a relative path here, which would no longer work if you made it absolute as I suggested above... I would argue doing this as part of the prepare step is not the best option anyway, and you should rather... [...]
+cibuild-%: checkdocker preparetree + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)-$*$(IMAGE_TAG) \ + /bin/bash -c '\
... add mkdir -p $(BUILDDIR) && cd $(BUILDDIR) here: this is consistent with how we run builds also on CentOS CI, where the prepare step is handled by Jenkins and creating the VPATH directory is part of the build script. In the current incarnation, the fact that we're performing a VPATH build is not clear if you're looking at the build instructions in isolation, which is also a slight drawback which would be neatly solved this way.
+ NOCONFIGURE=1 $(SRCDIR)/autogen.sh || exit 1 ; \ + $(CONFIGURE) $${CONFIGURE_OPTS}; $(CONFIGURE_ARGS) \
That semicolon will cause the build to fail if you dare setting CONFIGURE_ARGS :) As for CONFIGURE_OPTS, I think the name is pretty poor because it's too similar to CONFIGURE_ARGS... We could either rename it to something like CONFIGURE_IMAGE_ARGS, since it contains the arguments that are baked in the Docker image, or CONFIGURE_CROSS_ARGS, since we have no planned use (that I know of) outside of passing information specific to cross-compilation. I think I have a slight preference for the former, but either one would be an improvement. In any case, since we're not targeting the cross-compilation use case in this first implementation, you can just leave that variable out for now and add it back later along with the rest of the cross-compilation support. [...]
+ make -j $(SMP) $(MAKE_ARGS) ; \
The more common form would be "-j$(SMP)" from what I've seen. See also the existing Travis CI rules. [...]
+cicheck-%: + $(MAKE) cibuild-$* MAKE_ARGS="check gl_public_submodule_commit="
This doesn't seem to work: $ make -f tests/Makefile.ci.inc cicheck-centos-7 /usr/bin/gmake cibuild-centos-7 MAKE_ARGS="check gl_public_submodule_commit=" gmake[1]: Entering directory '/home/test/libvirt' gmake[1]: *** No rule to make target 'cibuild-centos-7'. Stop. gmake[1]: Leaving directory '/home/test/libvirt' make: *** [tests/Makefile.ci.inc:143: cicheck-centos-7] Error 2 The problem seems to be that it's not passing the '-f' argument to the sub-make, so it would probably work in non-standalone mode, but we should either make sure standalone mode works too or just snip it and tell users to pass MAKE_ARGS manually instead. One more question: what is gl_public_submodule_commit= supposed to do? I tried running builds both with and without it, and the results seem to be the same. [...]
+cihelp: + @echo "Build libvirt inside docker containers used for CI" + @echo + @echo "Available targets:" + @echo + @echo " cibuild-\$$IMAGE - run a default 'make'" + @echo " cicheck-\$$IMAGE - run a 'make check'" + @echo " cishell-\$$IMAGE - run an interactive shell"
Just a thought: instead of make ci-build-centos-7 MAKE_ARGS=check and in the future make ci-build-debian-9-cross-aarch64 would it make sense to have something like make ci-build OS=centos-7 MAKE_ARGS=check make ci-build OS=debian-9 CROSS=aarch64 instead? A bit more typing, perhaps, but it looks kinda better in my opinion, with the variable parts clearly presented as such... -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 15, 2019 at 01:24:20PM +0100, Andrea Bolognani wrote:
On Wed, 2019-03-06 at 09:34 +0000, Daniel P. Berrangé wrote: [...]
+++ b/.gitignore @@ -46,6 +46,7 @@ /autom4te.cache /build-aux/* /build/ +/citree/
Bikeshedding: I would prefer if we used /ci-tree/ instead, and same for the various ci-build, ci-shell, ci-... make targets.
Sure, I'm not fussed.
[...]
+# Run in a separate build directory. Set to empty +# for a in-source tree build, but note SRCDIR must +# also be set to a corresponding relative path +VPATHDIR = vpath +SRCDIR = ..
Do we even care about in-source builds? It's usually VPATH builds that will end up broken by mistake because someone was not careful enough when tweaking the build system...
As long as in-source builds are permitted by libvirt, we must provide support for using them here to reproduce problems. Personally I think we should drop support for in-source builds as having 2 different ways of building the same thing means we frequently break one or the other. There were objections last time I suggested that though. NB QEMU is about to drop support for in-source builds to reduce breakage
Either way, since we have full control over the way local directories will show up inside the container, I would define these as
SRCDIR = /build VPATHDIR = $(SRCDIR)/vpath
that way people will only ever have to override VPATHDIR.
I'm not sure that will work in all places I use these vars, but will investigate.
+# 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 +PWDB_MOUNTS = \ + --volume $(HERE)/citree/group:/etc/group:ro,z \ + --volume $(HERE)/citree/passwd:/etc/passwd:ro,z
Why do we copy the files under /citree before handing them over to Docker? Shouldn't we be able to mount them directly off /etc?
Prepare for a story of pain and suffering and swearing.... I did indeed just pass /etc/passwd direct to the --volume arg when i first wrote this and it all worked "fine". Then my screen locked and I couldn't log back in either through GDM or the Linux text console or SSH I had to reboot, at which point almost every single systemd service failed. Single user mode wouldn't even work. After booting single user mode with SELinux disabled I discovered that /etc/passwd now had the SELinux label of the container I had just run, instead of "passwd_file_t" Thus no process confined by SELinux had permission to read /etc/passwd making life rather difficult. There's probably a way to get docker to not screw with the SELinux labels but I think it is more prudent to take the absolutely guaranteed safe option & copy the file :-)
+GIT_ARGS = \ + -c advice.detachedHead=false \ + -q \
Why quiet? We're pretty verbose throughout the rest of the process.
I don't think the message from git were useful - the other stuff you see is all related to the actual build process which is the interesting bit.
+preparetree:
s/tree/-tree/
+ @if test "$(RECLONE)" = "1" ; then \ + rm -rf citree ; \ + fi + @if ! test -d citree ; then \ + mkdir -p citree/src; \ + cp /etc/passwd citree; \ + cp /etc/group citree; \
You can use $(CI_TREE) everywhere here.
+ echo "Cloning $(TOP) to $(HERE)/citree/root"; \
You're actually cloning to $(CI_TREE)/src here... And since this is another path that you're using a lot, I would define
HOST_SRCDIR = $(CI_TREE)/src
and use that instead, to avoid this kind of error.
+ git clone $(GIT_ARGS) $(TOP) $(HERE)/citree/src || exit 1; \ + for mod in $(SUBMODULES) ; \ + do \ + if test -d $(TOP)/$$mod ; \ + then \ + echo "Cloning $(TOP)/$$mod to $(HERE)/citree/$$mod"; \
The target is wrong here as well, it should be $(HOST_SRCDIR)/$$mod.
+ git clone $(GIT_ARGS) $(TOP)/$$mod $(HERE)/citree/src/$$mod || exit 1; \ + fi ; \ + done ; \ + mkdir -p citree/src/$(VPATHDIR) ; \
Oh, you're using $(VPATHDIR) assuming it's a relative path here, which would no longer work if you made it absolute as I suggested above...
I would argue doing this as part of the prepare step is not the best option anyway, and you should rather...
[...]
+cibuild-%: checkdocker preparetree + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)-$*$(IMAGE_TAG) \ + /bin/bash -c '\
... add
mkdir -p $(BUILDDIR) && cd $(BUILDDIR)
Yes, we could do that.
+ NOCONFIGURE=1 $(SRCDIR)/autogen.sh || exit 1 ; \ + $(CONFIGURE) $${CONFIGURE_OPTS}; $(CONFIGURE_ARGS) \
That semicolon will cause the build to fail if you dare setting CONFIGURE_ARGS :)
Heh, opps.
As for CONFIGURE_OPTS, I think the name is pretty poor because it's too similar to CONFIGURE_ARGS... We could either rename it to something like CONFIGURE_IMAGE_ARGS, since it contains the arguments that are baked in the Docker image, or CONFIGURE_CROSS_ARGS, since we have no planned use (that I know of) outside of passing information specific to cross-compilation. I think I have a slight preference for the former, but either one would be an improvement.
I don't think the name similarity actually matters in practice because only one of them is used by the developer, the other is just internal to the image, and we've documented which to use higher up in the makefile. So I'd rather just stick with the more concise names.
+cicheck-%: + $(MAKE) cibuild-$* MAKE_ARGS="check gl_public_submodule_commit="
This doesn't seem to work:
$ make -f tests/Makefile.ci.inc cicheck-centos-7 /usr/bin/gmake cibuild-centos-7 MAKE_ARGS="check gl_public_submodule_commit=" gmake[1]: Entering directory '/home/test/libvirt' gmake[1]: *** No rule to make target 'cibuild-centos-7'. Stop. gmake[1]: Leaving directory '/home/test/libvirt' make: *** [tests/Makefile.ci.inc:143: cicheck-centos-7] Error 2
The problem seems to be that it's not passing the '-f' argument to the sub-make, so it would probably work in non-standalone mode, but we should either make sure standalone mode works too or just snip it and tell users to pass MAKE_ARGS manually instead.
Hmm, not sure what
One more question: what is gl_public_submodule_commit= supposed to do? I tried running builds both with and without it, and the results seem to be the same.
gnulib has a make check target that will fail as the way we have cloned submodules isn't the normal way submodules are supposed to be initialized by git.
+cihelp: + @echo "Build libvirt inside docker containers used for CI" + @echo + @echo "Available targets:" + @echo + @echo " cibuild-\$$IMAGE - run a default 'make'" + @echo " cicheck-\$$IMAGE - run a 'make check'" + @echo " cishell-\$$IMAGE - run an interactive shell"
Just a thought: instead of
make ci-build-centos-7 MAKE_ARGS=check
and in the future
make ci-build-debian-9-cross-aarch64
would it make sense to have something like
make ci-build OS=centos-7 MAKE_ARGS=check make ci-build OS=debian-9 CROSS=aarch64
instead? A bit more typing, perhaps, but it looks kinda better in my opinion, with the variable parts clearly presented as such...
I rather prefer the more concise target names - I don't think it really adds anything to use variables 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 Fri, 2019-03-15 at 15:06 +0000, Daniel P. Berrangé wrote:
On Fri, Mar 15, 2019 at 01:24:20PM +0100, Andrea Bolognani wrote:
[...]
+# Run in a separate build directory. Set to empty +# for a in-source tree build, but note SRCDIR must +# also be set to a corresponding relative path +VPATHDIR = vpath +SRCDIR = ..
Do we even care about in-source builds? It's usually VPATH builds that will end up broken by mistake because someone was not careful enough when tweaking the build system...
As long as in-source builds are permitted by libvirt, we must provide support for using them here to reproduce problems.
Personally I think we should drop support for in-source builds as having 2 different ways of building the same thing means we frequently break one or the other. There were objections last time I suggested that though. NB QEMU is about to drop support for in-source builds to reduce breakage
I'm aware of the conversation happening in QEMU land, and in fact that's at least in part what caused the question :) But you're right, as long as we support both in libvirt we should also provide a way to pick one or the other when (CI) building.
Either way, since we have full control over the way local directories will show up inside the container, I would define these as
SRCDIR = /build VPATHDIR = $(SRCDIR)/vpath
that way people will only ever have to override VPATHDIR.
I'm not sure that will work in all places I use these vars, but will investigate.
I have mentioned some potential issues later in the mail, but along with those I have also provided solutions so I think we should be good :)
+# 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 +PWDB_MOUNTS = \ + --volume $(HERE)/citree/group:/etc/group:ro,z \ + --volume $(HERE)/citree/passwd:/etc/passwd:ro,z
Why do we copy the files under /citree before handing them over to Docker? Shouldn't we be able to mount them directly off /etc?
Prepare for a story of pain and suffering and swearing....
I did indeed just pass /etc/passwd direct to the --volume arg when i first wrote this and it all worked "fine".
Then my screen locked and I couldn't log back in either through GDM or the Linux text console or SSH
I had to reboot, at which point almost every single systemd service failed.
Single user mode wouldn't even work.
After booting single user mode with SELinux disabled I discovered that /etc/passwd now had the SELinux label of the container I had just run, instead of "passwd_file_t"
Thus no process confined by SELinux had permission to read /etc/passwd making life rather difficult.
There's probably a way to get docker to not screw with the SELinux labels but I think it is more prudent to take the absolutely guaranteed safe option & copy the file :-)
Ouch :( Perhaps we could add a very short comment mentioning the reason for making a copy of the files, for the benefit of whoever will touch the script months down the line. [...]
+GIT_ARGS = \ + -c advice.detachedHead=false \ + -q \
Why quiet? We're pretty verbose throughout the rest of the process.
I don't think the message from git were useful - the other stuff you see is all related to the actual build process which is the interesting bit.
Alright. [...]
As for CONFIGURE_OPTS, I think the name is pretty poor because it's too similar to CONFIGURE_ARGS... We could either rename it to something like CONFIGURE_IMAGE_ARGS, since it contains the arguments that are baked in the Docker image, or CONFIGURE_CROSS_ARGS, since we have no planned use (that I know of) outside of passing information specific to cross-compilation. I think I have a slight preference for the former, but either one would be an improvement.
I don't think the name similarity actually matters in practice because only one of them is used by the developer, the other is just internal to the image, and we've documented which to use higher up in the makefile. So I'd rather just stick with the more concise names.
I'd really prefer a name that removes the ambiguity. [...]
One more question: what is gl_public_submodule_commit= supposed to do? I tried running builds both with and without it, and the results seem to be the same.
gnulib has a make check target that will fail as the way we have cloned submodules isn't the normal way submodules are supposed to be initialized by git.
Hm, can you double check please? As mentioned above, I tried both with and without the make variable being set, and in both cases gnulib's 'make check' passed. [...]
+ @echo "Available targets:" + @echo + @echo " cibuild-\$$IMAGE - run a default 'make'" + @echo " cicheck-\$$IMAGE - run a 'make check'" + @echo " cishell-\$$IMAGE - run an interactive shell"
Just a thought: instead of
make ci-build-centos-7 MAKE_ARGS=check
and in the future
make ci-build-debian-9-cross-aarch64
would it make sense to have something like
make ci-build OS=centos-7 MAKE_ARGS=check make ci-build OS=debian-9 CROSS=aarch64
instead? A bit more typing, perhaps, but it looks kinda better in my opinion, with the variable parts clearly presented as such...
I rather prefer the more concise target names - I don't think it really adds anything to use variables
I disagree on concise: they're definitely shorter, but that's because all the information is squished together, which makes it harder to parse at a glance. When naming Docker images we don't have much of a choice, because we have pretty much the same restrictions as when naming files, but that's not the case here so we could do better... With that said, if you feel strongly about the current names I won't stand in the way :) -- Andrea Bolognani / Red Hat / Virtualization
On Fri, 2019-03-15 at 17:20 +0100, Andrea Bolognani wrote:
On Fri, 2019-03-15 at 15:06 +0000, Daniel P. Berrangé wrote:
+ @echo "Available targets:" + @echo + @echo " cibuild-\$$IMAGE - run a default 'make'" + @echo " cicheck-\$$IMAGE - run a 'make check'" + @echo " cishell-\$$IMAGE - run an interactive shell"
Just a thought: instead of
make ci-build-centos-7 MAKE_ARGS=check
and in the future
make ci-build-debian-9-cross-aarch64
would it make sense to have something like
make ci-build OS=centos-7 MAKE_ARGS=check make ci-build OS=debian-9 CROSS=aarch64
instead? A bit more typing, perhaps, but it looks kinda better in my opinion, with the variable parts clearly presented as such...
I rather prefer the more concise target names - I don't think it really adds anything to use variables
I disagree on concise: they're definitely shorter, but that's because all the information is squished together, which makes it harder to parse at a glance.
When naming Docker images we don't have much of a choice, because we have pretty much the same restrictions as when naming files, but that's not the case here so we could do better...
I see QEMU uses $ make docker ... docker-TEST@IMAGE: Run "TEST" in container "IMAGE". Note: "TEST" is one of the listed test name, or a script name under $QEMU_SRC/tests/docker/; "IMAGE" is one of the listed container name." I think adopting that convention, thus ending up with $ make ci-build@centos-7 MAKE_ARGS=check $ make ci-build@debian-9-cross-aarch64 would be a reasonable compromise between your approach and mine. -- 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 | 48 ++++++++++-------------------------------------- 1 file changed, 10 insertions(+), 38 deletions(-) diff --git a/.travis.yml b/.travis.yml index 55ba340a34..c093dbf550 100644 --- a/.travis.yml +++ b/.travis.yml @@ -11,26 +11,30 @@ matrix: - docker env: - IMAGE="ubuntu-18" - - DISTCHECK_CONFIGURE_FLAGS="--with-init-script=systemd" - - DOCKER_CMD="$LINUX_CMD" + - MAKE_ARGS="syntax-check distcheck DISTCHECK_CONFIGURE_FLAGS=--with-init-script-systemd" + script: + - make -f tests/Makefile.ci.inc cibuild-$IMAGE MAKE_ARGS="$MAKE_ARGS" - services: - docker env: - IMAGE="centos-7" - - DISTCHECK_CONFIGURE_FLAGS="--with-init-script=upstart" - - DOCKER_CMD="$LINUX_CMD" + - MAKE_ARGS="syntax-check distcheck DISTCHECK_CONFIGURE_FLAGS=--with-init-script-systemd" + script: + - make -f tests/Makefile.ci.inc cibuild-$IMAGE MAKE_ARGS="$MAKE_ARGS" - services: - docker env: - IMAGE="fedora-rawhide" - MINGW="mingw32" - - DOCKER_CMD="$MINGW_CMD" + script: + - make -f tests/Makefile.ci.inc cibuild-$IMAGE CONFIGURE=$MINGW-configure MAKE_ARGS="$MAKE_ARGS" - services: - docker env: - IMAGE="fedora-rawhide" - MINGW="mingw64" - - DOCKER_CMD="$MINGW_CMD" + script: + - make -f tests/Makefile.ci.inc cibuild-$IMAGE CONFIGURE=$MINGW-configure MAKE_ARGS="$MAKE_ARGS" - compiler: clang language: c os: osx @@ -39,44 +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" - -e DISTCHECK_CONFIGURE_FLAGS="$DISTCHECK_CONFIGURE_FLAGS" - "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 DISTCHECK_CONFIGURE_FLAGS=\"\$DISTCHECK_CONFIGURE_FLAGS\" || - ( - 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-06 at 09:34 +0000, Daniel P. Berrangé wrote:
+++ b/.travis.yml @@ -11,26 +11,30 @@ matrix: - docker env: - IMAGE="ubuntu-18" - - DISTCHECK_CONFIGURE_FLAGS="--with-init-script=systemd" - - DOCKER_CMD="$LINUX_CMD" + - MAKE_ARGS="syntax-check distcheck DISTCHECK_CONFIGURE_FLAGS=--with-init-script-systemd" + script: + - make -f tests/Makefile.ci.inc cibuild-$IMAGE MAKE_ARGS="$MAKE_ARGS"
Having a separate 'script' for each job in the matrix defeats the purpose of setting values in the environment. I would drop MINGW_CMD, change the existing LINUX_CMD to env: global: - LINUX_CMD=' if test "$MINGW"; then make -f tests/Makefile.ci.inc "cibuild-$IMAGE" MAKE_ARGS="$MAKE_ARGS" CONFIGURE="$MINGW-configure"; else make -f tests/Makefile.ci.inc "cibuild-$IMAGE" MAKE_ARGS="$MAKE_ARGS"; fi ' and the existing default script to script: - /bin/sh -xc "$LINUX_CMD" Alternatively, you can drop all _CMD variables, including MACOS_CMD, and put the shell code right into the 'script' directives, but doing so would mean losing the tracing feature which can be useful to track down build issues... -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 15, 2019 at 02:27:55PM +0100, Andrea Bolognani wrote:
On Wed, 2019-03-06 at 09:34 +0000, Daniel P. Berrangé wrote:
+++ b/.travis.yml @@ -11,26 +11,30 @@ matrix: - docker env: - IMAGE="ubuntu-18" - - DISTCHECK_CONFIGURE_FLAGS="--with-init-script=systemd" - - DOCKER_CMD="$LINUX_CMD" + - MAKE_ARGS="syntax-check distcheck DISTCHECK_CONFIGURE_FLAGS=--with-init-script-systemd" + script: + - make -f tests/Makefile.ci.inc cibuild-$IMAGE MAKE_ARGS="$MAKE_ARGS"
Having a separate 'script' for each job in the matrix defeats the purpose of setting values in the environment.
This could be written just using the script: entry without any env vars, but the env vars have the useful property that they are displayed in the build result page. So you can see the difference in each matrix entry at a glance.
I would drop MINGW_CMD, change the existing LINUX_CMD to
env: global: - LINUX_CMD=' if test "$MINGW"; then make -f tests/Makefile.ci.inc "cibuild-$IMAGE" MAKE_ARGS="$MAKE_ARGS" CONFIGURE="$MINGW-configure"; else make -f tests/Makefile.ci.inc "cibuild-$IMAGE" MAKE_ARGS="$MAKE_ARGS"; fi '
and the existing default script to
script: - /bin/sh -xc "$LINUX_CMD"
I really don't like having so many levels of indirection in the commands. It gets painful to understand, especially when you consider the shell quoting & escaping needs. I'd rather have duplication of the "script" entry that is easy to understand. 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 Fri, 2019-03-15 at 13:44 +0000, Daniel P. Berrangé wrote:
On Fri, Mar 15, 2019 at 02:27:55PM +0100, Andrea Bolognani wrote:
On Wed, 2019-03-06 at 09:34 +0000, Daniel P. Berrangé wrote:
+++ b/.travis.yml @@ -11,26 +11,30 @@ matrix: - docker env: - IMAGE="ubuntu-18" - - DISTCHECK_CONFIGURE_FLAGS="--with-init-script=systemd" - - DOCKER_CMD="$LINUX_CMD" + - MAKE_ARGS="syntax-check distcheck DISTCHECK_CONFIGURE_FLAGS=--with-init-script-systemd" + script: + - make -f tests/Makefile.ci.inc cibuild-$IMAGE MAKE_ARGS="$MAKE_ARGS"
Having a separate 'script' for each job in the matrix defeats the purpose of setting values in the environment.
This could be written just using the script: entry without any env vars, but the env vars have the useful property that they are displayed in the build result page. So you can see the difference in each matrix entry at a glance.
Agreed, that's why I didn't suggest touching the environment :)
I would drop MINGW_CMD, change the existing LINUX_CMD to
env: global: - LINUX_CMD=' if test "$MINGW"; then make -f tests/Makefile.ci.inc "cibuild-$IMAGE" MAKE_ARGS="$MAKE_ARGS" CONFIGURE="$MINGW-configure"; else make -f tests/Makefile.ci.inc "cibuild-$IMAGE" MAKE_ARGS="$MAKE_ARGS"; fi '
and the existing default script to
script: - /bin/sh -xc "$LINUX_CMD"
I really don't like having so many levels of indirection in the commands. It gets painful to understand, especially when you consider the shell quoting & escaping needs.
I'd rather have duplication of the "script" entry that is easy to understand.
Then go for the second suggestion, which you snipped in your reply:
Alternatively, you can drop all _CMD variables, including MACOS_CMD, and put the shell code right into the 'script' directives, but doing so would mean losing the tracing feature which can be useful to track down build issues...
Doing so would remove the indirection, even for macOS builds, while still avoiding duplicated script directives. -- Andrea Bolognani / Red Hat / Virtualization
ping On Wed, Mar 06, 2019 at 09:34:22AM +0000, 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.
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é (2): tests: add targets for building libvirt inside docker containers travis: convert ubuntu, centos & mingw builds to use new make rules
.gitignore | 1 + .travis.yml | 48 +++--------- Makefile.am | 2 + tests/Makefile.ci.inc | 174 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 187 insertions(+), 38 deletions(-) create mode 100644 tests/Makefile.ci.inc
-- 2.20.1
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 :|
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé