[Qemu-devel] [PATCH for 3.0 00/10] various docker fixes

[Qemu-devel] [PATCH for 3.0 00/10] various docker fixes

[Alex Bennée]

Hi,

The addition of the cross compilers for check-tcg broke the ability to
run "make docker-test".  In truth this was probably already broken as
it attempts to run every single test with every single docker image
which isn't something that gets done on a regular basis.

Anyway the patches clean up the ability to do that in a sane way
although it still takes a long time to run the full test set.

To help with running a better subset I've expanded the individual
tests so you can now run a line like:

  make docker-test-build TARGET_LIST=aarch64-softmmu J=30

To make sure your favourite architecture still builds everywhere.

There is now also a docker-test-unit which just runs the unit tests
although we have to do a little re-factoring to make sure we don't
attempt to run "make check" steps when the docker image isn't capable
of it. This is also needed to make sure the other test-FOO build tests
don't choke on the check step.

Finally there is a minor tweak for .gitignore and a fix for docker.py
throwing backtraces when we attempt to calculate SID_AGE. A cleaner
re-factoring can be left for a future release.

Alex Bennée (10):
  tests/.gitignore: don't ignore docker tests
  docker: debian-tricore add git tools
  docker: fail more gracefully on docker.py check
  docker: split configure_qemu from build_qemu
  docker: move make check into check_qemu helper
  docker: gracefully skip check_qemu
  docker: Makefile.include don't include partial images
  docker: disable debian-powerpc-user-cross
  docker: add test-unit runner
  docker: add expansion for docker-test-FOO to Makefile.include

 tests/.gitignore                              |  1 +
 tests/docker/Makefile.include                 | 14 +++++++++--
 tests/docker/common.rc                        | 23 ++++++++++++++++++-
 tests/docker/docker.py                        |  7 +++++-
 .../dockerfiles/debian-tricore-cross.docker   |  3 +++
 tests/docker/test-clang                       |  2 +-
 tests/docker/test-debug                       |  2 +-
 tests/docker/test-full                        |  2 +-
 tests/docker/test-quick                       |  2 +-
 tests/docker/test-unit                        | 19 +++++++++++++++
 10 files changed, 67 insertions(+), 8 deletions(-)
 create mode 100755 tests/docker/test-unit

-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 01/10] tests/.gitignore: don't ignore docker tests

[Alex Bennée]

This was being a little over enthusiastic hiding files.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/.gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/.gitignore b/tests/.gitignore
index 08e2df1ce1..72c18aaab0 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -9,6 +9,7 @@ qht-bench
 rcutorture
 test-*
 !test-*.c
+!docker/test-*
 test-qapi-commands.[ch]
 test-qapi-events.[ch]
 test-qapi-types.[ch]
-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 02/10] docker: debian-tricore add git tools

[Alex Bennée]

As we check out we need git installed before we start. We would have
had this based on the heavier qemu:debian9 image but we only use plain
debian:9 here.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/dockerfiles/debian-tricore-cross.docker | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/docker/dockerfiles/debian-tricore-cross.docker b/tests/docker/dockerfiles/debian-tricore-cross.docker
index 898b8dd511..5ba3ca88a7 100644
--- a/tests/docker/dockerfiles/debian-tricore-cross.docker
+++ b/tests/docker/dockerfiles/debian-tricore-cross.docker
@@ -11,6 +11,9 @@ FROM debian:9
 
 MAINTAINER Philippe Mathieu-Daudé <f4bug@amsat.org>
 
+RUN apt-get update && \
+    DEBIAN_FRONTEND=noninteractive apt-get install -yy git
+
 RUN git clone --single-branch \
         https://github.com/bkoppelmann/tricore-binutils.git \
         /usr/src/binutils && \
-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 03/10] docker: fail more gracefully on docker.py check

[Alex Bennée]

As this is called directly from the Makefile while determining
dependencies and it is possible the user was configured in one window
but not have credentials in the other. Let's catch the Exceptions and
deal with it quietly.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/docker/docker.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 69e7130db7..2f81c6b13b 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -479,7 +479,12 @@ class CheckCommand(SubCommand):
     def run(self, args, argv):
         tag = args.tag
 
-        dkr = Docker()
+        try:
+            dkr = Docker()
+        except:
+            print("Docker not set up")
+            return 1
+
         info = dkr.inspect_tag(tag)
         if info is None:
             print("Image does not exist")
-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 04/10] docker: split configure_qemu from build_qemu

[Alex Bennée]

This allows some tests that just want to configure QEMU's source tree
to do so.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/common.rc | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/docker/common.rc b/tests/docker/common.rc
index 046f8a5921..ba1f942328 100755
--- a/tests/docker/common.rc
+++ b/tests/docker/common.rc
@@ -21,7 +21,7 @@ requires()
     done
 }
 
-build_qemu()
+configure_qemu()
 {
     config_opts="--enable-werror \
                  ${TARGET_LIST:+--target-list=${TARGET_LIST}} \
@@ -32,6 +32,11 @@ build_qemu()
     echo $config_opts
     $QEMU_SRC/configure $config_opts || \
         { cat config.log && test_fail "Failed to run 'configure'"; }
+}
+
+build_qemu()
+{
+    configure_qemu $@
     make $MAKEFLAGS
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 05/10] docker: move make check into check_qemu helper

[Alex Bennée]

Not all docker images can run the check step. Let's everything into a
common helper so we don't need to replicate checks in the future.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/common.rc  | 10 ++++++++++
 tests/docker/test-clang |  2 +-
 tests/docker/test-debug |  2 +-
 tests/docker/test-full  |  2 +-
 tests/docker/test-quick |  2 +-
 5 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tests/docker/common.rc b/tests/docker/common.rc
index ba1f942328..cfc620d554 100755
--- a/tests/docker/common.rc
+++ b/tests/docker/common.rc
@@ -40,6 +40,16 @@ build_qemu()
     make $MAKEFLAGS
 }
 
+check_qemu()
+{
+    if test -z "$@"; then
+        TEST="check"
+    else
+        TEST="$@"
+    fi
+    make $MAKEFLAGS $TEST
+}
+
 test_fail()
 {
     echo "$@"
diff --git a/tests/docker/test-clang b/tests/docker/test-clang
index e90a793178..324e341cea 100755
--- a/tests/docker/test-clang
+++ b/tests/docker/test-clang
@@ -23,5 +23,5 @@ OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
 #OPTS="$OPTS --extra-cflags=-fsanitize=undefined \
     #--extra-cflags=-fno-sanitize=float-divide-by-zero"
 build_qemu $OPTS
-make $MAKEFLAGS check
+check_qemu
 install_qemu
diff --git a/tests/docker/test-debug b/tests/docker/test-debug
index d3f9f70d01..137f4f2ddc 100755
--- a/tests/docker/test-debug
+++ b/tests/docker/test-debug
@@ -22,5 +22,5 @@ OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
 OPTS="--enable-debug --enable-sanitizers $OPTS"
 
 build_qemu $OPTS
-make $MAKEFLAGS V=1 check
+check_qemu check V=1
 install_qemu
diff --git a/tests/docker/test-full b/tests/docker/test-full
index b4e42d25d7..aadc0f00a2 100755
--- a/tests/docker/test-full
+++ b/tests/docker/test-full
@@ -15,4 +15,4 @@
 
 cd "$BUILD_DIR"
 
-build_qemu && make check $MAKEFLAGS && install_qemu
+build_qemu && check_qemu && install_qemu
diff --git a/tests/docker/test-quick b/tests/docker/test-quick
index 3b7bce6105..eee59c55fb 100755
--- a/tests/docker/test-quick
+++ b/tests/docker/test-quick
@@ -18,5 +18,5 @@ cd "$BUILD_DIR"
 DEF_TARGET_LIST="x86_64-softmmu,aarch64-softmmu"
 TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
 build_qemu
-make check $MAKEFLAGS
+check_qemu
 install_qemu
-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 06/10] docker: gracefully skip check_qemu

[Alex Bennée]

Not all our images are able to run the tests. Rather than use features
we can just check for the existence and run-ability of gtester. If the
image has been setup for binfmt_misc it will be able to run anyway.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/common.rc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/docker/common.rc b/tests/docker/common.rc
index cfc620d554..6df431eb72 100755
--- a/tests/docker/common.rc
+++ b/tests/docker/common.rc
@@ -47,7 +47,13 @@ check_qemu()
     else
         TEST="$@"
     fi
-    make $MAKEFLAGS $TEST
+
+    if type gtester > /dev/null 2>&1 && \
+           gtester --version > /dev/null 2>&1; then
+        make $MAKEFLAGS $TEST
+    else
+        echo "No working gtester, skipping make $TEST"
+    fi
 }
 
 test_fail()
-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 07/10] docker: Makefile.include don't include partial images

[Alex Bennée]

Rename DOCKER_INTERMEDIATE_IMAGES to DOCKER_PARTIAL_IMAGES and add the
incomplete cross compiler images that can build tests but can't build
QEMU itself. We also add debian, debian-bootstrap and the tricode
images to the list.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/Makefile.include | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index b2a7e761cc..09fb7db7fa 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -6,7 +6,7 @@ DOCKER_SUFFIX := .docker
 DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles
 DOCKER_DEPRECATED_IMAGES := debian
 # we don't run tests on intermediate images (used as base by another image)
-DOCKER_INTERMEDIATE_IMAGES := debian8 debian9 debian8-mxe debian-ports debian-sid
+DOCKER_PARTIAL_IMAGES := debian debian8 debian9 debian8-mxe debian-ports debian-sid debian-bootstrap
 DOCKER_IMAGES := $(filter-out $(DOCKER_DEPRECATED_IMAGES),$(sort $(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
@@ -121,6 +121,11 @@ docker-image-travis: NOUSER=1
 # Specialist build images, sometimes very limited tools
 docker-image-tricore-cross: docker-image-debian9
 
+# These images may be good enough for building tests but not for test builds
+DOCKER_PARTIAL_IMAGES += debian-alpha-cross debian-hppa-cross debian-m68k-cross debian-sh4-cross
+DOCKER_PARTIAL_IMAGES += debian-sparc64-cross debian-mips64-cross debian-riscv64-cross
+DOCKER_PARTIAL_IMAGES += debian-tricore-cross debian-powerpc-cross fedora-i386-cross
+
 # Rules for building linux-user powered images
 #
 # These are slower than using native cross compiler setups but can
@@ -137,7 +142,7 @@ docker-image-debian-powerpc-user-cross: docker-binfmt-image-debian-powerpc-user
 DOCKER_USER_IMAGES += debian-powerpc-user
 
 # Expand all the pre-requistes for each docker image and test combination
-$(foreach i,$(filter-out $(DOCKER_INTERMEDIATE_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPRECATED_IMAGES)), \
+$(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPRECATED_IMAGES)), \
 	$(foreach t,$(DOCKER_TESTS) $(DOCKER_TOOLS), \
 		$(eval .PHONY: docker-$t@$i) \
 		$(eval docker-$t@$i: docker-image-$i docker-run-$t@$i) \
-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 08/10] docker: disable debian-powerpc-user-cross

[Alex Bennée]

While we can build we can't run the unit tests due to a missing system
call (249 - swapcontext). This could be a code generation issue as
other architectures don't seem to have this system call.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
---
 tests/docker/Makefile.include | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 09fb7db7fa..fe63aacf69 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -140,6 +140,9 @@ docker-binfmt-image-debian-powerpc-user: QEMU_TARGET = ppc-linux-user
 docker-binfmt-image-debian-powerpc-user: EXECUTABLE = ${BUILD_DIR}/ppc-linux-user/qemu-ppc
 docker-image-debian-powerpc-user-cross: docker-binfmt-image-debian-powerpc-user
 DOCKER_USER_IMAGES += debian-powerpc-user
+# Unfortunalty while we can build with this image we can't run unit
+# tests due to missing system call: 249 which causes an abort
+DOCKER_PARTIAL_IMAGES += debian-powerpc-user-cross
 
 # Expand all the pre-requistes for each docker image and test combination
 $(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPRECATED_IMAGES)), \
-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 09/10] docker: add test-unit runner

[Alex Bennée]

This test doesn't even build QEMU, it just runs all the unit tests.
Intended to make checking unit tests on all docker images easier.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/test-unit | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100755 tests/docker/test-unit

diff --git a/tests/docker/test-unit b/tests/docker/test-unit
new file mode 100755
index 0000000000..be0d90d748
--- /dev/null
+++ b/tests/docker/test-unit
@@ -0,0 +1,19 @@
+#!/bin/bash
+#
+# Build and run the unit tests
+#
+# Copyright (c) 2018 Linaro Ltd.
+#
+# Authors:
+#  Alex Bennée <alex.bennee@linaro.org>
+#
+# This work is licensed under the terms of the GNU GPL, version 2
+# or (at your option) any later version. See the COPYING file in
+# the top-level directory.
+
+. common.rc
+
+cd "$BUILD_DIR"
+
+configure_qemu
+check_qemu check-unit
-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 10/10] docker: add expansion for docker-test-FOO to Makefile.include

[Alex Bennée]

This allows us to run a particular test on all docker images. For
example:

  make docker-test-unit

Will run the unit tests on every supported image.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/Makefile.include | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index fe63aacf69..765b2c36f2 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -152,6 +152,7 @@ $(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPR
 	) \
 	$(foreach t,$(DOCKER_TESTS), \
 		$(eval docker-test: docker-$t@$i) \
+		$(eval docker-$t: docker-$t@$i) \
 	) \
 )
 
@@ -162,6 +163,7 @@ docker:
 	@echo
 	@echo '    docker:              Print this help.'
 	@echo '    docker-test:         Run all image/test combinations.'
+	@echo '    docker-TEST:         Run TEST on all image 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,'
-- 
2.17.1

Re: [Qemu-devel] [PATCH for 3.0 01/10] tests/.gitignore: don't ignore docker tests

[Fam Zheng]

On Mon, 07/09 16:21, Alex Bennée wrote:
> This was being a little over enthusiastic hiding files.

What is "this" that hides test-* and calls for this patch?

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/.gitignore | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 08e2df1ce1..72c18aaab0 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -9,6 +9,7 @@ qht-bench
>  rcutorture
>  test-*
>  !test-*.c
> +!docker/test-*
>  test-qapi-commands.[ch]
>  test-qapi-events.[ch]
>  test-qapi-types.[ch]
> -- 
> 2.17.1
> 

Re: [Qemu-devel] [PATCH for 3.0 05/10] docker: move make check into check_qemu helper

[Fam Zheng]

On Mon, 07/09 16:21, Alex Bennée wrote:
> Not all docker images can run the check step. Let's everything into a

"move everything"?

> common helper so we don't need to replicate checks in the future.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/docker/common.rc  | 10 ++++++++++
>  tests/docker/test-clang |  2 +-
>  tests/docker/test-debug |  2 +-
>  tests/docker/test-full  |  2 +-
>  tests/docker/test-quick |  2 +-
>  5 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/docker/common.rc b/tests/docker/common.rc
> index ba1f942328..cfc620d554 100755
> --- a/tests/docker/common.rc
> +++ b/tests/docker/common.rc
> @@ -40,6 +40,16 @@ build_qemu()
>      make $MAKEFLAGS
>  }
>  
> +check_qemu()
> +{
> +    if test -z "$@"; then
> +        TEST="check"
> +    else
> +        TEST="$@"
> +    fi
> +    make $MAKEFLAGS $TEST
> +}
> +
>  test_fail()
>  {
>      echo "$@"
> diff --git a/tests/docker/test-clang b/tests/docker/test-clang
> index e90a793178..324e341cea 100755
> --- a/tests/docker/test-clang
> +++ b/tests/docker/test-clang
> @@ -23,5 +23,5 @@ OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
>  #OPTS="$OPTS --extra-cflags=-fsanitize=undefined \
>      #--extra-cflags=-fno-sanitize=float-divide-by-zero"
>  build_qemu $OPTS
> -make $MAKEFLAGS check
> +check_qemu
>  install_qemu
> diff --git a/tests/docker/test-debug b/tests/docker/test-debug
> index d3f9f70d01..137f4f2ddc 100755
> --- a/tests/docker/test-debug
> +++ b/tests/docker/test-debug
> @@ -22,5 +22,5 @@ OPTS="--cxx=clang++ --cc=clang --host-cc=clang"
>  OPTS="--enable-debug --enable-sanitizers $OPTS"
>  
>  build_qemu $OPTS
> -make $MAKEFLAGS V=1 check
> +check_qemu check V=1
>  install_qemu
> diff --git a/tests/docker/test-full b/tests/docker/test-full
> index b4e42d25d7..aadc0f00a2 100755
> --- a/tests/docker/test-full
> +++ b/tests/docker/test-full
> @@ -15,4 +15,4 @@
>  
>  cd "$BUILD_DIR"
>  
> -build_qemu && make check $MAKEFLAGS && install_qemu
> +build_qemu && check_qemu && install_qemu
> diff --git a/tests/docker/test-quick b/tests/docker/test-quick
> index 3b7bce6105..eee59c55fb 100755
> --- a/tests/docker/test-quick
> +++ b/tests/docker/test-quick
> @@ -18,5 +18,5 @@ cd "$BUILD_DIR"
>  DEF_TARGET_LIST="x86_64-softmmu,aarch64-softmmu"
>  TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
>  build_qemu
> -make check $MAKEFLAGS
> +check_qemu
>  install_qemu
> -- 
> 2.17.1
> 

Re: [Qemu-devel] [PATCH for 3.0 09/10] docker: add test-unit runner

[Fam Zheng]

On Mon, 07/09 16:21, Alex Bennée wrote:
> This test doesn't even build QEMU, it just runs all the unit tests.
> Intended to make checking unit tests on all docker images easier.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/docker/test-unit | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100755 tests/docker/test-unit
> 
> diff --git a/tests/docker/test-unit b/tests/docker/test-unit
> new file mode 100755
> index 0000000000..be0d90d748
> --- /dev/null
> +++ b/tests/docker/test-unit
> @@ -0,0 +1,19 @@
> +#!/bin/bash
> +#
> +# Build and run the unit tests
> +#
> +# Copyright (c) 2018 Linaro Ltd.
> +#
> +# Authors:
> +#  Alex Bennée <alex.bennee@linaro.org>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2
> +# or (at your option) any later version. See the COPYING file in
> +# the top-level directory.
> +
> +. common.rc
> +
> +cd "$BUILD_DIR"
> +
> +configure_qemu

This reads a bit unusual and counter-intuitive: how could configure_qemu succeed
when the env cannot actually build it? Does configure_qemu fail but the side
effects needed to run build and run unit tests are done anyway?  Either way,
please add a comment explaining what is happening here.

Also, should we test the exit code of configure_qemu?

> +check_qemu check-unit
> -- 
> 2.17.1
> 

Re: [Qemu-devel] [PATCH for 3.0 00/10] various docker fixes

[Fam Zheng]

On Mon, 07/09 16:21, Alex Bennée wrote:
> Hi,
> 
> The addition of the cross compilers for check-tcg broke the ability to
> run "make docker-test".  In truth this was probably already broken as
> it attempts to run every single test with every single docker image
> which isn't something that gets done on a regular basis.
> 
> Anyway the patches clean up the ability to do that in a sane way
> although it still takes a long time to run the full test set.
> 
> To help with running a better subset I've expanded the individual
> tests so you can now run a line like:
> 
>   make docker-test-build TARGET_LIST=aarch64-softmmu J=30
> 
> To make sure your favourite architecture still builds everywhere.
> 
> There is now also a docker-test-unit which just runs the unit tests
> although we have to do a little re-factoring to make sure we don't
> attempt to run "make check" steps when the docker image isn't capable
> of it. This is also needed to make sure the other test-FOO build tests
> don't choke on the check step.
> 
> Finally there is a minor tweak for .gitignore and a fix for docker.py
> throwing backtraces when we attempt to calculate SID_AGE. A cleaner
> re-factoring can be left for a future release.

Thanks for the clean ups!

Looks good in general. The only thing I notice is the test-unit script.

Fam

Re: [Qemu-devel] [PATCH for 3.0 01/10] tests/.gitignore: don't ignore docker tests

[Alex Bennée]

Fam Zheng <famz@redhat.com> writes:

> On Mon, 07/09 16:21, Alex Bennée wrote:
>> This was being a little over enthusiastic hiding files.
>
> What is "this" that hides test-* and calls for this patch?

I thought that was implicit from the first line, this is the .gitignore.

>
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  tests/.gitignore | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/tests/.gitignore b/tests/.gitignore
>> index 08e2df1ce1..72c18aaab0 100644
>> --- a/tests/.gitignore
>> +++ b/tests/.gitignore
>> @@ -9,6 +9,7 @@ qht-bench
>>  rcutorture
>>  test-*
>>  !test-*.c
>> +!docker/test-*
>>  test-qapi-commands.[ch]
>>  test-qapi-events.[ch]
>>  test-qapi-types.[ch]
>> --
>> 2.17.1
>>


--
Alex Bennée

Re: [Qemu-devel] [PATCH for 3.0 01/10] tests/.gitignore: don't ignore docker tests

[Fam Zheng]

On Tue, 07/10 08:54, Alex Bennée wrote:
> 
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Mon, 07/09 16:21, Alex Bennée wrote:
> >> This was being a little over enthusiastic hiding files.
> >
> > What is "this" that hides test-* and calls for this patch?
> 
> I thought that was implicit from the first line, this is the .gitignore.

Indeed you're right. Maybe that line should be more specific, but I have no
strong opinion.

Fam

> 
> >
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  tests/.gitignore | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/tests/.gitignore b/tests/.gitignore
> >> index 08e2df1ce1..72c18aaab0 100644
> >> --- a/tests/.gitignore
> >> +++ b/tests/.gitignore
> >> @@ -9,6 +9,7 @@ qht-bench
> >>  rcutorture
> >>  test-*
> >>  !test-*.c
> >> +!docker/test-*
> >>  test-qapi-commands.[ch]
> >>  test-qapi-events.[ch]
> >>  test-qapi-types.[ch]
> >> --
> >> 2.17.1
> >>
> 
> 
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH for 3.0 02/10] docker: debian-tricore add git tools

[Philippe Mathieu-Daudé]

On 07/09/2018 12:21 PM, Alex Bennée wrote:
> As we check out we need git installed before we start. We would have
> had this based on the heavier qemu:debian9 image but we only use plain
> debian:9 here.

I first thought "weird, I checked this already" then noticed you rebased
on debian:9.
'stretch-slim' is an alias for '9-slim' and should be tinier (the
previous RFC image is 323MB).

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>> ---
>  tests/docker/dockerfiles/debian-tricore-cross.docker | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/docker/dockerfiles/debian-tricore-cross.docker b/tests/docker/dockerfiles/debian-tricore-cross.docker
> index 898b8dd511..5ba3ca88a7 100644
> --- a/tests/docker/dockerfiles/debian-tricore-cross.docker
> +++ b/tests/docker/dockerfiles/debian-tricore-cross.docker
> @@ -11,6 +11,9 @@ FROM debian:9
>  
>  MAINTAINER Philippe Mathieu-Daudé <f4bug@amsat.org>
>  
> +RUN apt-get update && \
> +    DEBIAN_FRONTEND=noninteractive apt-get install -yy git
> +
>  RUN git clone --single-branch \
>          https://github.com/bkoppelmann/tricore-binutils.git \
>          /usr/src/binutils && \
> 

Re: [Qemu-devel] [PATCH for 3.0 03/10] docker: fail more gracefully on docker.py check

[Philippe Mathieu-Daudé]

On 07/09/2018 12:21 PM, Alex Bennée wrote:
> As this is called directly from the Makefile while determining
> dependencies and it is possible the user was configured in one window
> but not have credentials in the other. Let's catch the Exceptions and
> deal with it quietly.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/docker/docker.py | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index 69e7130db7..2f81c6b13b 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -479,7 +479,12 @@ class CheckCommand(SubCommand):
>      def run(self, args, argv):
>          tag = args.tag
>  
> -        dkr = Docker()
> +        try:
> +            dkr = Docker()
> +        except:
> +            print("Docker not set up")
> +            return 1
> +
>          info = dkr.inspect_tag(tag)
>          if info is None:
>              print("Image does not exist")
> 

Re: [Qemu-devel] [PATCH for 3.0 10/10] docker: add expansion for docker-test-FOO to Makefile.include

[Philippe Mathieu-Daudé]

On 07/09/2018 12:21 PM, Alex Bennée wrote:
> This allows us to run a particular test on all docker images. For
> example:
> 
>   make docker-test-unit
> 
> Will run the unit tests on every supported image.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/docker/Makefile.include | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index fe63aacf69..765b2c36f2 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -152,6 +152,7 @@ $(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPR
>  	) \
>  	$(foreach t,$(DOCKER_TESTS), \
>  		$(eval docker-test: docker-$t@$i) \
> +		$(eval docker-$t: docker-$t@$i) \
>  	) \
>  )
>  
> @@ -162,6 +163,7 @@ docker:
>  	@echo
>  	@echo '    docker:              Print this help.'
>  	@echo '    docker-test:         Run all image/test combinations.'

Can we rename "docker-test" -> "docker-tests" or is it too late?

> +	@echo '    docker-TEST:         Run TEST on all image 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,'
> 

Re: [Qemu-devel] [PATCH for 3.0 01/10] tests/.gitignore: don't ignore docker tests

[Philippe Mathieu-Daudé]

On 07/09/2018 12:21 PM, Alex Bennée wrote:
> This was being a little over enthusiastic hiding files.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/.gitignore | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 08e2df1ce1..72c18aaab0 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -9,6 +9,7 @@ qht-bench
>  rcutorture
>  test-*
>  !test-*.c
> +!docker/test-*
>  test-qapi-commands.[ch]
>  test-qapi-events.[ch]
>  test-qapi-types.[ch]
> 

Re: [Qemu-devel] [PATCH for 3.0 10/10] docker: add expansion for docker-test-FOO to Makefile.include

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 07/09/2018 12:21 PM, Alex Bennée wrote:
>> This allows us to run a particular test on all docker images. For
>> example:
>>
>>   make docker-test-unit
>>
>> Will run the unit tests on every supported image.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  tests/docker/Makefile.include | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
>> index fe63aacf69..765b2c36f2 100644
>> --- a/tests/docker/Makefile.include
>> +++ b/tests/docker/Makefile.include
>> @@ -152,6 +152,7 @@ $(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPR
>>  	) \
>>  	$(foreach t,$(DOCKER_TESTS), \
>>  		$(eval docker-test: docker-$t@$i) \
>> +		$(eval docker-$t: docker-$t@$i) \
>>  	) \
>>  )
>>
>> @@ -162,6 +163,7 @@ docker:
>>  	@echo
>>  	@echo '    docker:              Print this help.'
>>  	@echo '    docker-test:         Run all image/test combinations.'
>
> Can we rename "docker-test" -> "docker-tests" or is it too late?

Probably, I don't know how many people were actively using it. Fam?

>
>> +	@echo '    docker-TEST:         Run TEST on all image 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,'
>>


--
Alex Bennée

Re: [Qemu-devel] [PATCH for 3.0 02/10] docker: debian-tricore add git tools

[Philippe Mathieu-Daudé]

On 07/10/2018 05:44 PM, Philippe Mathieu-Daudé wrote:
> On 07/09/2018 12:21 PM, Alex Bennée wrote:
>> As we check out we need git installed before we start. We would have
>> had this based on the heavier qemu:debian9 image but we only use plain
>> debian:9 here.
> 
> I first thought "weird, I checked this already" then noticed you rebased
> on debian:9.
> 'stretch-slim' is an alias for '9-slim' and should be tinier (the
> previous RFC image is 323MB).
> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>> ---
>>  tests/docker/dockerfiles/debian-tricore-cross.docker | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/tests/docker/dockerfiles/debian-tricore-cross.docker b/tests/docker/dockerfiles/debian-tricore-cross.docker
>> index 898b8dd511..5ba3ca88a7 100644
>> --- a/tests/docker/dockerfiles/debian-tricore-cross.docker
>> +++ b/tests/docker/dockerfiles/debian-tricore-cross.docker
>> @@ -11,6 +11,9 @@ FROM debian:9
>>  
>>  MAINTAINER Philippe Mathieu-Daudé <f4bug@amsat.org>
>>  
>> +RUN apt-get update && \
>> +    DEBIAN_FRONTEND=noninteractive apt-get install -yy git
>> +
>>  RUN git clone --single-branch \
>>          https://github.com/bkoppelmann/tricore-binutils.git \
>>          /usr/src/binutils && \
>>

Apparently this is not enough:

 ---> Running in a044a41d4521
Cloning into '/usr/src/binutils'...
Configuring for a x86_64-unknown-linux-gnu host.
Created "Makefile" in /usr/src/binutils
./configure: 8: /tmp/cNf15/cNf15.pos: cc: not found
*** The command 'cc -o conftest -w   conftest.c' failed.
*** You must set the environment variable CC to a working compiler.
subprocess.CalledProcessError: Command '['docker', 'build', '-t',
'qemu:debian-tricore-cross', '-f',
'/tmp/docker_build10CkqB/tmpEx12gW.docker', '/tmp/docker_build10CkqB']'
returned non-zero exit status 1
make: *** [docker-image-debian-tricore-cross] Error 1

Re: [Qemu-devel] [PATCH for 3.0 10/10] docker: add expansion for docker-test-FOO to Makefile.include

[Fam Zheng]

On Tue, 07/10 22:04, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
> > On 07/09/2018 12:21 PM, Alex Bennée wrote:
> >> This allows us to run a particular test on all docker images. For
> >> example:
> >>
> >>   make docker-test-unit
> >>
> >> Will run the unit tests on every supported image.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  tests/docker/Makefile.include | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> >> index fe63aacf69..765b2c36f2 100644
> >> --- a/tests/docker/Makefile.include
> >> +++ b/tests/docker/Makefile.include
> >> @@ -152,6 +152,7 @@ $(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPR
> >>  	) \
> >>  	$(foreach t,$(DOCKER_TESTS), \
> >>  		$(eval docker-test: docker-$t@$i) \
> >> +		$(eval docker-$t: docker-$t@$i) \
> >>  	) \
> >>  )
> >>
> >> @@ -162,6 +163,7 @@ docker:
> >>  	@echo
> >>  	@echo '    docker:              Print this help.'
> >>  	@echo '    docker-test:         Run all image/test combinations.'
> >
> > Can we rename "docker-test" -> "docker-tests" or is it too late?
> 
> Probably, I don't know how many people were actively using it. Fam?

I don't know either. Maybe because I'm not native English speaker, but why
bother? "Docker test" sounds like a generic "cover all" test, whereas "docker
tests" sounds like a set of many independent tests based on Docker. Both make
sense to me, though I agree the latter describes it better.

On the other hand, maybe we should create a selection instead of "all
combinations" and call it docker-test, to make it a bit more useful. Then we can
rename the current docker-test to "docker-all" or "docker-test-all".

Fam

> 
> >
> >> +	@echo '    docker-TEST:         Run TEST on all image 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,'
> >>
> 
> 
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH for 3.0 10/10] docker: add expansion for docker-test-FOO to Makefile.include

[Alex Bennée]

Fam Zheng <famz@redhat.com> writes:

> On Tue, 07/10 22:04, Alex Bennée wrote:
>>
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>
>> > On 07/09/2018 12:21 PM, Alex Bennée wrote:
>> >> This allows us to run a particular test on all docker images. For
>> >> example:
>> >>
>> >>   make docker-test-unit
>> >>
>> >> Will run the unit tests on every supported image.
>> >>
>> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> >> ---
>> >>  tests/docker/Makefile.include | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
>> >> index fe63aacf69..765b2c36f2 100644
>> >> --- a/tests/docker/Makefile.include
>> >> +++ b/tests/docker/Makefile.include
>> >> @@ -152,6 +152,7 @@ $(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPR
>> >>  	) \
>> >>  	$(foreach t,$(DOCKER_TESTS), \
>> >>  		$(eval docker-test: docker-$t@$i) \
>> >> +		$(eval docker-$t: docker-$t@$i) \
>> >>  	) \
>> >>  )
>> >>
>> >> @@ -162,6 +163,7 @@ docker:
>> >>  	@echo
>> >>  	@echo '    docker:              Print this help.'
>> >>  	@echo '    docker-test:         Run all image/test combinations.'
>> >
>> > Can we rename "docker-test" -> "docker-tests" or is it too late?
>>
>> Probably, I don't know how many people were actively using it. Fam?
>
> I don't know either. Maybe because I'm not native English speaker, but why
> bother? "Docker test" sounds like a generic "cover all" test, whereas "docker
> tests" sounds like a set of many independent tests based on Docker. Both make
> sense to me, though I agree the latter describes it better.
>
> On the other hand, maybe we should create a selection instead of "all
> combinations" and call it docker-test, to make it a bit more useful.

Well allowing the individual docket-test-FOO expansion is a halfway
house to that.

> Then we can
> rename the current docker-test to "docker-all" or "docker-test-all".

docker-all-tests might be better in case we ever create and test-all
test script...

>
> Fam
>
>>
>> >
>> >> +	@echo '    docker-TEST:         Run TEST on all image 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,'
>> >>
>>
>>
>> --
>> Alex Bennée


--
Alex Bennée

Re: [Qemu-devel] [PATCH for 3.0 09/10] docker: add test-unit runner

[Alex Bennée]

Fam Zheng <famz@redhat.com> writes:

> On Mon, 07/09 16:21, Alex Bennée wrote:
>> This test doesn't even build QEMU, it just runs all the unit tests.
>> Intended to make checking unit tests on all docker images easier.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  tests/docker/test-unit | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>  create mode 100755 tests/docker/test-unit
>>
>> diff --git a/tests/docker/test-unit b/tests/docker/test-unit
>> new file mode 100755
>> index 0000000000..be0d90d748
>> --- /dev/null
>> +++ b/tests/docker/test-unit
>> @@ -0,0 +1,19 @@
>> +#!/bin/bash
>> +#
>> +# Build and run the unit tests
>> +#
>> +# Copyright (c) 2018 Linaro Ltd.
>> +#
>> +# Authors:
>> +#  Alex Bennée <alex.bennee@linaro.org>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2
>> +# or (at your option) any later version. See the COPYING file in
>> +# the top-level directory.
>> +
>> +. common.rc
>> +
>> +cd "$BUILD_DIR"
>> +
>> +configure_qemu
>
> This reads a bit unusual and counter-intuitive: how could configure_qemu succeed
> when the env cannot actually build it? Does configure_qemu fail but the side
> effects needed to run build and run unit tests are done anyway?  Either way,
> please add a comment explaining what is happening here.

Although we are not building QEMU itself we still need a configured
build environment as we build sub-modules of QEMU to bind into the unit
tests. I'll add a comment.

>
> Also, should we test the exit code of configure_qemu?

Or maybe just make the script -e?

>
>> +check_qemu check-unit
>> --
>> 2.17.1
>>


--
Alex Bennée

Re: [Qemu-devel] [PATCH for 3.0 02/10] docker: debian-tricore add git tools

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 07/09/2018 12:21 PM, Alex Bennée wrote:
>> As we check out we need git installed before we start. We would have
>> had this based on the heavier qemu:debian9 image but we only use plain
>> debian:9 here.
>
> I first thought "weird, I checked this already" then noticed you rebased
> on debian:9.
> 'stretch-slim' is an alias for '9-slim' and should be tinier (the
> previous RFC image is 323MB).

Hmm why not just switch to qemu:debian9 as we need the compiler as well?
It's not like there will be many users only carrying this image in their
docker setup.

>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>> ---
>>  tests/docker/dockerfiles/debian-tricore-cross.docker | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/tests/docker/dockerfiles/debian-tricore-cross.docker b/tests/docker/dockerfiles/debian-tricore-cross.docker
>> index 898b8dd511..5ba3ca88a7 100644
>> --- a/tests/docker/dockerfiles/debian-tricore-cross.docker
>> +++ b/tests/docker/dockerfiles/debian-tricore-cross.docker
>> @@ -11,6 +11,9 @@ FROM debian:9
>>
>>  MAINTAINER Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> +RUN apt-get update && \
>> +    DEBIAN_FRONTEND=noninteractive apt-get install -yy git
>> +
>>  RUN git clone --single-branch \
>>          https://github.com/bkoppelmann/tricore-binutils.git \
>>          /usr/src/binutils && \
>>


--
Alex Bennée