Message ID | 20160527135437.20474-5-famz@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> 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) >
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
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
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
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
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 --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)