diff mbox

[v6,04/15] Makefile: Rules for docker testing

Message ID 20160527135437.20474-5-famz@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fam Zheng May 27, 2016, 1:54 p.m. UTC
This adds a group of make targets to run docker tests, all are available
in source tree without running ./configure.

The usage is shown with "make docker".

Besides the fixed ones, dynamic targets for building each image and
running each test in each image are generated automatically by make,
scanning $(SRC_PATH)/tests/docker/ files with specific patterns.

Alternative to manually list particular targets (docker-TEST@IMAGE)
set, you can control which tests/images to run by filtering variables,
TESTS= and IMAGES=, which are expressed in Makefile pattern syntax,
"foo% %bar ...". For example:

    $ make docker-test IMAGES="ubuntu fedora"

Unfortunately, it's impossible to propagate "-j $JOBS" into make in
containers, however since each combination is made a first class target
in the top Makefile, "make -j$N docker-test" still parallels the tests
coarsely.

Still, $J is made a magic variable to let all make invocations in
containers to use -j$J.

Instead of providing a live version of the source tree to the docker
container we snapshot it with git-archive. This ensures the tree is in a
pristine state for whatever operations the container is going to run on
them.

Uncommitted changes known to files known by the git index will be
included in the snapshot if there are any.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 Makefile                      |   4 +-
 tests/docker/Makefile.include | 121 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+), 1 deletion(-)
 create mode 100644 tests/docker/Makefile.include

Comments

Paolo Bonzini May 31, 2016, 8:51 a.m. UTC | #1
> 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)
>
Fam Zheng May 31, 2016, 11 a.m. UTC | #2
On Tue, 05/31 10:51, Paolo Bonzini wrote:
> > 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?

This is for quiet-command, which is only conditionally included by 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

I prefer we support running from the src tree without running configure, but
$(MAKE) invocations doesn't propagate make variables such as SRC_PATH...

> 
> - add to the toplevel Makefile a rule like
> 
> 	docker docker-%:
> 		$(MAKE) -C tests/docker $@

... and explicitly passing it (and $(V), etc.) here seems very ad-hocery.

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

Maybe I should make patch 3 a patch to make top Makefile include rules.mak
unconditionally?

Fam
Paolo Bonzini May 31, 2016, 12:02 p.m. UTC | #3
On 31/05/2016 13:00, Fam Zheng wrote:
> On Tue, 05/31 10:51, Paolo Bonzini wrote:
>>> 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?
> 
> This is for quiet-command, which is only conditionally included by top
> Makefile.

Ah, it's for the case when configure has not been executed yet and the
toplevel Makefile "assumes we are in the search tree".

>>
>> 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
> 
> I prefer we support running from the src tree without running configure, but
> $(MAKE) invocations doesn't propagate make variables such as SRC_PATH...
> 
>>
>> - add to the toplevel Makefile a rule like
>>
>> 	docker docker-%:
>> 		$(MAKE) -C tests/docker $@
> 
> ... and explicitly passing it (and $(V), etc.) here seems very ad-hocery.

V and others would be passed down to the recursive make.  Only SRC_PATH
would not be passed down.

>>
>> I prefer the latter.  Either would make patch 3 unnecessary.
> 
> Maybe I should make patch 3 a patch to make top Makefile include rules.mak
> unconditionally?

Yeah, that would be good.

I'm still a bit undecided about the pollution introduced by
tests/docker/Makefile.include, but I guess that's okay.

By the way, could you prepare a patch to rename tests/Makefile to
tests/Makefile.include?  It's a good convention.

Thanks,

Paolo
Fam Zheng May 31, 2016, 12:40 p.m. UTC | #4
On Tue, 05/31 14:02, Paolo Bonzini wrote:
> 
> 
> On 31/05/2016 13:00, Fam Zheng wrote:
> > On Tue, 05/31 10:51, Paolo Bonzini wrote:
> >>> 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?
> > 
> > This is for quiet-command, which is only conditionally included by top
> > Makefile.
> 
> Ah, it's for the case when configure has not been executed yet and the
> toplevel Makefile "assumes we are in the search tree".
> 
> >>
> >> 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
> > 
> > I prefer we support running from the src tree without running configure, but
> > $(MAKE) invocations doesn't propagate make variables such as SRC_PATH...
> > 
> >>
> >> - add to the toplevel Makefile a rule like
> >>
> >> 	docker docker-%:
> >> 		$(MAKE) -C tests/docker $@
> > 
> > ... and explicitly passing it (and $(V), etc.) here seems very ad-hocery.
> 
> V and others would be passed down to the recursive make.  Only SRC_PATH
> would not be passed down.

Yes, you are right. So it certainly will work, I just preferred recursive
include over resursive make for consistency (with tests/Makefile).

> 
> >>
> >> I prefer the latter.  Either would make patch 3 unnecessary.
> > 
> > Maybe I should make patch 3 a patch to make top Makefile include rules.mak
> > unconditionally?
> 
> Yeah, that would be good.
> 
> I'm still a bit undecided about the pollution introduced by
> tests/docker/Makefile.include, but I guess that's okay.

I think it's also okay to switch to "make -C tests/docker" for docker targets
(so "make docker" becomes "make -C tests/docker help"), this way the top
Makefile is not touched.

> 
> By the way, could you prepare a patch to rename tests/Makefile to
> tests/Makefile.include?  It's a good convention.

Sounds good, will do.

Fam
Paolo Bonzini May 31, 2016, 12:48 p.m. UTC | #5
On 31/05/2016 14:40, Fam Zheng wrote:
> > I'm still a bit undecided about the pollution introduced by
> > tests/docker/Makefile.include, but I guess that's okay.
> 
> I think it's also okay to switch to "make -C tests/docker" for docker targets
> (so "make docker" becomes "make -C tests/docker help"), this way the top
> Makefile is not touched.

That would be a bit harder to discover.  Go ahead with your current
solution.  Perhaps rename SRC_COPY to DOCKER_SRC_COPY?

Thanks,

Paolo
Fam Zheng June 1, 2016, 2:26 a.m. UTC | #6
On Tue, 05/31 14:48, Paolo Bonzini wrote:
> 
> 
> On 31/05/2016 14:40, Fam Zheng wrote:
> > > I'm still a bit undecided about the pollution introduced by
> > > tests/docker/Makefile.include, but I guess that's okay.
> > 
> > I think it's also okay to switch to "make -C tests/docker" for docker targets
> > (so "make docker" becomes "make -C tests/docker help"), this way the top
> > Makefile is not touched.
> 
> That would be a bit harder to discover.  Go ahead with your current
> solution.  Perhaps rename SRC_COPY to DOCKER_SRC_COPY?

OK, good idea.

Fam
diff mbox

Patch

diff --git a/Makefile b/Makefile
index a5d7e62..3db4fa0 100644
--- a/Makefile
+++ b/Makefile
@@ -6,7 +6,7 @@  BUILD_DIR=$(CURDIR)
 # Before including a proper config-host.mak, assume we are in the source tree
 SRC_PATH=.
 
-UNCHECKED_GOALS := %clean TAGS cscope ctags
+UNCHECKED_GOALS := %clean TAGS cscope ctags docker docker-%
 
 # All following code might depend on configuration variables
 ifneq ($(wildcard config-host.mak),)
@@ -652,3 +652,5 @@  endif
 # Include automatically generated dependency files
 # Dependencies in Makefile.objs files come from our recursive subdir rules
 -include $(wildcard *.d tests/*.d)
+
+include $(SRC_PATH)/tests/docker/Makefile.include
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
+
+.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)
+
+# Make archive from git repo $1 to tar.gz $2
+make-archive-maybe = $(if $(wildcard $1/*), \
+	$(call quiet-command, \
+		(cd $1; if git diff-index --quiet HEAD -- &>/dev/null; then \
+			git archive -1 HEAD --format=tar.gz -o $2; \
+		else \
+			git archive -1 $$(git stash create) --format=tar.gz -o $2; \
+		fi), \
+		"  ARCHIVE $(notdir $2)"))
+
+$(SRC_COPY):
+	@mkdir -p $@
+	$(call make-archive-maybe, $(SRC_PATH), $(SRC_COPY)/qemu.tgz)
+	$(call make-archive-maybe, $(SRC_PATH)/dtc, $(SRC_COPY)/dtc.tgz)
+	$(call make-archive-maybe, $(SRC_PATH)/pixman, $(SRC_COPY)/pixman.tgz)
+	$(call quiet-command, cp "$(SRC_PATH)/tests/docker/run" "$(SRC_COPY)/run", \
+		"  COPY RUNNER")
+
+docker-qemu-src: $(SRC_COPY)
+
+docker-image: ${DOCKER_TARGETS}
+
+# General rule for building docker images
+docker-image-%: $(DOCKER_FILES_DIR)/%.docker
+	$(call quiet-command,\
+		$(SRC_PATH)/tests/docker/docker.py build qemu:$* $< \
+		$(if $V,,--quiet) $(if $(NOCACHE),--no-cache),\
+		"  BUILD $*")
+
+# Expand all the pre-requistes for each docker image and test combination
+$(foreach i,$(DOCKER_IMAGES), \
+	$(foreach t,$(DOCKER_TESTS) $(DOCKER_TOOLS), \
+		$(eval .PHONY: docker-$t@$i) \
+		$(eval docker-$t@$i: docker-image-$i docker-run-$t@$i) \
+	) \
+	$(foreach t,$(DOCKER_TESTS), \
+		$(eval docker-test: docker-$t@$i) \
+	) \
+)
+
+docker:
+	@echo 'Build QEMU and run tests inside Docker containers'
+	@echo
+	@echo 'Available targets:'
+	@echo
+	@echo '    docker:              Print this help.'
+	@echo '    docker-test:         Run all image/test combinations.'
+	@echo '    docker-clean:        Kill and remove residual docker testing containers.'
+	@echo '    docker-TEST@IMAGE:   Run "TEST" in container "IMAGE".'
+	@echo '                         Note: "TEST" is one of the listed test name,'
+	@echo '                         or a script name under $$QEMU_SRC/tests/docker/;'
+	@echo '                         "IMAGE" is one of the listed container name."'
+	@echo '    docker-image:        Build all images.'
+	@echo '    docker-image-IMAGE:  Build image "IMAGE".'
+	@echo
+	@echo 'Available container images:'
+	@echo '    $(DOCKER_IMAGES)'
+	@echo
+	@echo 'Available tests:'
+	@echo '    $(DOCKER_TESTS)'
+	@echo
+	@echo 'Available tools:'
+	@echo '    $(DOCKER_TOOLS)'
+	@echo
+	@echo 'Special variables:'
+	@echo '    TARGET_LIST=a,b,c    Override target list in builds.'
+	@echo '    IMAGES="a b c ..":   Filters which images to build or run.'
+	@echo '    TESTS="x y z .."     Filters which tests to run (for docker-test).'
+	@echo '    J=[0..9]*            Overrides the -jN parameter for make commands'
+	@echo '                         (default is 1)'
+	@echo '    DEBUG=1              Stop and drop to shell in the created container'
+	@echo '                         before running the command.'
+	@echo '    NOCACHE=1            Ignore cache when build images.'
+
+docker-run-%: CMD = $(shell echo '$@' | sed -e 's/docker-run-\([^@]*\)@\(.*\)/\1/')
+docker-run-%: IMAGE = $(shell echo '$@' | sed -e 's/docker-run-\([^@]*\)@\(.*\)/\2/')
+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)