Re: [Qemu-devel] [PATCH v6 04/15] Makefile: Rules for docker testing

[Paolo Bonzini]

> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> new file mode 100644
> index 0000000..372733d
> --- /dev/null
> +++ b/tests/docker/Makefile.include
> @@ -0,0 +1,121 @@
> +# Makefile for Docker tests
> +
> +include $(SRC_PATH)/rules.mak

Why include this _and_ include tests/docker/Makefile.include from the
top Makefile?

I think you should do one of this:

a) drop this inclusion; nice, but it pollutes the toplevel makefile a bit

b) do the following:

- link this file into the build tree in configure

- include ../../config-host.mak

- add to the toplevel Makefile a rule like

        docker docker-%:
                $(MAKE) -C tests/docker $@

I prefer the latter.  Either would make patch 3 unnecessary.

> +.PHONY: docker docker-test docker-clean docker-image docker-qemu-src
> +
> +DOCKER_SUFFIX := .docker
> +DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles
> +DOCKER_IMAGES := $(notdir $(basename $(wildcard 
> $(DOCKER_FILES_DIR)/*.docker)))
> +DOCKER_TARGETS := $(patsubst %,docker-image-%,$(DOCKER_IMAGES))
> +# Use a global constant ccache directory to speed up repetitive builds
> +DOCKER_CCACHE_DIR := /var/tmp/qemu-docker-ccache
> +
> +DOCKER_TESTS := $(notdir $(shell \
> +     find $(SRC_PATH)/tests/docker/ -name 'test-*' -type f))
> +
> +DOCKER_TOOLS := travis
> +
> +TESTS ?= %
> +IMAGES ?= %
> +SRC_COPY := $(shell mktemp -u /tmp/qemu-src.XXXXX)

This is unsafe, especially if you place it in /tmp.  Something like this
should work better:

...
# do not use -p here, so that a conflict causes the build to fail
qemu-src.%:
        @mkdir $@
        $(call make-archive-maybe, $(SRC_PATH), $@/qemu.tgz)
        $(call make-archive-maybe, $(SRC_PATH)/dtc, $@/dtc.tgz)
        $(call make-archive-maybe, $(SRC_PATH)/pixman, $@/pixman.tgz)
        $(call quiet-command, cp $(SRC_PATH)/tests/docker/run $@/run, \
                "  COPY RUNNER")

CUR_TIME = $(shell date +%Y-%m-%d-%H.%M.%S)

# Makes the definition constant after the first expansion
SRC_COPY = $(eval SRC_COPY := qemu-src.$(CUR_TIME))$(SRC_COPY)

# Perhaps creating a symlink to the latest src tree can be useful?
docker-qemu-src:
        $(MAKE) $(SRC_COPY)
        ln -sf $(SRC_COPY) qemu-src

Thanks,

Paolo

> +docker-run-%: docker-qemu-src
> +     @if test -z "$(IMAGE)" || test -z "$(CMD)"; \
> +             then echo "Invalid target"; exit 1; \
> +     fi
> +     $(if $(filter $(TESTS),$(CMD)),$(if $(filter $(IMAGES),$(IMAGE)), \
> +             $(call quiet-command,\
> +                     $(SRC_PATH)/tests/docker/docker.py run $(if $V,,--rm) \
> +                             -t \
> +                             $(if $(DEBUG),-i,--net=none) \
> +                             -e TARGET_LIST=$(TARGET_LIST) \
> +                             -e V=$V -e J=$J -e DEBUG=$(DEBUG)\
> +                             -e CCACHE_DIR=/var/tmp/ccache \
> +                             -v $$(realpath 
> $(SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \
> +                             -v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z \
> +                             -w /var/tmp/qemu \
> +                             qemu:$(IMAGE) \
> +                             $(if $V,/bin/bash -x ,) \
> +                             ./run \
> +                             $(CMD); \
> +                     , "  RUN $(CMD) in $(IMAGE)")))
> +
> +docker-clean:
> +     $(call quiet-command, $(SRC_PATH)/tests/docker/docker.py clean)
>