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

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.

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

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

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