Re: [PATCH v3 2/6] tests: add targets for building libvirt inside docker containers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux