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.
Getting the right docker command syntax to use them, however, is not
entirely easy. This patch addresses that usability issue by introducing
some make targets. To run a simple build (aka 'make all') using the
Fedora 28 container:
make ci-build@fedora-28
To also run unit tests
make ci-check@fedora-28
This is just syntax sugar for calling the previous command with a
custom make target
make ci-build@fedora-28 MAKE_ARGS="check"
To do a purely interactive build it is possible to request a shell
make ci-shell@fedora-28
To do a mingw build, it is currently possible to use the fedora-rawhide
and request a different configure script
make ci-build@fedora-rawhide CONFIGURE=mingw32-configure
It is also possible to do cross compiled builds via the Debian containers
make ci-build@debian-9-cross-s390x
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 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
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
.gitignore | 1 +
Makefile.am | 2 +
Makefile.ci | 206 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 209 insertions(+)
create mode 100644 Makefile.ci
diff --git a/.gitignore b/.gitignore
index 3223cf1b76..a67a842294 100644
--- a/.gitignore
+++ b/.gitignore
@@ -45,6 +45,7 @@
/autom4te.cache
/build-aux/*
/build/
+/ci-tree/
/confdefs.h
/config.cache
/config.guess
diff --git a/Makefile.am b/Makefile.am
index 3c06e2619a..8add2f5d61 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -109,3 +109,5 @@ gen-AUTHORS:
mv -f $(distdir)/AUTHORS-tmp $(distdir)/AUTHORS && \
rm -f all.list maint.list contrib.list; \
fi
+
+include Makefile.ci
diff --git a/Makefile.ci b/Makefile.ci
new file mode 100644
index 0000000000..5448fedd97
--- /dev/null
+++ b/Makefile.ci
@@ -0,0 +1,206 @@
+# -*- makefile -*-
+# vim: filetype=make
+
+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
+
+TOP = $(shell git rev-parse --show-toplevel)
+
+# The directory holding the clo%%%%ne of the git repo that
+# we will expose to the container
+HOST_SRCDIR = $(SCRATCHDIR)/src
+
+# The directory holding the source inside the
+# container. ie where we told docker to expose
+# the $GIT/ci-tree directory from the host
+CONT_SRCDIR = /src
+
+# 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
+
+# The directory holding the build output inside the
+# container.
+CONT_BUILDDIR = $(CONT_SRCDIR)/$(CONT_VPATH)
+
+# Can be overridden with mingw{32,64}-configure if desired
+CONFIGURE = $(CONT_SRCDIR)/configure
+
+# Default to using all possible CPUs
+SMP = $(shell getconf _NPROCESSORS_ONLN)
+
+# Any extra arguments to pass to make
+MAKE_ARGS =
+
+# Any extra arguments to pass to configure
+CONFIGURE_ARGS =
+
+# Avoid pulling submodules over the network by locally
+# cloning them
+SUBMODULES = .gnulib src/keycodemapdb
+
+IMAGE_PREFIX = quay.io/libvirt/buildenv-
+IMAGE_TAG = :master
+
+# We delete the virtual root after completion, set
+# to 0 if you need to keep it around for debugging
+CLEAN = 1
+
+# We'll always freshly clone the virtual root each
+# time in case it was not cleaned up before. Set
+# to 1 if you want to try restarting a previously
+# preserved env
+REUSE = 0
+
+# We need the container process to run with current host IDs
+# so that it can access the passed in build directory
+UID = $(shell id -u)
+GID = $(shell id -g)
+
+# 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.
+#
+# We do not directly mount /etc/{passwd,group} as Docker
+# is liable to mess with SELinux labelling which will
+# then prevent the host accessing them. Copying them
+# first is safer.
+PWDB_MOUNTS = \
+ --volume $(SCRATCHDIR)/group:/etc/group:ro,z \
+ --volume $(SCRATCHDIR)/passwd:/etc/passwd:ro,z \
+ $(NULL)
+
+# Docker containers can have very large ulimits
+# for nofiles - as much as 1048576. This makes
+# libvirt very slow at exec'ing programs.
+ULIMIT_FILES = 1024
+
+# Args to use when cloning a git repo.
+# -c stop it complaining about checking out a random hash
+# -q stop it displaying progress info for local clone
+# --local ensure we don't actually copy files
+GIT_ARGS = \
+ -c advice.detachedHead=false \
+ -q \
+ --local \
+ $(NULL)
+
+# 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
+DOCKER_ARGS = \
+ --rm \
+ --user $(UID):$(GID) \
+ --interactive \
+ --tty \
+ $(PWDB_MOUNTS) \
+ --volume $(HOST_SRCDIR):$(CONT_SRCDIR):z \
+ --workdir $(CONT_SRCDIR) \
+ --ulimit nofile=$(ULIMIT_FILES):$(ULIMIT_FILES) \
+ $(NULL)
+
+check-docker:
+ @echo -n "Checking if Docker is available and running..." && \
+ docker version 1>/dev/null && echo "yes"
+
+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; \
+ echo "Cloning $(TOP) to $(HOST_SRCDIR)"; \
+ git clone $(GIT_ARGS) $(TOP) $(HOST_SRCDIR) || exit 1; \
+ 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 ; \
+ else \
+ test "$(CLEAN)" = "1" && rm -rf ci-tree || : ; \
+ fi
+
+# $CONFIGURE_OPTS is a env that can optionally be set in the container,
+# populated at build time from the Dockerfile. A typical use case would
+# be to pass --host/--target args to trigger cross-compilation
+#
+# This can be augmented by make local args in $(CONFIGURE_ARGS)
+ci-build@%: check-docker prepare-tree
+ docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)$*$(IMAGE_TAG) \
+ /bin/bash -c '\
+ mkdir -p $(CONT_BUILDDIR) || exit 1 ; \
+ cd $(CONT_BUILDDIR) ; \
+ NOCONFIGURE=1 $(CONT_SRCDIR)/autogen.sh || exit 1 ; \
+ $(CONFIGURE) $${CONFIGURE_OPTS} $(CONFIGURE_ARGS) ; \
+ if test $$? != 0 ; \
+ then \
+ test -f config.log && cat config.log ; \
+ exit 1 ; \
+ fi; \
+ find -name test-suite.log -delete ; \
+ make -j$(SMP) $(MAKE_ARGS) ; \
+ if test $$? != 0 ; then \
+ LOGS=`find -name test-suite.log` ; \
+ if test "$${LOGS}" != "" ; then \
+ echo "=== LOG FILE(S) START ===" ; \
+ cat $${LOGS} ; \
+ echo "=== LOG FILE(S) END ===" ; \
+ fi ; \
+ exit 1 ;\
+ fi'
+ @test "$(CLEAN)" = "1" && rm -rf ci-tree || :
+
+ci-check@%:
+ $(MAKE) -f $(THIS_FILE) ci-build@$* MAKE_ARGS="check gl_public_submodule_commit="
+
+ci-shell@%: prepare-tree
+ docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)$*$(IMAGE_TAG) /bin/bash
+ @test "$(CLEAN)" = "1" && rm -rf ci-tree || :
+
+ci-help:
+ @echo "Build libvirt inside docker containers used for CI"
+ @echo
+ @echo "Available targets:"
+ @echo
+ @echo " ci-build@\$$IMAGE - run a default 'make'"
+ @echo " ci-check@\$$IMAGE - run a 'make check'"
+ @echo " ci-shell@\$$IMAGE - run an interactive shell"
+ @echo
+ @echo "Available x86 container images:"
+ @echo
+ @echo " centos-7"
+ @echo " debian-8"
+ @echo " debian-9"
+ @echo " debian-sid"
+ @echo " fedora-28"
+ @echo " fedora-29"
+ @echo " fedora-rawhide"
+ @echo " ubuntu-16"
+ @echo " ubuntu-18"
+ @echo
+ @echo "Available make variables:"
+ @echo
+ @echo " CLEAN=0 - do not delete '$(SCRATCHDIR)' after completion"
+ @echo " REUSE=1 - re-use existing '$(SCRATCHDIR)' content"
+ @echo
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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: > > 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. 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. > > + 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: 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. > > $ 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; \ IMHO, build rules must never touch the user's GIT repository state, so I don't want to run a submodule update. > 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: Yeah, left over cruft from earlier version > [...] > > +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. > > + mkdir -p $(CONT_BUILDDIR) || exit 1 ; \ > > + cd $(CONT_BUILDDIR) ; \ > > This should also have > > || exit 1 > > just in case. That's impossible to reach, since we exit the line above if mkdir fails. > > + 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. Oh, yes, that's nice 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Apr 01, 2019 at 01:03:21PM +0200, Andrea Bolognani wrote: > 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. I really disagree. I want the commit messages to be more verbose so that details of what's being fixed are clearly visible. If you don't care about this detail fine, just ignore it, but that's not a reason to purge cut useful info out of the commit messages. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2019-04-03 at 11:44 +0100, Daniel P. Berrangé wrote: > On Mon, Apr 01, 2019 at 01:03:21PM +0200, Andrea Bolognani wrote: > > 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. > > I really disagree. I want the commit messages to be more verbose so > that details of what's being fixed are clearly visible. If you don't > care about this detail fine, just ignore it, but that's not a reason > to purge cut useful info out of the commit messages. You're not really fixing anything with this commit, you're introducing a new feature and presenting its use and motivation behind it - up until the point where you spend a dozen lines to explain in detail why a certain implementation details is needed. This rationale should be in the code rather than in the commit message, so that the next person wondering what the gl_foo thingy is there for will be able to learn that without having to dig through the git history. At the same time, someone scrolling through the git history might be interested in the new cool feature you're adding, but will most likely not care about the implementation detail. It's also worth noting that you didn't include this detail in the commit messages for the first two revisions, so clearly you didn't deem it as critical until I asked about it during review ;) Anyway, if after reading the above you still want to keep it in that's fine: I think both my time and yours are better used on something else than arguing further about this :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.