[PATCH RFC 0/6] Switch iotests to pyvenv

[PATCH RFC 0/6] Switch iotests to pyvenv

[John Snow]

Hi, this is ... a fairly incomplete series about trying to get iotests
to run out of the configure-time venv. I'm looking for some feedback, so
out to the list it goes.

Primarily, I'm having doubts about these points:

1) I think I need something like "mkvenv install" in the first patch,
   but mkvenv.py is getting pretty long...

2) Is there a way to optimize the speed for patch #2? Maybe installing
   this package can be skipped until it's needed, but that means that
   things like iotest's ./check might get complicated to support that.

3) I cheated quite a bit in patch 4 to use the correct Python to launch
   iotests, but I'm wondering if there's a nicer way to solve this
   more *completely*.

John Snow (6):
  experiment: add mkvenv install
  build, tests: Add qemu in-tree packages to pyvenv at configure time.
  iotests: get rid of '..' in path environment output
  iotests: use the correct python to run linters
  iotests: use pyvenv/bin/python3 to launch child test processes
  iotests: don't add qemu.git/python to PYTHONPATH

 configure                     | 31 +++++++++++++++++++++++++++
 python/scripts/mkvenv.py      | 40 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/linters.py |  2 +-
 tests/qemu-iotests/testenv.py | 21 ++++++++++++------
 4 files changed, 87 insertions(+), 7 deletions(-)

-- 
2.40.1

[PATCH RFC 1/6] experiment: add mkvenv install

[John Snow]

This is just so I can do "mkvenv install './python'" or "mkvenv install
file:python" to install the in-tree packages to pyvenv.

It probably isn't quite appropriate to bypass do_ensure in its entirety
like this because we miss out on a lot of error handling, but as a quick
proof of concept it works just fine.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/scripts/mkvenv.py | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/python/scripts/mkvenv.py b/python/scripts/mkvenv.py
index a47f1eaf5d..ea8df34111 100644
--- a/python/scripts/mkvenv.py
+++ b/python/scripts/mkvenv.py
@@ -940,6 +940,35 @@ def _add_ensure_subcommand(subparsers: Any) -> None:
     )
 
 
+def _add_install_subcommand(subparsers: Any) -> None:
+    subparser = subparsers.add_parser(
+        "install", help="Install the specified package."
+    )
+    subparser.add_argument(
+        "--online",
+        action="store_true",
+        help="Install packages from PyPI, if necessary.",
+    )
+    subparser.add_argument(
+        "--dir",
+        type=str,
+        action="store",
+        help="Path to vendored packages where we may install from.",
+    )
+    subparser.add_argument(
+        '--editable',
+        action="store_true",
+        help="Should package(s) be installed in editable mode?"
+    )
+    subparser.add_argument(
+        "dep_specs",
+        type=str,
+        action="store",
+        help="PEP 508 Dependency specification, e.g. 'meson>=0.61.5'",
+        nargs="+",
+    )
+
+
 def main() -> int:
     """CLI interface to make_qemu_venv. See module docstring."""
     if os.environ.get("DEBUG") or os.environ.get("GITLAB_CI"):
@@ -964,6 +993,7 @@ def main() -> int:
     _add_create_subcommand(subparsers)
     _add_post_init_subcommand(subparsers)
     _add_ensure_subcommand(subparsers)
+    _add_install_subcommand(subparsers)
 
     args = parser.parse_args()
     try:
@@ -982,6 +1012,16 @@ def main() -> int:
                 wheels_dir=args.dir,
                 prog=args.diagnose,
             )
+        if args.command == "install":
+            print(f"mkvenv: installing {', '.join(args.dep_specs)}", file=sys.stderr)
+            pip_args = list(args.dep_specs)
+            if args.editable:
+                pip_args.insert(0, "--editable")
+            pip_install(
+                args=pip_args,
+                online=args.online,
+                wheels_dir=args.dir
+            )
         logger.debug("mkvenv.py %s: exiting", args.command)
     except Ouch as exc:
         print("\n*** Ouch! ***\n", file=sys.stderr)
-- 
2.40.1

[PATCH RFC 2/6] build, tests: Add qemu in-tree packages to pyvenv at configure time.

[John Snow]

though, ouch: on my machine this takes 3-4 entire seconds to do. I wish
it wasn't so slow, but we can't rely on these packages not having any
dependencies any more.

We could theoretically use a .pth hack when creating the venv to
automatically include this directory as an "installed packages"
location, but when we go to drop qemu.qmp in the future, that will break
- I think we need to *install* this package.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 configure | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/configure b/configure
index 01a53576a7..d2e0abc068 100755
--- a/configure
+++ b/configure
@@ -250,6 +250,7 @@ git_submodules_action="update"
 git="git"
 debug_tcg="no"
 docs="auto"
+tests="enabled"
 EXESUF=""
 prefix="/usr/local"
 qemu_suffix="qemu"
@@ -639,6 +640,10 @@ for opt do
   ;;
   --disable-docs) docs=disabled
   ;;
+  --enable-tests) tests=enabled
+  ;;
+  --disable-tests) tests=disabled
+  ;;
   --cpu=*)
   ;;
   --target-list=*) target_list="$optarg"
@@ -985,6 +990,32 @@ if test "$docs" != "disabled" ; then
     fi
 fi
 
+# Optionally pre-load the testing pre-requisites. This is for iotests,
+# vmtests, and anything else that uses Python qemu.* packages. Note that
+# our in-tree qemu packages are currently pure python with zero external
+# dependencies. For this reason, it excludes the Avocado dependencies
+# which are installed on-demand at time of use instead.
+
+mkvenv_flags=""
+if test "$pypi" = "enabled" ; then
+    mkvenv_flags="--online"
+fi
+
+if test "$tests" = "enabled" ; then
+    if ! $mkvenv install \
+         $mkvenv_flags \
+         --editable \
+         --dir "${source_path}/python/wheels" \
+         "${source_path}/python/";
+    then
+        echo "There was a problem installing the in-tree python packages for testing."
+        exit 1
+    fi
+    touch pyvenv/tests.group
+fi
+
+echo "mkvenv: done for now, ciao!"
+
 # Probe for ninja
 
 if test -z "$ninja"; then
-- 
2.40.1

[PATCH RFC 3/6] iotests: get rid of '..' in path environment output

[John Snow]

Resolve the build_root before we append more items onto it so that the
environment output is more concise with less parent directory confetti
in it.

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

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 9a37ad9152..e67ebd254b 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -216,7 +216,7 @@ def __init__(self, source_dir: str, build_dir: str,
         self.source_iotests = source_dir
         self.build_iotests = build_dir
 
-        self.build_root = os.path.join(self.build_iotests, '..', '..')
+        self.build_root = Path(self.build_iotests).parent.parent
 
         self.init_directories()
 
-- 
2.40.1

[PATCH RFC 4/6] iotests: use the correct python to run linters

[John Snow]

Whichever python is used to run iotest 297 should be the one used to
actually run the linters.

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

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 65c4c4e827..9fb3fd1449 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -68,7 +68,7 @@ def run_linter(
     :raise CalledProcessError: If the linter process exits with failure.
     """
     subprocess.run(
-        ('python3', '-m', tool, *args),
+        (sys.executable, '-m', tool, *args),
         env=env,
         check=True,
         stdout=subprocess.PIPE if suppress_output else None,
-- 
2.40.1

[PATCH RFC 5/6] iotests: use pyvenv/bin/python3 to launch child test processes

[John Snow]

Now that there's a fancy venv set up for us by configure, we should take
care to use it even when check is invoked directly.

./check will now use the pyvenv environment when launching python tests,
which allows those tests to find and access the 'qemu.*' packages
without PYTHONPATH modifications.

RFC: This patch now means that ./check may launch test subprocesses
using a different Python than the one used to launch it. If that isn't
acceptable, we might need a launcher shim whose job it is to sit above
"check" and just chooses the correct Python.

...Or maybe it's fine the way it is.

Comments welcome, sorry for my indecision.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/testenv.py | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index e67ebd254b..1b095d70f2 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -138,7 +138,20 @@ def init_binaries(self) -> None:
              PYTHON (for bash tests)
              QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG, QSD_PROG
         """
-        self.python = sys.executable
+        # The python we want to use to launch tests.
+        self.python: str = str(
+            Path(self.build_root).joinpath('pyvenv', 'bin', 'python3')
+        )
+        # RFC: Do I need to amend '.exe' for windows, or nah?
+
+        if self.python != sys.executable:
+            print(
+                "Note: "
+                f"check was launched with a Python ({sys.executable}) "
+                f"that doesn't match QEMU's configured Python ({self.python})."
+                " QEMU's Python will be used for individual test processes.",
+                file=sys.stderr
+            )
 
         def root(*names: str) -> str:
             return os.path.join(self.build_root, *names)
-- 
2.40.1

[PATCH RFC 6/6] iotests: don't add qemu.git/python to PYTHONPATH

[John Snow]

qemu.* should be provided by the configure-time venv, now.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/testenv.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 1b095d70f2..6441145701 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -108,12 +108,8 @@ def init_directories(self) -> None:
              SAMPLE_IMG_DIR
         """
 
-        # Path where qemu goodies live in this source tree.
-        qemu_srctree_path = Path(__file__, '../../../python').resolve()
-
         self.pythonpath = os.pathsep.join(filter(None, (
             self.source_iotests,
-            str(qemu_srctree_path),
             os.getenv('PYTHONPATH'),
         )))
 
-- 
2.40.1

Re: [PATCH RFC 0/6] Switch iotests to pyvenv

[Paolo Bonzini]

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

Il mer 21 giu 2023, 02:21 John Snow <jsnow@redhat.com> ha scritto:

> Hi, this is ... a fairly incomplete series about trying to get iotests
> to run out of the configure-time venv. I'm looking for some feedback, so
> out to the list it goes.
>
> Primarily, I'm having doubts about these points:
>
> 1) I think I need something like "mkvenv install" in the first patch,
>    but mkvenv.py is getting pretty long...
>

It's not a lot of code, but using pip install from configure might also be
good enough, I don't know.

2) Is there a way to optimize the speed for patch #2? Maybe installing

   this package can be skipped until it's needed, but that means that
>    things like iotest's ./check might get complicated to support that.
>
> 3) I cheated quite a bit in patch 4 to use the correct Python to launch
>    iotests, but I'm wondering if there's a nicer way to solve this
>    more *completely*.
>

Maybe patch 4 can use distlib.scripts as well to create the check script in
the build directory? (Yes that's another mkvenv functionality...) On a
phone and don't have the docs at hand, so I am not sure. If not, your
solution is good enough.

Apart from this the only issue is the speed. IIRC having a prebuilt .whl
would fix it, I think for Meson we observed that the slow part was building
the wheel. Possibilities:

1) using --no-pep517 if that also speeds it up?

2) already removing the sources to qemu.qmp since that's the plan anyway;
and then, if you want editability you can install the package with --user
--editable, i.e. outside the venv

Paolo


> John Snow (6):
>   experiment: add mkvenv install
>   build, tests: Add qemu in-tree packages to pyvenv at configure time.
>   iotests: get rid of '..' in path environment output
>   iotests: use the correct python to run linters
>   iotests: use pyvenv/bin/python3 to launch child test processes
>   iotests: don't add qemu.git/python to PYTHONPATH
>
>  configure                     | 31 +++++++++++++++++++++++++++
>  python/scripts/mkvenv.py      | 40 +++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/linters.py |  2 +-
>  tests/qemu-iotests/testenv.py | 21 ++++++++++++------
>  4 files changed, 87 insertions(+), 7 deletions(-)
>
> --
> 2.40.1
>
>
>

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

Re: [PATCH RFC 0/6] Switch iotests to pyvenv

[Paolo Bonzini]

On Wed, Jun 21, 2023 at 9:08 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> Maybe patch 4 can use distlib.scripts as well to create the check script in the build directory? (Yes that's another mkvenv functionality...) On a phone and don't have the docs at hand, so I am not sure. If not, your solution is good enough.
>
> Apart from this the only issue is the speed. IIRC having a prebuilt .whl would fix it, I think for Meson we observed that the slow part was building the wheel. Possibilities:
>
> 1) using --no-pep517 if that also speeds it up?
>
> 2) already removing the sources to qemu.qmp since that's the plan anyway; and then, if you want editability you can install the package with --user --editable, i.e. outside the venv

Nope, it's 3 second always and 1.5 even with the wheel.

Maybe replace qemu.qmp with a wheel and leaving PYTHONPATH for the rest?

Paolo

> Paolo
>
>>
>> John Snow (6):
>>   experiment: add mkvenv install
>>   build, tests: Add qemu in-tree packages to pyvenv at configure time.
>>   iotests: get rid of '..' in path environment output
>>   iotests: use the correct python to run linters
>>   iotests: use pyvenv/bin/python3 to launch child test processes
>>   iotests: don't add qemu.git/python to PYTHONPATH
>>
>>  configure                     | 31 +++++++++++++++++++++++++++
>>  python/scripts/mkvenv.py      | 40 +++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/linters.py |  2 +-
>>  tests/qemu-iotests/testenv.py | 21 ++++++++++++------
>>  4 files changed, 87 insertions(+), 7 deletions(-)
>>
>> --
>> 2.40.1
>>
>>

Re: [PATCH RFC 0/6] Switch iotests to pyvenv

[John Snow]

On Thu, Jun 22, 2023 at 5:24 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Wed, Jun 21, 2023 at 9:08 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Maybe patch 4 can use distlib.scripts as well to create the check script in the build directory? (Yes that's another mkvenv functionality...) On a phone and don't have the docs at hand, so I am not sure. If not, your solution is good enough.
> >

Yeah, that's a possibility... we could "install" the iotests script.
That might keep things simple. I'll investigate it.

> > Apart from this the only issue is the speed. IIRC having a prebuilt .whl would fix it, I think for Meson we observed that the slow part was building the wheel. Possibilities:
> >
> > 1) using --no-pep517 if that also speeds it up?
> >
> > 2) already removing the sources to qemu.qmp since that's the plan anyway; and then, if you want editability you can install the package with --user --editable, i.e. outside the venv
>
> Nope, it's 3 second always and 1.5 even with the wheel.
>
> Maybe replace qemu.qmp with a wheel and leaving PYTHONPATH for the rest?
>
> Paolo
>

Hm, I guess so. It's just disappointing because I was really hoping to
be able to use "pip install" to handle dependencies like a normal
package instead of trying to shoulder that burden with an increasing
amount of custom logic that's hard for anyone but me (or you, now) to
maintain.

It kind of defeats the point of having formatted it as a package to begin with.

Maybe there's a sane way to amortize the cost of installation by not
re-creating it after every call to configure instead -- the rest of
the script is fast enough, perhaps we could default clear to *False*
from now on and use the _get_version() bits to detect if the local
internal package is already installed or not -- and if it is, just
leave it alone.

If we always install it in editable mode, and the path where it is
"installed" is what we expect it to be, it shouldn't have any problems
with being out of date.... I think. We could conceivably use the
"faux" package version the internal package has to signal when the
script needs to re-install it.

Something like that?

--js

Re: [PATCH RFC 0/6] Switch iotests to pyvenv

[Paolo Bonzini]

On Thu, Jun 22, 2023 at 11:03 PM John Snow <jsnow@redhat.com> wrote:
> If we always install it in editable mode, and the path where it is
> "installed" is what we expect it to be, it shouldn't have any problems
> with being out of date.... I think. We could conceivably use the
> "faux" package version the internal package has to signal when the
> script needs to re-install it.

Stupid question, why not treat it just like avocado?

Re: [PATCH RFC 0/6] Switch iotests to pyvenv

[John Snow]

On Thu, Jun 22, 2023 at 5:05 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Thu, Jun 22, 2023 at 11:03 PM John Snow <jsnow@redhat.com> wrote:
> > If we always install it in editable mode, and the path where it is
> > "installed" is what we expect it to be, it shouldn't have any problems
> > with being out of date.... I think. We could conceivably use the
> > "faux" package version the internal package has to signal when the
> > script needs to re-install it.
>
> Stupid question, why not treat it just like avocado?
>

How do you mean? (i.e. installing it on-demand in reaction to "make
check-avocado"?)

Re: [PATCH RFC 0/6] Switch iotests to pyvenv

[Paolo Bonzini]

On Thu, Jun 22, 2023 at 11:08 PM John Snow <jsnow@redhat.com> wrote:
>
> On Thu, Jun 22, 2023 at 5:05 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On Thu, Jun 22, 2023 at 11:03 PM John Snow <jsnow@redhat.com> wrote:
> > > If we always install it in editable mode, and the path where it is
> > > "installed" is what we expect it to be, it shouldn't have any problems
> > > with being out of date.... I think. We could conceivably use the
> > > "faux" package version the internal package has to signal when the
> > > script needs to re-install it.
> >
> > Stupid question, why not treat it just like avocado?
> >
>
> How do you mean? (i.e. installing it on-demand in reaction to "make
> check-avocado"?)

Yes, installing it on-demand the first time "make check-iotests" is
run, using a "depend:" keyword argument in
tests/qemu-iotests/meson.build.

BTW,

from distlib.scripts import ScriptMaker
ScriptMaker('..', '.').make('foo.py')

Seems to do the right thing as long as foo.py includes a shebang (I
tested it inside a virtual environment).

Paolo

Re: [PATCH RFC 0/6] Switch iotests to pyvenv

[John Snow]

On Thu, Jun 22, 2023 at 5:12 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Thu, Jun 22, 2023 at 11:08 PM John Snow <jsnow@redhat.com> wrote:
> >
> > On Thu, Jun 22, 2023 at 5:05 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > On Thu, Jun 22, 2023 at 11:03 PM John Snow <jsnow@redhat.com> wrote:
> > > > If we always install it in editable mode, and the path where it is
> > > > "installed" is what we expect it to be, it shouldn't have any problems
> > > > with being out of date.... I think. We could conceivably use the
> > > > "faux" package version the internal package has to signal when the
> > > > script needs to re-install it.
> > >
> > > Stupid question, why not treat it just like avocado?
> > >
> >
> > How do you mean? (i.e. installing it on-demand in reaction to "make
> > check-avocado"?)
>
> Yes, installing it on-demand the first time "make check-iotests" is
> run, using a "depend:" keyword argument in
> tests/qemu-iotests/meson.build.
>
> BTW,
>
> from distlib.scripts import ScriptMaker
> ScriptMaker('..', '.').make('foo.py')
>
> Seems to do the right thing as long as foo.py includes a shebang (I
> tested it inside a virtual environment).
>
> Paolo

That's possible, but it means that it will break if you run configure
and then immediately go to invoke iotests, unless we have a way to
have iotests bootstrap itself. Which I think can't be done through the
makefile, because we don't know which "make" to run in order to get
that to happen. (Or at least, I don't!)

Possibly I could teach mkvenv a new trick, like "mkvenv init iotests"
and have the mkvenv script DTRT at that point, whatever that is --
ideally exiting very quickly without doing anything.

Re: [PATCH RFC 0/6] Switch iotests to pyvenv

[Paolo Bonzini]

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

Il gio 22 giu 2023, 23:18 John Snow <jsnow@redhat.com> ha scritto:

> Possibly I could teach mkvenv a new trick, like "mkvenv init iotests"
> and have the mkvenv script DTRT at that point, whatever that is --
> ideally exiting very quickly without doing anything.
>

Or maybe check itself should do the bootstrap if it's invoked from the
venv?!?

Paolo

>

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