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

[Andrea Bolognani]

On Mon, 2019-04-01 at 11:39 +0100, Daniel P. Berrangé wrote:
> 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.

My point is that this is really only interesting to people hacking
on Makefile.ci, and it's just noise to all other developers going
through the git log. So we should not drop the information, just
move it to a comment inside Makefile.ci, possibly with the shell
output snipped for brevity.

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

Ouch. Yeah, the first option seems more palatable.

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

Okay, this makes sense. Just make sure you test with '-f' rather
than with '-d', because if the submodules have been initialized
with 'git submodule update --init' then .git will be a regular
file rather than a directory. And you can still use the shorter
version I suggested :)

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

Alright.

-- 
Andrea Bolognani / Red Hat / Virtualization