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

On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote: Throughout the commit message, s/docker/Docker/g [...] s/mingw/MinGW/ s/and/image and/ [...] 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. 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. [...] Since you don't use $(HERE) anywhere else, you could do SCRATCHDIR = $(shell pwd)/ci-tree here. [...] Not sure what happened with that comment ;) [...] Again for all messages displayed to the user and comments, s/docker/Docker/g This is actually $(HOST_SRCDIR). [...] 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 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. [...] 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). [...] 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. 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. [...] There's nothing bash-specific in this script AFAICT, so you can use /bin/sh instead. This should also have || exit 1 just in case. [...] 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 :) 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. [...] This should depend on check-docker too. [...] Not anymore! Not anymore! We should figure out a way to fetch this dynamically. We can do that later, though. -- Andrea Bolognani / Red Hat / Virtualization