[PATCH 0/9] tests, python: prepare to expand usage of test venv

[PATCH 0/9] tests, python: prepare to expand usage of test venv

[John Snow]

GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/548326343

This series collects some of the uncontroversial elements that serve as
pre-requisites for a later series that seeks to generate a testing venv
by default.

This series makes the following material changes:

- Install the 'qemu' package into the avocado testing venv
- Use the avocado testing venv to run vm-tests
- Use the avocado testing venv to run device-crash-test

None of these changes impact 'make check'; these are all specialty
tests that are not run by default. This series also doesn't change how
iotests are run, doesn't add any new dependencies for SRPM builds, etc.

NOTE: patch 8 isn't strictly required for this series, but including it
here "early" helps the subsequent series. Since the debian docker files
are layered, testing downstream pipelines can fail because the base
image is pulled from the main QEMU repo instead of the downstream.

In other words: I need this patch in origin/main in order to have the
venv module available for later patches that will actually need it in
our debian10 derivative images.

(in other-other-words: the 'clang-user' test *will* need it later.)

John Snow (9):
  python: update for mypy 0.950
  tests: add "TESTS_PYTHON" variable to Makefile
  tests: use python3 as the python executable name
  tests: silence pip upgrade warnings during venv creation
  tests: add quiet-venv-pip macro
  tests: install "qemu" namespace package into venv
  tests: use tests/venv to run basevm.py-based scripts
  tests: add python3-venv to debian10.docker
  tests: run 'device-crash-test' from tests/venv

 .gitlab-ci.d/buildtest.yml               |  8 +++++---
 python/qemu/qmp/util.py                  |  4 +++-
 python/setup.cfg                         |  1 +
 scripts/device-crash-test                | 14 +++++++++++---
 tests/Makefile.include                   | 18 ++++++++++--------
 tests/avocado/avocado_qemu/__init__.py   | 11 +++++------
 tests/avocado/virtio_check_params.py     |  1 -
 tests/avocado/virtio_version.py          |  1 -
 tests/docker/dockerfiles/debian10.docker |  1 +
 tests/requirements.txt                   |  1 +
 tests/vm/Makefile.include                | 13 +++++++------
 tests/vm/basevm.py                       |  6 +++---
 12 files changed, 47 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH 1/9] python: update for mypy 0.950

[John Snow]

typeshed (included in mypy) recently updated to improve the typing for
WriteTransport objects. I was working around this, but now there's a
version where I shouldn't work around it.

Unfortunately this creates some minor ugliness if I want to support both
pre- and post-0.950 versions. For now, for my sanity, just disable the
unused-ignores warning.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 python/qemu/qmp/util.py | 4 +++-
 python/setup.cfg        | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/python/qemu/qmp/util.py b/python/qemu/qmp/util.py
index eaa5fc7d5f9..ca6225e9cda 100644
--- a/python/qemu/qmp/util.py
+++ b/python/qemu/qmp/util.py
@@ -40,7 +40,9 @@ async def flush(writer: asyncio.StreamWriter) -> None:
     drain. The flow control limits are restored after the call is
     completed.
     """
-    transport = cast(asyncio.WriteTransport, writer.transport)
+    transport = cast(  # type: ignore[redundant-cast]
+        asyncio.WriteTransport, writer.transport
+    )
 
     # https://github.com/python/typeshed/issues/5779
     low, high = transport.get_write_buffer_limits()  # type: ignore
diff --git a/python/setup.cfg b/python/setup.cfg
index e877ea56475..c2c61c75190 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -79,6 +79,7 @@ strict = True
 python_version = 3.6
 warn_unused_configs = True
 namespace_packages = True
+warn_unused_ignores = False
 
 [mypy-qemu.utils.qom_fuse]
 # fusepy has no type stubs:
-- 
2.34.1

[PATCH 2/9] tests: add "TESTS_PYTHON" variable to Makefile

[John Snow]

This is a convenience feature: $(PYTHON) points to the Python executable
we were instructed to use by the configure script. We use that Python to
create a virtual environment with the "check-venv" target in
tests/Makefile.include.

$(TESTS_PYTHON) points to the Python executable belonging to the virtual
environment tied to the build. This Python executable is a symlink to
the binary used to create the venv, which will be the version provided
at configure time.

Using $(TESTS_PYTHON) therefore uses the $(PYTHON) executable, but with
paths modified to use packages installed to the venv.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/Makefile.include | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index ec84b2ebc04..146aaa96a00 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -89,6 +89,7 @@ TARGETS=$(patsubst libqemu-%.fa, %, $(filter libqemu-%.fa, $(ninja-targets)))
 TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv
 TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt
 TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
+TESTS_PYTHON=$(TESTS_VENV_DIR)/bin/python
 ifndef AVOCADO_TESTS
 	AVOCADO_TESTS=tests/avocado
 endif
@@ -108,7 +109,7 @@ $(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
             $(PYTHON) -m venv $@, \
             VENV, $@)
 	$(call quiet-command, \
-            $(TESTS_VENV_DIR)/bin/python -m pip -q install -r $(TESTS_VENV_REQ), \
+            $(TESTS_PYTHON) -m pip -q install -r $(TESTS_VENV_REQ), \
             PIP, $(TESTS_VENV_REQ))
 	$(call quiet-command, touch $@)
 
@@ -126,7 +127,7 @@ FEDORA_31_DOWNLOAD=$(filter $(FEDORA_31_ARCHES),$(FEDORA_31_ARCHES_CANDIDATES))
 # download one specific Fedora 31 image
 get-vm-image-fedora-31-%: check-venv
 	$(call quiet-command, \
-             $(TESTS_VENV_DIR)/bin/python -m avocado vmimage get \
+             $(TESTS_PYTHON) -m avocado vmimage get \
              --distro=fedora --distro-version=31 --arch=$*, \
 	"AVOCADO", "Downloading avocado tests VM image for $*")
 
@@ -135,7 +136,7 @@ get-vm-images: check-venv $(patsubst %,get-vm-image-fedora-31-%, $(FEDORA_31_DOW
 
 check-avocado: check-venv $(TESTS_RESULTS_DIR) get-vm-images
 	$(call quiet-command, \
-            $(TESTS_VENV_DIR)/bin/python -m avocado \
+            $(TESTS_PYTHON) -m avocado \
             --show=$(AVOCADO_SHOW) run --job-results-dir=$(TESTS_RESULTS_DIR) \
             $(if $(AVOCADO_TAGS),, --filter-by-tags-include-empty \
 			--filter-by-tags-include-empty-key) \
-- 
2.34.1

[PATCH 3/9] tests: use python3 as the python executable name

[John Snow]

Use "python3" instead of "python" as per PEP0394:
https://peps.python.org/pep-0394/

This should always be defined (in a venv, at least!), matching the
preferred python shebang of "#!/usr/bin/env python3".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 146aaa96a00..f68adda0650 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -89,7 +89,7 @@ TARGETS=$(patsubst libqemu-%.fa, %, $(filter libqemu-%.fa, $(ninja-targets)))
 TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv
 TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt
 TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
-TESTS_PYTHON=$(TESTS_VENV_DIR)/bin/python
+TESTS_PYTHON=$(TESTS_VENV_DIR)/bin/python3
 ifndef AVOCADO_TESTS
 	AVOCADO_TESTS=tests/avocado
 endif
-- 
2.34.1

[PATCH 4/9] tests: silence pip upgrade warnings during venv creation

[John Snow]

Turn off the nag warning coaxing us to upgrade pip. It's not really that
interesting to see in CI logs, and as long as nothing is broken --
nothing is broken.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/Makefile.include | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index f68adda0650..839ffde876a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -109,8 +109,8 @@ $(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
             $(PYTHON) -m venv $@, \
             VENV, $@)
 	$(call quiet-command, \
-            $(TESTS_PYTHON) -m pip -q install -r $(TESTS_VENV_REQ), \
-            PIP, $(TESTS_VENV_REQ))
+            $(TESTS_PYTHON) -m pip -q --disable-pip-version-check install \
+            -r $(TESTS_VENV_REQ), PIP, $(TESTS_VENV_REQ))
 	$(call quiet-command, touch $@)
 
 $(TESTS_RESULTS_DIR):
-- 
2.34.1

[PATCH 5/9] tests: add quiet-venv-pip macro

[John Snow]

Factor out the "test venv pip" macro; rewrite the "check-venv" rule to
be a little more compact. Replace the "PIP" pseudo-command output with
"VENVPIP" to make it 1% more clear that we are talking about using pip
to install something into a venv.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/Makefile.include | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 839ffde876a..052d7f56e9a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -104,13 +104,13 @@ else
 	AVOCADO_CMDLINE_TAGS=$(addprefix -t , $(AVOCADO_TAGS))
 endif
 
+quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
+    $(TESTS_PYTHON) -m pip -q --disable-pip-version-check $1, \
+    "VENVPIP","$1")
+
 $(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
-	$(call quiet-command, \
-            $(PYTHON) -m venv $@, \
-            VENV, $@)
-	$(call quiet-command, \
-            $(TESTS_PYTHON) -m pip -q --disable-pip-version-check install \
-            -r $(TESTS_VENV_REQ), PIP, $(TESTS_VENV_REQ))
+	$(call quiet-command, $(PYTHON) -m venv $@, VENV, $@)
+	$(call quiet-venv-pip,install -r $(TESTS_VENV_REQ))
 	$(call quiet-command, touch $@)
 
 $(TESTS_RESULTS_DIR):
-- 
2.34.1

[PATCH 6/9] tests: install "qemu" namespace package into venv

[John Snow]

This patch adds the "qemu" namespace package to the $build/tests/venv
directory. It does so in "editable" mode, which means that changes to
the source python directory will actively be reflected by the venv.

This patch also then removes any sys.path hacking from the avocado test
scripts directly. By doing this, the environment of where to find these
packages is managed entirely by the virtual environment and not by the
scripts themselves.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/Makefile.include                 |  1 +
 tests/avocado/avocado_qemu/__init__.py | 11 +++++------
 tests/avocado/virtio_check_params.py   |  1 -
 tests/avocado/virtio_version.py        |  1 -
 tests/requirements.txt                 |  1 +
 5 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 052d7f56e9a..d13a3403e9f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -110,6 +110,7 @@ quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
 
 $(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
 	$(call quiet-command, $(PYTHON) -m venv $@, VENV, $@)
+	$(call quiet-venv-pip,install -e "$(SRC_PATH)/python/")
 	$(call quiet-venv-pip,install -r $(TESTS_VENV_REQ))
 	$(call quiet-command, touch $@)
 
diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
index 39f15c1d518..b656a70c55b 100644
--- a/tests/avocado/avocado_qemu/__init__.py
+++ b/tests/avocado/avocado_qemu/__init__.py
@@ -21,6 +21,11 @@
 from avocado.utils import cloudinit, datadrainer, process, ssh, vmimage
 from avocado.utils.path import find_command
 
+from qemu.machine import QEMUMachine
+from qemu.utils import (get_info_usernet_hostfwd_port, kvm_available,
+                        tcg_available)
+
+
 #: The QEMU build root directory.  It may also be the source directory
 #: if building from the source dir, but it's safer to use BUILD_DIR for
 #: that purpose.  Be aware that if this code is moved outside of a source
@@ -35,12 +40,6 @@
 else:
     SOURCE_DIR = BUILD_DIR
 
-sys.path.append(os.path.join(SOURCE_DIR, 'python'))
-
-from qemu.machine import QEMUMachine
-from qemu.utils import (get_info_usernet_hostfwd_port, kvm_available,
-                        tcg_available)
-
 
 def has_cmd(name, args=None):
     """
diff --git a/tests/avocado/virtio_check_params.py b/tests/avocado/virtio_check_params.py
index e869690473a..4093da8a674 100644
--- a/tests/avocado/virtio_check_params.py
+++ b/tests/avocado/virtio_check_params.py
@@ -22,7 +22,6 @@
 import re
 import logging
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.machine import QEMUMachine
 from avocado_qemu import QemuSystemTest
 from avocado import skip
diff --git a/tests/avocado/virtio_version.py b/tests/avocado/virtio_version.py
index 208910bb844..c84e48813a1 100644
--- a/tests/avocado/virtio_version.py
+++ b/tests/avocado/virtio_version.py
@@ -11,7 +11,6 @@
 import sys
 import os
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.machine import QEMUMachine
 from avocado_qemu import QemuSystemTest
 
diff --git a/tests/requirements.txt b/tests/requirements.txt
index a21b59b4439..0ba561b6bdf 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1,5 +1,6 @@
 # Add Python module requirements, one per line, to be installed
 # in the tests/venv Python virtual environment. For more info,
 # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
+# Note that qemu.git/python/ is always implicitly installed.
 avocado-framework==88.1
 pycdlib==1.11.0
-- 
2.34.1

[PATCH 7/9] tests: use tests/venv to run basevm.py-based scripts

[John Snow]

This patch co-opts the virtual environment being used by avocado tests
to also run the basevm.py tests. This is being done in preparation for
for the qemu.qmp package being removed from qemu.git.

As part of the change, remove any sys.path() hacks and treat "qemu" as a
normal third-party import.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/vm/Makefile.include | 13 +++++++------
 tests/vm/basevm.py        |  6 +++---
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index ae91f5043e5..588bc999cc9 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -84,10 +84,11 @@ vm-clean-all:
 
 $(IMAGES_DIR)/%.img:	$(SRC_PATH)/tests/vm/% \
 			$(SRC_PATH)/tests/vm/basevm.py \
-			$(SRC_PATH)/tests/vm/Makefile.include
+			$(SRC_PATH)/tests/vm/Makefile.include \
+			check-venv
 	@mkdir -p $(IMAGES_DIR)
 	$(call quiet-command, \
-		$(PYTHON) $< \
+		$(TESTS_PYTHON) $< \
 		$(if $(V)$(DEBUG), --debug) \
 		$(if $(GENISOIMAGE),--genisoimage $(GENISOIMAGE)) \
 		$(if $(QEMU_LOCAL),--build-path $(BUILD_DIR)) \
@@ -101,9 +102,9 @@ $(IMAGES_DIR)/%.img:	$(SRC_PATH)/tests/vm/% \
 
 
 # Build in VM $(IMAGE)
-vm-build-%: $(IMAGES_DIR)/%.img
+vm-build-%: $(IMAGES_DIR)/%.img check-venv
 	$(call quiet-command, \
-		$(PYTHON) $(SRC_PATH)/tests/vm/$* \
+		$(TESTS_PYTHON) $(SRC_PATH)/tests/vm/$* \
 		$(if $(V)$(DEBUG), --debug) \
 		$(if $(DEBUG), --interactive) \
 		$(if $(J),--jobs $(J)) \
@@ -127,9 +128,9 @@ vm-boot-serial-%: $(IMAGES_DIR)/%.img
 		-device virtio-net-pci,netdev=vnet \
 	|| true
 
-vm-boot-ssh-%: $(IMAGES_DIR)/%.img
+vm-boot-ssh-%: $(IMAGES_DIR)/%.img check-venv
 	$(call quiet-command, \
-		$(PYTHON) $(SRC_PATH)/tests/vm/$* \
+		$(TESTS_PYTHON) $(SRC_PATH)/tests/vm/$* \
 		$(if $(J),--jobs $(J)) \
 		$(if $(V)$(DEBUG), --debug) \
 		$(if $(QEMU_LOCAL),--build-path $(BUILD_DIR)) \
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 254e11c932b..d7d0413df35 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -18,9 +18,6 @@
 import logging
 import time
 import datetime
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.machine import QEMUMachine
-from qemu.utils import get_info_usernet_hostfwd_port, kvm_available
 import subprocess
 import hashlib
 import argparse
@@ -31,6 +28,9 @@
 import traceback
 import shlex
 
+from qemu.machine import QEMUMachine
+from qemu.utils import get_info_usernet_hostfwd_port, kvm_available
+
 SSH_KEY_FILE = os.path.join(os.path.dirname(__file__),
                "..", "keys", "id_rsa")
 SSH_PUB_KEY_FILE = os.path.join(os.path.dirname(__file__),
-- 
2.34.1

[PATCH 8/9] tests: add python3-venv to debian10.docker

[John Snow]

This is needed to be able to add a venv-building step to 'make check';
the clang-user job in particular needs this to be able to run
check-unit.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/docker/dockerfiles/debian10.docker | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
index b414af1b9f7..03be9230664 100644
--- a/tests/docker/dockerfiles/debian10.docker
+++ b/tests/docker/dockerfiles/debian10.docker
@@ -34,4 +34,5 @@ RUN apt update && \
         python3 \
         python3-sphinx \
         python3-sphinx-rtd-theme \
+        python3-venv \
         $(apt-get -s build-dep --arch-only qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
-- 
2.34.1

[PATCH 9/9] tests: run 'device-crash-test' from tests/venv

[John Snow]

Remove the sys.path hacking from device-crash-test, and add in a little
user-friendly message for anyone who was used to running this script
directly from the source tree.

Modify the GitLab job recipes to create the tests/venv first, then run
device-crash-test from that venv.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 .gitlab-ci.d/buildtest.yml |  8 +++++---
 scripts/device-crash-test  | 14 +++++++++++---
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index e9620c30748..fde29c35aa3 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -110,7 +110,8 @@ crash-test-debian:
     IMAGE: debian-amd64
   script:
     - cd build
-    - scripts/device-crash-test -q ./qemu-system-i386
+    - make check-venv
+    - tests/venv/bin/python3 scripts/device-crash-test -q ./qemu-system-i386
 
 build-system-fedora:
   extends: .native_build_job_template
@@ -155,8 +156,9 @@ crash-test-fedora:
     IMAGE: fedora
   script:
     - cd build
-    - scripts/device-crash-test -q ./qemu-system-ppc
-    - scripts/device-crash-test -q ./qemu-system-riscv32
+    - make check-venv
+    - tests/venv/bin/python3 scripts/device-crash-test -q ./qemu-system-ppc
+    - tests/venv/bin/python3 scripts/device-crash-test -q ./qemu-system-riscv32
 
 build-system-centos:
   extends: .native_build_job_template
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index a203b3fdea2..73bcb986937 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -33,10 +33,18 @@ import re
 import random
 import argparse
 from itertools import chain
+from pathlib import Path
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
-from qemu.machine import QEMUMachine
-from qemu.qmp import ConnectError
+try:
+    from qemu.machine import QEMUMachine
+    from qemu.qmp import ConnectError
+except ModuleNotFoundError as exc:
+    path = Path(__file__).resolve()
+    print(f"Module '{exc.name}' not found.")
+    print("  Try 'make check-venv' from your build directory,")
+    print("  and then one way to run this script is like so:")
+    print(f'  > $builddir/tests/venv/bin/python3 "{path}"')
+    sys.exit(1)
 
 logger = logging.getLogger('device-crash-test')
 dbg = logger.debug
-- 
2.34.1

Re: [PATCH 9/9] tests: run 'device-crash-test' from tests/venv

[Paolo Bonzini]

On 5/26/22 02:09, John Snow wrote:
> Remove the sys.path hacking from device-crash-test, and add in a little
> user-friendly message for anyone who was used to running this script
> directly from the source tree.
> 
> Modify the GitLab job recipes to create the tests/venv first, then run
> device-crash-test from that venv.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   .gitlab-ci.d/buildtest.yml |  8 +++++---
>   scripts/device-crash-test  | 14 +++++++++++---
>   2 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> index e9620c30748..fde29c35aa3 100644
> --- a/.gitlab-ci.d/buildtest.yml
> +++ b/.gitlab-ci.d/buildtest.yml
> @@ -110,7 +110,8 @@ crash-test-debian:
>       IMAGE: debian-amd64
>     script:
>       - cd build
> -    - scripts/device-crash-test -q ./qemu-system-i386
> +    - make check-venv
> +    - tests/venv/bin/python3 scripts/device-crash-test -q ./qemu-system-i386
>   
>   build-system-fedora:
>     extends: .native_build_job_template
> @@ -155,8 +156,9 @@ crash-test-fedora:
>       IMAGE: fedora
>     script:
>       - cd build
> -    - scripts/device-crash-test -q ./qemu-system-ppc
> -    - scripts/device-crash-test -q ./qemu-system-riscv32
> +    - make check-venv
> +    - tests/venv/bin/python3 scripts/device-crash-test -q ./qemu-system-ppc
> +    - tests/venv/bin/python3 scripts/device-crash-test -q ./qemu-system-riscv32
>   
>   build-system-centos:
>     extends: .native_build_job_template
> diff --git a/scripts/device-crash-test b/scripts/device-crash-test
> index a203b3fdea2..73bcb986937 100755
> --- a/scripts/device-crash-test
> +++ b/scripts/device-crash-test
> @@ -33,10 +33,18 @@ import re
>   import random
>   import argparse
>   from itertools import chain
> +from pathlib import Path
>   
> -sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
> -from qemu.machine import QEMUMachine
> -from qemu.qmp import ConnectError
> +try:
> +    from qemu.machine import QEMUMachine
> +    from qemu.qmp import ConnectError
> +except ModuleNotFoundError as exc:
> +    path = Path(__file__).resolve()
> +    print(f"Module '{exc.name}' not found.")
> +    print("  Try 'make check-venv' from your build directory,")
> +    print("  and then one way to run this script is like so:")
> +    print(f'  > $builddir/tests/venv/bin/python3 "{path}"')
> +    sys.exit(1)
>   
>   logger = logging.getLogger('device-crash-test')
>   dbg = logger.debug

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Even though I'd still prefer the venv to be setup early (so the 
check-venv change in buildtest.yml and the friendly message in the 
script will go away), this is a step in the right direction.

Paolo

Re: [PATCH 8/9] tests: add python3-venv to debian10.docker

[Paolo Bonzini]

On 5/26/22 02:09, John Snow wrote:
> This is needed to be able to add a venv-building step to 'make check';
> the clang-user job in particular needs this to be able to run
> check-unit.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/docker/dockerfiles/debian10.docker | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
> index b414af1b9f7..03be9230664 100644
> --- a/tests/docker/dockerfiles/debian10.docker
> +++ b/tests/docker/dockerfiles/debian10.docker
> @@ -34,4 +34,5 @@ RUN apt update && \
>           python3 \
>           python3-sphinx \
>           python3-sphinx-rtd-theme \
> +        python3-venv \
>           $(apt-get -s build-dep --arch-only qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 6/9] tests: install "qemu" namespace package into venv

[Paolo Bonzini]

On 5/26/22 02:09, John Snow wrote:
> This patch adds the "qemu" namespace package to the $build/tests/venv
> directory. It does so in "editable" mode, which means that changes to
> the source python directory will actively be reflected by the venv.
> 
> This patch also then removes any sys.path hacking from the avocado test
> scripts directly. By doing this, the environment of where to find these
> packages is managed entirely by the virtual environment and not by the
> scripts themselves.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/Makefile.include                 |  1 +
>   tests/avocado/avocado_qemu/__init__.py | 11 +++++------
>   tests/avocado/virtio_check_params.py   |  1 -
>   tests/avocado/virtio_version.py        |  1 -
>   tests/requirements.txt                 |  1 +
>   5 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 052d7f56e9a..d13a3403e9f 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -110,6 +110,7 @@ quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
>   
>   $(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
>   	$(call quiet-command, $(PYTHON) -m venv $@, VENV, $@)
> +	$(call quiet-venv-pip,install -e "$(SRC_PATH)/python/")
>   	$(call quiet-venv-pip,install -r $(TESTS_VENV_REQ))
>   	$(call quiet-command, touch $@)
>   
> diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> index 39f15c1d518..b656a70c55b 100644
> --- a/tests/avocado/avocado_qemu/__init__.py
> +++ b/tests/avocado/avocado_qemu/__init__.py
> @@ -21,6 +21,11 @@
>   from avocado.utils import cloudinit, datadrainer, process, ssh, vmimage
>   from avocado.utils.path import find_command
>   
> +from qemu.machine import QEMUMachine
> +from qemu.utils import (get_info_usernet_hostfwd_port, kvm_available,
> +                        tcg_available)
> +
> +
>   #: The QEMU build root directory.  It may also be the source directory
>   #: if building from the source dir, but it's safer to use BUILD_DIR for
>   #: that purpose.  Be aware that if this code is moved outside of a source
> @@ -35,12 +40,6 @@
>   else:
>       SOURCE_DIR = BUILD_DIR
>   
> -sys.path.append(os.path.join(SOURCE_DIR, 'python'))
> -
> -from qemu.machine import QEMUMachine
> -from qemu.utils import (get_info_usernet_hostfwd_port, kvm_available,
> -                        tcg_available)
> -
>   
>   def has_cmd(name, args=None):
>       """
> diff --git a/tests/avocado/virtio_check_params.py b/tests/avocado/virtio_check_params.py
> index e869690473a..4093da8a674 100644
> --- a/tests/avocado/virtio_check_params.py
> +++ b/tests/avocado/virtio_check_params.py
> @@ -22,7 +22,6 @@
>   import re
>   import logging
>   
> -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>   from qemu.machine import QEMUMachine
>   from avocado_qemu import QemuSystemTest
>   from avocado import skip
> diff --git a/tests/avocado/virtio_version.py b/tests/avocado/virtio_version.py
> index 208910bb844..c84e48813a1 100644
> --- a/tests/avocado/virtio_version.py
> +++ b/tests/avocado/virtio_version.py
> @@ -11,7 +11,6 @@
>   import sys
>   import os
>   
> -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>   from qemu.machine import QEMUMachine
>   from avocado_qemu import QemuSystemTest
>   
> diff --git a/tests/requirements.txt b/tests/requirements.txt
> index a21b59b4439..0ba561b6bdf 100644
> --- a/tests/requirements.txt
> +++ b/tests/requirements.txt
> @@ -1,5 +1,6 @@
>   # Add Python module requirements, one per line, to be installed
>   # in the tests/venv Python virtual environment. For more info,
>   # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
> +# Note that qemu.git/python/ is always implicitly installed.
>   avocado-framework==88.1
>   pycdlib==1.11.0

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 5/9] tests: add quiet-venv-pip macro

[Paolo Bonzini]

On 5/26/22 02:09, John Snow wrote:
> Factor out the "test venv pip" macro; rewrite the "check-venv" rule to
> be a little more compact. Replace the "PIP" pseudo-command output with
> "VENVPIP" to make it 1% more clear that we are talking about using pip
> to install something into a venv.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/Makefile.include | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 839ffde876a..052d7f56e9a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -104,13 +104,13 @@ else
>   	AVOCADO_CMDLINE_TAGS=$(addprefix -t , $(AVOCADO_TAGS))
>   endif
>   
> +quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
> +    $(TESTS_PYTHON) -m pip -q --disable-pip-version-check $1, \
> +    "VENVPIP","$1")
> +
>   $(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
> -	$(call quiet-command, \
> -            $(PYTHON) -m venv $@, \
> -            VENV, $@)
> -	$(call quiet-command, \
> -            $(TESTS_PYTHON) -m pip -q --disable-pip-version-check install \
> -            -r $(TESTS_VENV_REQ), PIP, $(TESTS_VENV_REQ))
> +	$(call quiet-command, $(PYTHON) -m venv $@, VENV, $@)
> +	$(call quiet-venv-pip,install -r $(TESTS_VENV_REQ))
>   	$(call quiet-command, touch $@)
>   
>   $(TESTS_RESULTS_DIR):

Sooner or later I'd like quiet-command to be changed to English 
descriptions like the ones currently emitted during the ninja build, but 
stuff for later.

Paolo

Re: [PATCH 5/9] tests: add quiet-venv-pip macro

[Paolo Bonzini]

On 5/26/22 02:09, John Snow wrote:
> Factor out the "test venv pip" macro; rewrite the "check-venv" rule to
> be a little more compact. Replace the "PIP" pseudo-command output with
> "VENVPIP" to make it 1% more clear that we are talking about using pip
> to install something into a venv.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/Makefile.include | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 839ffde876a..052d7f56e9a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -104,13 +104,13 @@ else
>   	AVOCADO_CMDLINE_TAGS=$(addprefix -t , $(AVOCADO_TAGS))
>   endif
>   
> +quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
> +    $(TESTS_PYTHON) -m pip -q --disable-pip-version-check $1, \
> +    "VENVPIP","$1")
> +
>   $(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
> -	$(call quiet-command, \
> -            $(PYTHON) -m venv $@, \
> -            VENV, $@)
> -	$(call quiet-command, \
> -            $(TESTS_PYTHON) -m pip -q --disable-pip-version-check install \
> -            -r $(TESTS_VENV_REQ), PIP, $(TESTS_VENV_REQ))
> +	$(call quiet-command, $(PYTHON) -m venv $@, VENV, $@)
> +	$(call quiet-venv-pip,install -r $(TESTS_VENV_REQ))
>   	$(call quiet-command, touch $@)
>   
>   $(TESTS_RESULTS_DIR):

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 4/9] tests: silence pip upgrade warnings during venv creation

[Paolo Bonzini]

On 5/26/22 02:09, John Snow wrote:
> Turn off the nag warning coaxing us to upgrade pip. It's not really that
> interesting to see in CI logs, and as long as nothing is broken --
> nothing is broken.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/Makefile.include | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index f68adda0650..839ffde876a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -109,8 +109,8 @@ $(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
>               $(PYTHON) -m venv $@, \
>               VENV, $@)
>   	$(call quiet-command, \
> -            $(TESTS_PYTHON) -m pip -q install -r $(TESTS_VENV_REQ), \
> -            PIP, $(TESTS_VENV_REQ))
> +            $(TESTS_PYTHON) -m pip -q --disable-pip-version-check install \
> +            -r $(TESTS_VENV_REQ), PIP, $(TESTS_VENV_REQ))
>   	$(call quiet-command, touch $@)
>   
>   $(TESTS_RESULTS_DIR):

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 3/9] tests: use python3 as the python executable name

[Paolo Bonzini]

On 5/26/22 02:09, John Snow wrote:
> Use "python3" instead of "python" as per PEP0394:
> https://peps.python.org/pep-0394/
> 
> This should always be defined (in a venv, at least!), matching the
> preferred python shebang of "#!/usr/bin/env python3".
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/Makefile.include | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 146aaa96a00..f68adda0650 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -89,7 +89,7 @@ TARGETS=$(patsubst libqemu-%.fa, %, $(filter libqemu-%.fa, $(ninja-targets)))
>   TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv
>   TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt
>   TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
> -TESTS_PYTHON=$(TESTS_VENV_DIR)/bin/python
> +TESTS_PYTHON=$(TESTS_VENV_DIR)/bin/python3
>   ifndef AVOCADO_TESTS
>   	AVOCADO_TESTS=tests/avocado
>   endif

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 9/9] tests: run 'device-crash-test' from tests/venv

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 3429 bytes --]

On Thu, May 26, 2022, 8:14 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 5/26/22 02:09, John Snow wrote:
> > Remove the sys.path hacking from device-crash-test, and add in a little
> > user-friendly message for anyone who was used to running this script
> > directly from the source tree.
> >
> > Modify the GitLab job recipes to create the tests/venv first, then run
> > device-crash-test from that venv.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   .gitlab-ci.d/buildtest.yml |  8 +++++---
> >   scripts/device-crash-test  | 14 +++++++++++---
> >   2 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> > index e9620c30748..fde29c35aa3 100644
> > --- a/.gitlab-ci.d/buildtest.yml
> > +++ b/.gitlab-ci.d/buildtest.yml
> > @@ -110,7 +110,8 @@ crash-test-debian:
> >       IMAGE: debian-amd64
> >     script:
> >       - cd build
> > -    - scripts/device-crash-test -q ./qemu-system-i386
> > +    - make check-venv
> > +    - tests/venv/bin/python3 scripts/device-crash-test -q
> ./qemu-system-i386
> >
> >   build-system-fedora:
> >     extends: .native_build_job_template
> > @@ -155,8 +156,9 @@ crash-test-fedora:
> >       IMAGE: fedora
> >     script:
> >       - cd build
> > -    - scripts/device-crash-test -q ./qemu-system-ppc
> > -    - scripts/device-crash-test -q ./qemu-system-riscv32
> > +    - make check-venv
> > +    - tests/venv/bin/python3 scripts/device-crash-test -q
> ./qemu-system-ppc
> > +    - tests/venv/bin/python3 scripts/device-crash-test -q
> ./qemu-system-riscv32
> >
> >   build-system-centos:
> >     extends: .native_build_job_template
> > diff --git a/scripts/device-crash-test b/scripts/device-crash-test
> > index a203b3fdea2..73bcb986937 100755
> > --- a/scripts/device-crash-test
> > +++ b/scripts/device-crash-test
> > @@ -33,10 +33,18 @@ import re
> >   import random
> >   import argparse
> >   from itertools import chain
> > +from pathlib import Path
> >
> > -sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
> > -from qemu.machine import QEMUMachine
> > -from qemu.qmp import ConnectError
> > +try:
> > +    from qemu.machine import QEMUMachine
> > +    from qemu.qmp import ConnectError
> > +except ModuleNotFoundError as exc:
> > +    path = Path(__file__).resolve()
> > +    print(f"Module '{exc.name}' not found.")
> > +    print("  Try 'make check-venv' from your build directory,")
> > +    print("  and then one way to run this script is like so:")
> > +    print(f'  > $builddir/tests/venv/bin/python3 "{path}"')
> > +    sys.exit(1)
> >
> >   logger = logging.getLogger('device-crash-test')
> >   dbg = logger.debug
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Even though I'd still prefer the venv to be setup early (so the
> check-venv change in buildtest.yml and the friendly message in the
> script will go away), this is a step in the right direction.
>
> Paolo
>

Agree, figured I'd do baby steps before I wound up with a 40 patch series,
and this gives Thomas et al a chance to find out if this ruins their
workflow.

(I'll probably keep the friendly message a little while more anyway,
though; to catch anyone who runs this script manually for a release or so.
I should add a section to our QEMU developer's guide and just link to it in
the message and explain the many, many ways you might enter a venv or
install the package.)

[-- Attachment #2: Type: text/html, Size: 4853 bytes --]

Re: [PATCH 5/9] tests: add quiet-venv-pip macro

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

On Thu, May 26, 2022, 8:16 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 5/26/22 02:09, John Snow wrote:
> > Factor out the "test venv pip" macro; rewrite the "check-venv" rule to
> > be a little more compact. Replace the "PIP" pseudo-command output with
> > "VENVPIP" to make it 1% more clear that we are talking about using pip
> > to install something into a venv.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/Makefile.include | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index 839ffde876a..052d7f56e9a 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -104,13 +104,13 @@ else
> >       AVOCADO_CMDLINE_TAGS=$(addprefix -t , $(AVOCADO_TAGS))
> >   endif
> >
> > +quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
> > +    $(TESTS_PYTHON) -m pip -q --disable-pip-version-check $1, \
> > +    "VENVPIP","$1")
> > +
> >   $(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
> > -     $(call quiet-command, \
> > -            $(PYTHON) -m venv $@, \
> > -            VENV, $@)
> > -     $(call quiet-command, \
> > -            $(TESTS_PYTHON) -m pip -q --disable-pip-version-check
> install \
> > -            -r $(TESTS_VENV_REQ), PIP, $(TESTS_VENV_REQ))
> > +     $(call quiet-command, $(PYTHON) -m venv $@, VENV, $@)
> > +     $(call quiet-venv-pip,install -r $(TESTS_VENV_REQ))
> >       $(call quiet-command, touch $@)
> >
> >   $(TESTS_RESULTS_DIR):
>
> Sooner or later I'd like quiet-command to be changed to English
> descriptions like the ones currently emitted during the ninja build, but
> stuff for later.
>
> Paolo
>

If it helps, this is a bit of a stopgap on the way to the configure-driven
version; ideally this goes away by the end of this little project.

(I just thought it made the recipes read nicer and reduced the chance for
anyone else getting the pip flags wrong in the interim.)

--js

>

[-- Attachment #2: Type: text/html, Size: 2984 bytes --]

Re: [PATCH 0/9] tests, python: prepare to expand usage of test venv

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 4017 bytes --]

On Wed, May 25, 2022, 8:09 PM John Snow <jsnow@redhat.com> wrote:

> GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/548326343
>
> This series collects some of the uncontroversial elements that serve as
> pre-requisites for a later series that seeks to generate a testing venv
> by default.
>
> This series makes the following material changes:
>
> - Install the 'qemu' package into the avocado testing venv
> - Use the avocado testing venv to run vm-tests
> - Use the avocado testing venv to run device-crash-test
>
> None of these changes impact 'make check'; these are all specialty
> tests that are not run by default. This series also doesn't change how
> iotests are run, doesn't add any new dependencies for SRPM builds, etc.
>
> NOTE: patch 8 isn't strictly required for this series, but including it
> here "early" helps the subsequent series. Since the debian docker files
> are layered, testing downstream pipelines can fail because the base
> image is pulled from the main QEMU repo instead of the downstream.
>
> In other words: I need this patch in origin/main in order to have the
> venv module available for later patches that will actually need it in
> our debian10 derivative images.
>
> (in other-other-words: the 'clang-user' test *will* need it later.)
>
> John Snow (9):
>   python: update for mypy 0.950
>   tests: add "TESTS_PYTHON" variable to Makefile
>   tests: use python3 as the python executable name
>   tests: silence pip upgrade warnings during venv creation
>   tests: add quiet-venv-pip macro
>   tests: install "qemu" namespace package into venv
>   tests: use tests/venv to run basevm.py-based scripts
>   tests: add python3-venv to debian10.docker
>   tests: run 'device-crash-test' from tests/venv
>
>  .gitlab-ci.d/buildtest.yml               |  8 +++++---
>  python/qemu/qmp/util.py                  |  4 +++-
>  python/setup.cfg                         |  1 +
>  scripts/device-crash-test                | 14 +++++++++++---
>  tests/Makefile.include                   | 18 ++++++++++--------
>  tests/avocado/avocado_qemu/__init__.py   | 11 +++++------
>  tests/avocado/virtio_check_params.py     |  1 -
>  tests/avocado/virtio_version.py          |  1 -
>  tests/docker/dockerfiles/debian10.docker |  1 +
>  tests/requirements.txt                   |  1 +
>  tests/vm/Makefile.include                | 13 +++++++------
>  tests/vm/basevm.py                       |  6 +++---
>  12 files changed, 47 insertions(+), 32 deletions(-)
>
> --
> 2.34.1
>

Paolo: thanks for reviews.

I have a follow-up series that does more adventurous things (the pythonized
bootstrapper, sub-dependency groups, and switching iotests over), but it
introduces some new problems that are more "rfc" tier again.

In short: I allow iotests to bootstrap itself, but this creates two subtle
problems:

(1) If check is engaged without running check-venv first and iotests
creates its own venv, the python binary it uses will be whichever one is
your system default, not necessarily the one you configured your build with.

This is reasonable behavior IMO, but if you later run "make check", there's
no guarantee that Make will re-make the venv with the correct python
binary  That's a subtle landmine.

(2) Similarly, if the venv requirements.txt (or python/setup.cfg) change,
iotests doesn't have the logic to notice it ought to re-make the venv. Only
the makefile does. However, at least in this case, the makefile is
guaranteed to notice if/when we run "check block" again. The odds of these
files changing for most people who aren't *me* are pretty low, so it may
not really come up much. Still, it's not bullet-proof.

(Bonus not-at-all-subtle problem) I need to remove iotest 297, otherwise
iotests under a venv that doesn't install mypy/pylint will never run. I
discussed this upstream recently w/ Kevin, but my series to address it
isn't ready yet. (Just pre-emptively pointing it out to say I'm aware of
it!)

WIP. Will send new RFC series today predicated on top of this series.

--js

>

[-- Attachment #2: Type: text/html, Size: 5411 bytes --]

Re: [PATCH 5/9] tests: add quiet-venv-pip macro

[Paolo Bonzini]

On Thu, May 26, 2022 at 4:17 PM John Snow <jsnow@redhat.com> wrote:
>> > -     $(call quiet-command, \
>> > -            $(TESTS_PYTHON) -m pip -q --disable-pip-version-check install \
>> > -            -r $(TESTS_VENV_REQ), PIP, $(TESTS_VENV_REQ))
>> > +     $(call quiet-command, $(PYTHON) -m venv $@, VENV, $@)
>> > +     $(call quiet-venv-pip,install -r $(TESTS_VENV_REQ))
>> >       $(call quiet-command, touch $@)
>> >
>> >   $(TESTS_RESULTS_DIR):
>>
>> Sooner or later I'd like quiet-command to be changed to English
>> descriptions like the ones currently emitted during the ninja build, but
>> stuff for later.
>
> If it helps, this is a bit of a stopgap on the way to the configure-driven version; ideally this goes away by the end of this little project.
>
> (I just thought it made the recipes read nicer and reduced the chance for anyone else getting the pip flags wrong in the interim.)

Don't worry, you're at least consistent with the current way the macros work.

Paolo

Re: [PATCH 0/9] tests, python: prepare to expand usage of test venv

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]

Paolo: I assume this falls under your jurisdiction...ish, unless Cleber
(avocado) or Alex (tests more broadly) have any specific inputs.

I'm fine with waiting for reviews, but don't know whose bucket this goes to.


On Wed, May 25, 2022, 8:09 PM John Snow <jsnow@redhat.com> wrote:

> GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/548326343
>
> This series collects some of the uncontroversial elements that serve as
> pre-requisites for a later series that seeks to generate a testing venv
> by default.
>
> This series makes the following material changes:
>
> - Install the 'qemu' package into the avocado testing venv
> - Use the avocado testing venv to run vm-tests
> - Use the avocado testing venv to run device-crash-test
>
> None of these changes impact 'make check'; these are all specialty
> tests that are not run by default. This series also doesn't change how
> iotests are run, doesn't add any new dependencies for SRPM builds, etc.
>
> NOTE: patch 8 isn't strictly required for this series, but including it
> here "early" helps the subsequent series. Since the debian docker files
> are layered, testing downstream pipelines can fail because the base
> image is pulled from the main QEMU repo instead of the downstream.
>
> In other words: I need this patch in origin/main in order to have the
> venv module available for later patches that will actually need it in
> our debian10 derivative images.
>
> (in other-other-words: the 'clang-user' test *will* need it later.)
>
> John Snow (9):
>   python: update for mypy 0.950
>   tests: add "TESTS_PYTHON" variable to Makefile
>   tests: use python3 as the python executable name
>   tests: silence pip upgrade warnings during venv creation
>   tests: add quiet-venv-pip macro
>   tests: install "qemu" namespace package into venv
>   tests: use tests/venv to run basevm.py-based scripts
>   tests: add python3-venv to debian10.docker
>   tests: run 'device-crash-test' from tests/venv
>
>  .gitlab-ci.d/buildtest.yml               |  8 +++++---
>  python/qemu/qmp/util.py                  |  4 +++-
>  python/setup.cfg                         |  1 +
>  scripts/device-crash-test                | 14 +++++++++++---
>  tests/Makefile.include                   | 18 ++++++++++--------
>  tests/avocado/avocado_qemu/__init__.py   | 11 +++++------
>  tests/avocado/virtio_check_params.py     |  1 -
>  tests/avocado/virtio_version.py          |  1 -
>  tests/docker/dockerfiles/debian10.docker |  1 +
>  tests/requirements.txt                   |  1 +
>  tests/vm/Makefile.include                | 13 +++++++------
>  tests/vm/basevm.py                       |  6 +++---
>  12 files changed, 47 insertions(+), 32 deletions(-)
>
> --
> 2.34.1
>
>

[-- Attachment #2: Type: text/html, Size: 3571 bytes --]

Re: [PATCH 8/9] tests: add python3-venv to debian10.docker

[Thomas Huth]

On 26/05/2022 02.09, John Snow wrote:
> This is needed to be able to add a venv-building step to 'make check';
> the clang-user job in particular needs this to be able to run
> check-unit.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/docker/dockerfiles/debian10.docker | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
> index b414af1b9f7..03be9230664 100644
> --- a/tests/docker/dockerfiles/debian10.docker
> +++ b/tests/docker/dockerfiles/debian10.docker
> @@ -34,4 +34,5 @@ RUN apt update && \
>           python3 \
>           python3-sphinx \
>           python3-sphinx-rtd-theme \
> +        python3-venv \
>           $(apt-get -s build-dep --arch-only qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)

Note that we'll (hopefully) drop the debian 10 container soon, since Debian 
10 is EOL by the time we publish the next QEMU release.

  Thomas

Re: [PATCH 8/9] tests: add python3-venv to debian10.docker

[John Snow]

On Mon, May 30, 2022 at 3:33 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 26/05/2022 02.09, John Snow wrote:
> > This is needed to be able to add a venv-building step to 'make check';
> > the clang-user job in particular needs this to be able to run
> > check-unit.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/docker/dockerfiles/debian10.docker | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
> > index b414af1b9f7..03be9230664 100644
> > --- a/tests/docker/dockerfiles/debian10.docker
> > +++ b/tests/docker/dockerfiles/debian10.docker
> > @@ -34,4 +34,5 @@ RUN apt update && \
> >           python3 \
> >           python3-sphinx \
> >           python3-sphinx-rtd-theme \
> > +        python3-venv \
> >           $(apt-get -s build-dep --arch-only qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
>
> Note that we'll (hopefully) drop the debian 10 container soon, since Debian
> 10 is EOL by the time we publish the next QEMU release.
>

Noted -- do you think it'd be OK to sneak this change in first and
have you move the requisite to the new container? :)

Re: [PATCH 8/9] tests: add python3-venv to debian10.docker

[Thomas Huth]

On 31/05/2022 20.28, John Snow wrote:
> On Mon, May 30, 2022 at 3:33 AM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 26/05/2022 02.09, John Snow wrote:
>>> This is needed to be able to add a venv-building step to 'make check';
>>> the clang-user job in particular needs this to be able to run
>>> check-unit.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>    tests/docker/dockerfiles/debian10.docker | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
>>> index b414af1b9f7..03be9230664 100644
>>> --- a/tests/docker/dockerfiles/debian10.docker
>>> +++ b/tests/docker/dockerfiles/debian10.docker
>>> @@ -34,4 +34,5 @@ RUN apt update && \
>>>            python3 \
>>>            python3-sphinx \
>>>            python3-sphinx-rtd-theme \
>>> +        python3-venv \
>>>            $(apt-get -s build-dep --arch-only qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
>>
>> Note that we'll (hopefully) drop the debian 10 container soon, since Debian
>> 10 is EOL by the time we publish the next QEMU release.
>>
> 
> Noted -- do you think it'd be OK to sneak this change in first and
> have you move the requisite to the new container? :)

I don't mind - whatever comes first ... I just wanted to make you aware that 
there might be conflicts ;-)

  Thomas

Re: [PATCH 0/9] tests, python: prepare to expand usage of test venv

[Paolo Bonzini]

On 5/26/22 16:34, John Snow wrote:
> (1) If check is engaged without running check-venv first and iotests 
> creates its own venv, the python binary it uses will be whichever one is 
> your system default, not necessarily the one you configured your build with.
> 
> This is reasonable behavior IMO, but if you later run "make check", 
> there's no guarantee that Make will re-make the venv with the correct 
> python binary  That's a subtle landmine.

Yup, that's also a reason to make initial venv creation part of 
configure.  I consider that on the same level as running Meson and 
setting up git submodules.

> (2) Similarly, if the venv requirements.txt (or python/setup.cfg) 
> change, iotests doesn't have the logic to notice it ought to re-make the 
> venv. Only the makefile does. However, at least in this case, the 
> makefile is guaranteed to notice if/when we run "check block" again. The 
> odds of these files changing for most people who aren't *me* are pretty 
> low, so it may not really come up much. Still, it's not bullet-proof.

Yeah, this is fine.  Compare it with e.g. running clang-query: it needs 
a "make" first to rebuild compile_commands.json.

Paolo

> (Bonus not-at-all-subtle problem) I need to remove iotest 297, otherwise 
> iotests under a venv that doesn't install mypy/pylint will never run. I 
> discussed this upstream recently w/ Kevin, but my series to address it 
> isn't ready yet. (Just pre-emptively pointing it out to say I'm aware of 
> it!)

Re: [PATCH 0/9] tests, python: prepare to expand usage of test venv

[Paolo Bonzini]

On 5/27/22 16:27, John Snow wrote:
> Paolo: I assume this falls under your jurisdiction...ish, unless Cleber 
> (avocado) or Alex (tests more broadly) have any specific inputs.
> 
> I'm fine with waiting for reviews, but don't know whose bucket this goes to.
> 

I thought it was yours, but I've queued it now.

Paolo

Re: [PATCH 0/9] tests, python: prepare to expand usage of test venv

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

On Wed, Jun 1, 2022, 6:06 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 5/27/22 16:27, John Snow wrote:
> > Paolo: I assume this falls under your jurisdiction...ish, unless Cleber
> > (avocado) or Alex (tests more broadly) have any specific inputs.
> >
> > I'm fine with waiting for reviews, but don't know whose bucket this goes
> to.
> >
>
> I thought it was yours, but I've queued it now.
>
> Paolo
>

I wanted to be polite since it was build system and tests as well - I don't
technically maintain most of these files :)

Thank you!

>

[-- Attachment #2: Type: text/html, Size: 1143 bytes --]

Re: [PATCH 8/9] tests: add python3-venv to debian10.docker

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Wed, Jun 1, 2022, 3:29 AM Thomas Huth <thuth@redhat.com> wrote:

> On 31/05/2022 20.28, John Snow wrote:
> > On Mon, May 30, 2022 at 3:33 AM Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> On 26/05/2022 02.09, John Snow wrote:
> >>> This is needed to be able to add a venv-building step to 'make check';
> >>> the clang-user job in particular needs this to be able to run
> >>> check-unit.
> >>>
> >>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>> ---
> >>>    tests/docker/dockerfiles/debian10.docker | 1 +
> >>>    1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/tests/docker/dockerfiles/debian10.docker
> b/tests/docker/dockerfiles/debian10.docker
> >>> index b414af1b9f7..03be9230664 100644
> >>> --- a/tests/docker/dockerfiles/debian10.docker
> >>> +++ b/tests/docker/dockerfiles/debian10.docker
> >>> @@ -34,4 +34,5 @@ RUN apt update && \
> >>>            python3 \
> >>>            python3-sphinx \
> >>>            python3-sphinx-rtd-theme \
> >>> +        python3-venv \
> >>>            $(apt-get -s build-dep --arch-only qemu | egrep ^Inst |
> fgrep '[all]' | cut -d\  -f2)
> >>
> >> Note that we'll (hopefully) drop the debian 10 container soon, since
> Debian
> >> 10 is EOL by the time we publish the next QEMU release.
> >>
> >
> > Noted -- do you think it'd be OK to sneak this change in first and
> > have you move the requisite to the new container? :)
>
> I don't mind - whatever comes first ... I just wanted to make you aware
> that
> there might be conflicts ;-)
>
>   Thomas
>

Yep, got it! No problem at all. Thanks ~~

>

[-- Attachment #2: Type: text/html, Size: 2589 bytes --]