10:06 a.m.
On Fri, Mar 15, 2019 at 01:24:20PM +0100, Andrea Bolognani wrote:
On Wed, 2019-03-06 at 09:34 +0000, Daniel P. Berrangé wrote:
[...]
> +++ b/.gitignore
> @@ -46,6 +46,7 @@
> /autom4te.cache
> /build-aux/*
> /build/
> +/citree/
Bikeshedding: I would prefer if we used /ci-tree/ instead, and same
for the various ci-build, ci-shell, ci-... make targets.
Sure, I'm not fussed.
[...]
> +# Run in a separate build directory. Set to empty
> +# for a in-source tree build, but note SRCDIR must
> +# also be set to a corresponding relative path
> +VPATHDIR = vpath
> +SRCDIR = ..
Do we even care about in-source builds? It's usually VPATH builds
that will end up broken by mistake because someone was not careful
enough when tweaking the build system...
As long as in-source builds are permitted by libvirt, we must
provide support for using them here to reproduce problems.
Personally I think we should drop support for in-source builds
as having 2 different ways of building the same thing means we
frequently break one or the other. There were objections last
time I suggested that though. NB QEMU is about to drop support
for in-source builds to reduce breakage
Either way, since we have full control over the way local
directories
will show up inside the container, I would define these as
SRCDIR = /build
VPATHDIR = $(SRCDIR)/vpath
that way people will only ever have to override VPATHDIR.
I'm not sure that will work in all places I use these vars,
but will investigate.
> +# Docker doesn't require the IDs you run as to exist in
> +# the container's /etc/passwd & /etc/group files, but
> +# if they do not, then libvirt's 'make check' will fail
> +# many tests
> +PWDB_MOUNTS = \
> + --volume $(HERE)/citree/group:/etc/group:ro,z \
> + --volume $(HERE)/citree/passwd:/etc/passwd:ro,z
Why do we copy the files under /citree before handing them over to
Docker? Shouldn't we be able to mount them directly off /etc?
Prepare for a story of pain and suffering and swearing....
I did indeed just pass /etc/passwd direct to the --volume
arg when i first wrote this and it all worked "fine".
Then my screen locked and I couldn't log back in either
through GDM or the Linux text console or SSH
I had to reboot, at which point almost every single
systemd service failed.
Single user mode wouldn't even work.
After booting single user mode with SELinux disabled I
discovered that /etc/passwd now had the SELinux label
of the container I had just run, instead of "passwd_file_t"
Thus no process confined by SELinux had permission to
read /etc/passwd making life rather difficult.
There's probably a way to get docker to not screw with
the SELinux labels but I think it is more prudent to
take the absolutely guaranteed safe option & copy the
file :-)
> +GIT_ARGS = \
> + -c advice.detachedHead=false \
> + -q \
Why quiet? We're pretty verbose throughout the rest of the process.
I don't think the message from git were useful - the other
stuff you see is all related to the actual build process
which is the interesting bit.
> +preparetree:
s/tree/-tree/
> + @if test "$(RECLONE)" = "1" ; then \
> + rm -rf citree ; \
> + fi
> + @if ! test -d citree ; then \
> + mkdir -p citree/src; \
> + cp /etc/passwd citree; \
> + cp /etc/group citree; \
You can use $(CI_TREE) everywhere here.
> + echo "Cloning $(TOP) to $(HERE)/citree/root"; \
You're actually cloning to $(CI_TREE)/src here... And since this is
another path that you're using a lot, I would define
HOST_SRCDIR = $(CI_TREE)/src
and use that instead, to avoid this kind of error.
> + git clone $(GIT_ARGS) $(TOP) $(HERE)/citree/src || exit 1; \
> + for mod in $(SUBMODULES) ; \
> + do \
> + if test -d $(TOP)/$$mod ; \
> + then \
> + echo "Cloning $(TOP)/$$mod to $(HERE)/citree/$$mod"; \
The target is wrong here as well, it should be $(HOST_SRCDIR)/$$mod.
> + git clone $(GIT_ARGS) $(TOP)/$$mod $(HERE)/citree/src/$$mod || exit 1; \
> + fi ; \
> + done ; \
> + mkdir -p citree/src/$(VPATHDIR) ; \
Oh, you're using $(VPATHDIR) assuming it's a relative path here,
which would no longer work if you made it absolute as I suggested
above...
I would argue doing this as part of the prepare step is not the
best option anyway, and you should rather...
[...]
> +cibuild-%: checkdocker preparetree
> + docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)-$*$(IMAGE_TAG) \
> + /bin/bash -c '\
... add
mkdir -p $(BUILDDIR) && cd $(BUILDDIR)
Yes, we could do that.
> + NOCONFIGURE=1 $(SRCDIR)/autogen.sh || exit 1 ; \
> + $(CONFIGURE) $${CONFIGURE_OPTS}; $(CONFIGURE_ARGS) \
That semicolon will cause the build to fail if you dare setting
CONFIGURE_ARGS :)
Heh, opps.
As for CONFIGURE_OPTS, I think the name is pretty poor because it's
too similar to CONFIGURE_ARGS... We could either rename it to
something like CONFIGURE_IMAGE_ARGS, since it contains the arguments
that are baked in the Docker image, or CONFIGURE_CROSS_ARGS, since
we have no planned use (that I know of) outside of passing
information specific to cross-compilation. I think I have a slight
preference for the former, but either one would be an improvement.
I don't think the name similarity actually matters in practice
because only one of them is used by the developer, the other is
just internal to the image, and we've documented which to use
higher up in the makefile. So I'd rather just stick with
the more concise names.
> +cicheck-%:
> + $(MAKE) cibuild-$* MAKE_ARGS="check gl_public_submodule_commit="
This doesn't seem to work:
$ make -f tests/Makefile.ci.inc cicheck-centos-7
/usr/bin/gmake cibuild-centos-7 MAKE_ARGS="check
gl_public_submodule_commit="
gmake[1]: Entering directory '/home/test/libvirt'
gmake[1]: *** No rule to make target 'cibuild-centos-7'. Stop.
gmake[1]: Leaving directory '/home/test/libvirt'
make: *** [tests/Makefile.ci.inc:143: cicheck-centos-7] Error 2
The problem seems to be that it's not passing the '-f' argument to
the sub-make, so it would probably work in non-standalone mode, but
we should either make sure standalone mode works too or just snip it
and tell users to pass MAKE_ARGS manually instead.
Hmm, not sure what
One more question: what is gl_public_submodule_commit= supposed to
do? I tried running builds both with and without it, and the results
seem to be the same.
gnulib has a make check target that will fail as the way we have
cloned submodules isn't the normal way submodules are supposed
to be initialized by git.
> +cihelp:
> + @echo "Build libvirt inside docker containers used for CI"
> + @echo
> + @echo "Available targets:"
> + @echo
> + @echo " cibuild-\$$IMAGE - run a default 'make'"
> + @echo " cicheck-\$$IMAGE - run a 'make check'"
> + @echo " cishell-\$$IMAGE - run an interactive shell"
Just a thought: instead of
make ci-build-centos-7 MAKE_ARGS=check
and in the future
make ci-build-debian-9-cross-aarch64
would it make sense to have something like
make ci-build OS=centos-7 MAKE_ARGS=check
make ci-build OS=debian-9 CROSS=aarch64
instead? A bit more typing, perhaps, but it looks kinda better
in my opinion, with the variable parts clearly presented as such...
I rather prefer the more concise target names - I don't think it
really adds anything to use variables
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 :|