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 containr:

   make cibuild-fedora-28

To also run unit tests

   make cicheck-fedora-28

This is just syntax sugar for calling the previous command with a
custom make target

   make cibuild-fedora-28 MAKE_ARGS="check"

To do a purely interactive build it is possible to request a shell

   make cishell-fedora-28

To do a mingw build, it is currently possible to use the fedora-rawhide
and request a different configure script

   make cibuild-fedora-rawhide CONFIGURE=mingw32-configure

In all cases the GIT source tree is cloned locally into a 'citree/src'
sub-directory which is then exposed to the container at '/build'. It is
setup to facilitate VPATH build so the initial working directory
is '/build/vpath'. An in source tree build can be requested instead
by passing  VPATHDIR= SRCDIR=.  args 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.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 .gitignore            |   1 +
 Makefile.am           |   2 +
 tests/Makefile.ci.inc | 174 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 tests/Makefile.ci.inc

diff --git a/.gitignore b/.gitignore
index 3303eed411..a30882d72b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -46,6 +46,7 @@
 /autom4te.cache
 /build-aux/*
 /build/
+/citree/
 /confdefs.h
 /config.cache
 /config.guess
diff --git a/Makefile.am b/Makefile.am
index 709064c6a6..0c8ab13ebc 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -123,3 +123,5 @@ gen-AUTHORS:
 	  mv -f $(distdir)/AUTHORS-tmp $(distdir)/AUTHORS && \
 	  rm -f all.list maint.list contrib.list; \
 	fi
+
+include tests/Makefile.ci.inc
diff --git a/tests/Makefile.ci.inc b/tests/Makefile.ci.inc
new file mode 100644
index 0000000000..7d521326cc
--- /dev/null
+++ b/tests/Makefile.ci.inc
@@ -0,0 +1,174 @@
+# -*- makefile -*-
+
+HERE = $(shell pwd)
+TOP = $(shell git rev-parse --show-toplevel)
+
+# 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 = ..
+
+# Can be overridden with mingw{32,64}-configure if desired
+CONFIGURE = $(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 0 if you want to try restarting a previously
+# preserved env
+RECLONE = 1
+
+# 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
+PWDB_MOUNTS = \
+	--volume $(HERE)/citree/group:/etc/group:ro,z \
+	--volume $(HERE)/citree/passwd:/etc/passwd:ro,z
+
+# 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
+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 $(HERE)/citree/src:/build:z \
+	--workdir /build/$(VPATHDIR) \
+	--ulimit nofile=$(ULIMIT_FILES):$(ULIMIT_FILES) \
+	$(NULL)
+
+checkdocker:
+	@echo -n "Checking if docker is available and running..." && \
+	docker version 1>/dev/null && echo "yes"
+
+preparetree:
+	@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; \
+		echo "Cloning $(TOP) to $(HERE)/citree/root"; \
+		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"; \
+				git clone $(GIT_ARGS) $(TOP)/$$mod $(HERE)/citree/src/$$mod || exit 1; \
+			fi ; \
+		done ; \
+		mkdir -p citree/src/$(VPATHDIR) ; \
+	else \
+		test "$(CLEAN)" = "1" && rm -rf citree || : ; \
+	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)
+cibuild-%: checkdocker preparetree
+	docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)-$*$(IMAGE_TAG) \
+		/bin/bash -c '\
+		NOCONFIGURE=1 $(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 citree || :
+
+cicheck-%:
+	$(MAKE) cibuild-$* MAKE_ARGS="check gl_public_submodule_commit="
+
+cishell-%: preparetree
+	docker run $(DOCKER_ARGS) $(IMAGE_PREFIX)-$*$(IMAGE_TAG) /bin/bash
+	@test "$(CLEAN)" = "1" && rm -rf citree || :
+
+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"
+	@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 '$(HERE)/citree' after completion"
+	@echo "    RECLONE=0 - re-use existing '$(HERE)/citree' content"
+	@echo
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, 2019-03-15 at 15:06 +0000, Daniel P. Berrangé wrote:
> On Fri, Mar 15, 2019 at 01:24:20PM +0100, Andrea Bolognani wrote:
> > [...]
> > > +# 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

I'm aware of the conversation happening in QEMU land, and in fact
that's at least in part what caused the question :)

But you're right, as long as we support both in libvirt we should
also provide a way to pick one or the other when (CI) building.

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

I have mentioned some potential issues later in the mail, but along
with those I have also provided solutions so I think we should be
good :)

> > > +# 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 :-)

Ouch :(

Perhaps we could add a very short comment mentioning the reason for
making a copy of the files, for the benefit of whoever will touch
the script months down the line.

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

Alright.

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

I'd really prefer a name that removes the ambiguity.

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

Hm, can you double check please? As mentioned above, I tried both
with and without the make variable being set, and in both cases
gnulib's 'make check' passed.

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

I disagree on concise: they're definitely shorter, but that's
because all the information is squished together, which makes it
harder to parse at a glance.

When naming Docker images we don't have much of a choice, because
we have pretty much the same restrictions as when naming files, but
that's not the case here so we could do better...

With that said, if you feel strongly about the current names I won't
stand in the way :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, 2019-03-15 at 17:20 +0100, Andrea Bolognani wrote:
> On Fri, 2019-03-15 at 15:06 +0000, Daniel P. Berrangé wrote:
> > > > +	@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
> 
> I disagree on concise: they're definitely shorter, but that's
> because all the information is squished together, which makes it
> harder to parse at a glance.
> 
> When naming Docker images we don't have much of a choice, because
> we have pretty much the same restrictions as when naming files, but
> that's not the case here so we could do better...

I see QEMU uses

  $ make docker
  ...
  docker-TEST@IMAGE:   Run "TEST" in container "IMAGE".
                       Note: "TEST" is one of the listed test name,
                       or a script name under $QEMU_SRC/tests/docker/;
                       "IMAGE" is one of the listed container name."

I think adopting that convention, thus ending up with

  $ make ci-build@centos-7 MAKE_ARGS=check
  $ make ci-build@debian-9-cross-aarch64

would be a reasonable compromise between your approach and mine.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list