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