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

Mon Apr 1 10:39:24 UTC 2019

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