Re: [libvirt] [PATCH v3 2/6] tests: add targets for building libvirt inside docker containers

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.

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

> + 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:

[...] > +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.

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