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.
[...]
> +++ b/tests/Makefile.ci.inc
I think this file belongs in the top-level rather than in tests/,
given that you can use it to perform a build that goes nowhere near
our test suite or even just spawn a shell inside the container.
Additionally, while it's true that we include the file from
Makefile.am, we also use it in a stand-alone fashion as part of
all the Travis CI jobs, so I would argue the .inc suffix is not
warranted.
> @@ -0,0 +1,174 @@
> +# -*- makefile -*-
This is pretty useful! We should also add
# vim: filetype=make
for those of us who prefer That Other Text Editor™ (and probably
introduce the same magic comments in the various Makefile.inc.am
and friends as well).
[...]
> +# 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...
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.
Now for the bikeshedding :)
I would use BUILDDIR rather than VPATHDIR because that matches the
name used by autotools for the same concept, and also because having
to set VPATHDIR when you *do not* want a VPATH build just seems off.
Additionally, having SRCDIR pointing to /build also strikes me as
fairly odd... Personally, I'd do
SRCDIR = /libvirt
BUILDDIR = $(SRCDIR)/build
instead.
[...]
> +# Avoid pulling submodules over the network by locally
> +# cloning them
> +SUBMODULES = .gnulib src/keycodemapdb
We should not hardcode this list, and figure it out dynamically by
calling 'git submodule' and parsing its output instead.
[...]
> +# We'll always freshly clone the virtual root each
> +# time in case it was not cleaned up before. Set
> +# to 0 if you want to try restarting a previously
> +# preserved env
> +RECLONE = 1
Bikeshedding: I'd call this REUSE, with of course the default being
0 and all the logic involving it being flipped.
[...]
> +# 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?
You're also not using $(NULL) to finish of the list here
[...]
> +# Args to use when cloning a git repo
It would be nice if you briefly documented these the same way you do
for Docker arguments.
> +GIT_ARGS = \
> + -c advice.detachedHead=false \
> + -q \
Why quiet? We're pretty verbose throughout the rest of the process.
> + --local \
According to the documentation, --local is implied when cloning from
a local path rather than a URL. Not that it hurts to keep it around.
[...]
> +# Args to use when running the docker env
> +# --rm stop inactive containers getting left behind
> +# --user we execute as the same user & group account
> +# as dev so that file ownership matches host
> +# instead of root:root
> +# --volume to pass in the cloned git repo & config
> +# --workdir to set cwd to vpath build location
> +# --ulimit lower files limit for performance reasons
> +# --interactive
> +# --tty Ensure we have ability to Ctrl-C the build
Adding this little reference sheet was a brilliant idea, thanks! :)
> +DOCKER_ARGS = \
> + --rm \
> + --user $(UID):$(GID) \
> + --interactive \
> + --tty \
> + $(PWDB_MOUNTS) \
> + --volume $(HERE)/citree/src:/build:z \
> + --workdir /build/$(VPATHDIR) \
So based on the comments above this would turn into
--volume $(HERE)/citree/src:$(SRCDIR):z \
--workdir $(BUILDDIR) \
I just noticed that you're using "$(HERE)/citree" a whole lot: I
think it would make sense to define
CI_TREE = $(HERE)/citree
earlier in the file and just use that instead.
[...]
> +checkdocker:
s/docker/-docker/
> + @echo -n "Checking if docker is available and running..." && \
s/docker/Docker/
s/.../... /
[...]
> +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)
here: this is consistent with how we run builds also on CentOS CI,
where the prepare step is handled by Jenkins and creating the VPATH
directory is part of the build script. In the current incarnation,
the fact that we're performing a VPATH build is not clear if you're
looking at the build instructions in isolation, which is also a
slight drawback which would be neatly solved this way.
> + 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 :)
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.
In any case, since we're not targeting the cross-compilation use
case in this first implementation, you can just leave that variable
out for now and add it back later along with the rest of the
cross-compilation support.
[...]
> + make -j $(SMP) $(MAKE_ARGS) ; \
The more common form would be "-j$(SMP)" from what I've seen. See
also the existing Travis CI rules.
[...]
> +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.
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.
[...]
> +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...
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
