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.
[...]
> +HERE = $(shell pwd)
> +
> +# Figure out name and path to this file. This isn't
> +# portable but we only care for modern GNU make
> +THIS_FILE = $(abspath $(lastword $(MAKEFILE_LIST)))
> +
> +# The directory holding content on the host that we will
> +# expose to the container.
> +SCRATCHDIR = $(HERE)/ci-tree
Since you don't use $(HERE) anywhere else, you could do
SCRATCHDIR = $(shell pwd)/ci-tree
here.
[...]
> +# The directory holding the clo%%%%ne of the git repo that
Not sure what happened with that comment ;)
[...]
> +# The directory holding the source inside the
> +# container. ie where we told docker to expose
Again for all messages displayed to the user and comments,
s/docker/Docker/g
> +# the $GIT/ci-tree directory from the host
This is actually $(HOST_SRCDIR).
[...]
> +# 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.
[...]
> +# Avoid pulling submodules over the network by locally
> +# cloning them
> +SUBMODULES = .gnulib src/keycodemapdb
I still very much don't like having this hardcoded instead of
figured out at runtime by parsing the output of 'git submodule', but
we can take care of that in a follow-up patch.
[...]
> +prepare-tree:
> + @if test "$(REUSE)" != "1" ; then \
> + rm -rf ci-tree ; \
> + fi
> + @if ! test -d ci-tree ; then \
> + mkdir -p ci-tree/src; \
> + cp /etc/passwd ci-tree; \
> + cp /etc/group ci-tree; \
All instances of 'ci-tree' here, a few lines down, and also in
the ci-build@% and ci-shell@% targets, should be changed to
$(SCRATCHDIR).
[...]
> + 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:
$ make -f Makefile.ci ci-build@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; \
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:
$ make -f Makefile.ci ci-shell@debian-9 CLEAN=0
Cloning /home/test/libvirt to /home/test/libvirt/ci-tree/src
Cloning /home/test/libvirt/.gnulib to /home/test/libvirt/ci-tree/src/.gnulib
Cloning /home/test/libvirt/src/keycodemapdb to /home/test/libvirt/ci-tree/src/src/keycodemapdb
docker run --rm --user 1000:1000 --interactive --tty --volume /home/test/libvirt/ci-tree/group:/etc/group:ro,z --volume /home/test/libvirt/ci-tree/passwd:/etc/passwd:ro,z --volume /home/test/libvirt/ci-tree/src:/src:z --workdir /src --ulimit nofile=1024:1024 quay.io/libvirt/buildenv-debian-9:master /bin/bash
test@14f00bd5a24a:/src$ exit
$ make -f Makefile.ci ci-shell@debian-9 REUSE=1
docker run --rm --user 1000:1000 --interactive --tty --volume /home/test/libvirt/ci-tree/group:/etc/group:ro,z --volume /home/test/libvirt/ci-tree/passwd:/etc/passwd:ro,z --volume /home/test/libvirt/ci-tree/src:/src:z --workdir /src --ulimit nofile=1024:1024 quay.io/libvirt/buildenv-debian-9:master /bin/bash
/usr/bin/docker-current: Error response from daemon: oci runtime error: container_linux.go:247: starting container process caused "process_linux.go:364: container init caused \"rootfs_linux.go:54: mounting \\\"/home/test/libvirt/ci-tree/group\\\" to rootfs \\\"/var/lib/docker/overlay2/388b4560961db1d446ee8621903ff33d29e51e30de3a9a661448ebdac0fd9d39/merged\\\" at \\\"/var/lib/docker/overlay2/388b4560961db1d446ee8621903ff33d29e51e30de3a9a661448ebdac0fd9d39/merged/etc/group\\\" caused \\\"not a directory\\\"\""
: Are you trying to mount a directory onto a file (or vice-versa)? Check if the specified host path exists and is the expected type.
make: *** [Makefile.ci:176: ci-shell@debian-9] Error 125
Removing the else branch entirely makes this work.
[...]
> +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.
> + mkdir -p $(CONT_BUILDDIR) || exit 1 ; \
> + cd $(CONT_BUILDDIR) ; \
This should also have
|| exit 1
just in case.
[...]
> + find -name test-suite.log -delete ; \
I was today years old when I realized you can call find without
giving it a directory to search in, in which case it will start from
the current directory :)
> + 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@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.
[...]
> +ci-shell@%: prepare-tree
This should depend on check-docker too.
[...]
> + @echo "Available x86 container images:"
> + @echo
> + @echo " centos-7"
> + @echo " debian-8"
Not anymore!
> + @echo " debian-9"
> + @echo " debian-sid"
> + @echo " fedora-28"
> + @echo " fedora-29"
> + @echo " fedora-rawhide"
> + @echo " ubuntu-16"
Not anymore!
We should figure out a way to fetch this dynamically. We can do
that later, though.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
