RFC: This is a very early, crude attempt at switching over to an
external Python package dependency for QMP. This series does not
actually make the switch in and of itself, but instead just switches to
the paradigm of using a venv in general to install the QEMU python
packages instead of using PYTHONPATH to load them from the source tree.

(By installing the package, we can process dependencies.)

I'm sending it to the list so I can show you some of what's ugly so far
and my notes on how I might make it less ugly.

(1) This doesn't trigger venv creation *from* iotests, it merely prints
a friendly error message if "make check-venv" has not been run
first. Not the greatest.

(2) This isn't acceptable for SRPM builds, because it uses PyPI to fetch
packages just-in-time. My thought is to use an environment variable like
QEMU_CHECK_NO_INTERNET that changes the behavior of the venv setup
process. We can use "--system-site-packages" as an argument to venv
creation and "--no-index" as an argument to pip installation to achieve
good behavior in SRPM building scenarios. It'd be up to the spec-writer
to opt into that behavior.

(3) Using one venv for *all* tests means that avocado comes as a pre-req
for iotests -- which adds avocado as a BuildRequires for the Fedora
SRPM. That's probably not ideal. It may be better to model the test venv
as something that can be created in stages: the "core" venv first, and
the avocado packages only when needed.

You can see in these patches that I wasn't really sure how to tie the
check-venv step as a dependency of 'check' or 'check-block', and it
winds up feeling kind of hacky and fragile as a result.

(Patches 6 and 7 feel particularly fishy.)

What I think I would like to do is replace the makefile logic with a
Python bootstrapping script. This will allow me to add in environment
variable logic to accommodate #2 pretty easily. It will also allow
iotests to call into the bootstrap script whenever it detects the venv
isn't set up, which it needed to do anyway in order to print a
user-friendly error message. Lastly, it will make it easier to create a
"tiered" venv that layers in the avocado dependencies only as-needed,
which avoids us having to bloat the SRPM build dependencies.

In the end, I think that approach will:

- Allow us to run iotests without having to run a manual prep step
- Keep additional SRPM deps to a minimum
- Keep makefile hacks to a minimum

The only downside I am really frowning at is that I will have to
replicate some "update the venv if it's outdated" logic that is usually
handled by the Make system in the venv bootstrapper. Still, I think it's
probably the only way to hit all of the requirements here without trying
to concoct a fairly complex Makefile.

any thoughts? If not, I'll just move on to trying to hack up that
version next instead.

--js

John Snow (9):
  python: update for mypy 0.950
  tests: add "TESTS_PYTHON" variable to Makefile
  tests: install "qemu" namespace package into venv
  tests: silence pip upgrade warnings during venv creation
  tests: use tests/venv to run basevm.py-based scripts
  tests: add check-venv as a dependency of check and check-block
  tests: add check-venv to build-tcg-disabled CI recipe
  iotests: fix source directory location
  iotests: use tests/venv for running tests

 .gitlab-ci.d/buildtest.yml             |  1 +
 python/qemu/qmp/util.py                |  4 +++-
 python/setup.cfg                       |  1 +
 tests/Makefile.include                 | 23 +++++++++++++++------
 tests/avocado/avocado_qemu/__init__.py | 11 +++++-----
 tests/avocado/virtio_check_params.py   |  1 -
 tests/avocado/virtio_version.py        |  1 -
 tests/qemu-iotests/testenv.py          | 28 +++++++++++++++++---------
 tests/requirements.txt                 |  1 +
 tests/vm/Makefile.include              | 13 ++++++------
 tests/vm/basevm.py                     |  6 +++---
 11 files changed, 57 insertions(+), 33 deletions(-)

-- 
2.34.1

On 5/13/22 02:06, John Snow wrote:
> The only downside I am really frowning at is that I will have to
> replicate some "update the venv if it's outdated" logic that is usually
> handled by the Make system in the venv bootstrapper. Still, I think it's
> probably the only way to hit all of the requirements here without trying
> to concoct a fairly complex Makefile.
> 
> any thoughts? If not, I'll just move on to trying to hack up that
> version next instead.

I would not even bother with keeping the venv up to date.  Just initialize
it in configure, this is exactly what configure remains useful for in the
Meson-based world:

- add configure options --enable-python-qemu={enabled,system,internal,pip,
auto}/--disable-python-qemu (auto means system>internal>pip>disabled; enabled means
system>internal>pip>error) and matching CONFIG_PYTHON_QEMU=y to
config-host.mak

- use CONFIG_PYTHON_QEMU to enable/disable iotests in tests/qemu-iotests/meson.build

- add a configure option --enable-avocado=
{system,pip,auto,enabled}/--disable-avocado and matching
CONFIG_AVOCADO=y to config-host.mak

- use it to enable/disable acceptance tests in tests/Makefile.include

- build the venv in configure and use the options to pick the right pip install
commands, like

has_python_module() {
   $python -c "import $1" > /dev/null 2>&1
}

# do_pip VENV-PATH VAR PACKAGE [PATH] -- PIP-OPTIONS
do_pip() {
     local num_args source
     num_args=5
     test $4 = '--' && num_args=4
     eval source=\$$2
     # Try to resolve the package using a system install
     case $source in
       enabled|auto|system)
         if has_python_module $3; then
           source=system
         elif test $source = system; then
           error_exit "Python package $3 not installed"
         fi
     esac
     # See if a bundled copy is present
     case $source in
       enabled|auto|internal)
         if test $num_args = 5 && test -f $4/setup.cfg; then
           source=internal
         elif test $source = internal; then
           error_exit "Sources for Python package $3 not found in the QEMU source tree"
         fi
     esac
     # Install the bundled copy or let pip download the package
     case $source in
       internal)
         # The Pip error message should be clear enough
         (cd $1 && . bin/activate && pip install "$@") || exit 1
       ;;
       enabled|auto|pip)
         shift $num_args
         if (cd $1 && . bin/activate && pip install "$@"); then
           source=pip
         elif test $source = auto; then
           source=disabled
         else
           # The Pip error message should be clear enough
           exit 1
         fi
       ;;
     esac
     eval $2=\$source
}

rm -rf venv/
$python -m venv venv/
do_pip venv/ enable_python_qemu qemu.qmp python/qemu -- qemu.qmp
do_pip venv/ enable_avocado avocado -- -r tests/requirements.txt

On Fri, May 13, 2022, 6:21 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 5/13/22 02:06, John Snow wrote:
> > The only downside I am really frowning at is that I will have to
> > replicate some "update the venv if it's outdated" logic that is usually
> > handled by the Make system in the venv bootstrapper. Still, I think it's
> > probably the only way to hit all of the requirements here without trying
> > to concoct a fairly complex Makefile.
> >
> > any thoughts? If not, I'll just move on to trying to hack up that
> > version next instead.
>
> I would not even bother with keeping the venv up to date.  Just initialize
>

I'm worried about this idea being very inconvenient for iterative
development of the python code.

it in configure, this is exactly what configure remains useful for in the
> Meson-based world:
>
> - add configure options --enable-python-qemu={enabled,system,internal,pip,
> auto}/--disable-python-qemu (auto means system>internal>pip>disabled;
> enabled means
> system>internal>pip>error) and matching CONFIG_PYTHON_QEMU=y to
> config-host.mak
>

I'm not sure this makes sense. python/qemu will continue to exist in-tree
and will only ever be "internal" in that sense. It won't be something you
can wholesale install from pip.

i.e. I plan to continue to break off pieces and upstream them, but I intend
to leave several modules as internal only.

So I'm not sure "internal" vs "pip" makes sense config-wise, it's intended
to be a mixture of both, really.

But, I suppose this is how you'd like to address different venv setup
behaviors to accommodate spec builds vs dev builds - with a configure flag
of some kind.

(I suppose you'd then like to see configure error out if it doesn't have
the necessary requisites given the venv-style chosen?)

- use CONFIG_PYTHON_QEMU to enable/disable iotests in
> tests/qemu-iotests/meson.build
>

So it's just skipped if you don't have the reqs to make the venv? (Not an
error?)


> - add a configure option --enable-avocado=
> {system,pip,auto,enabled}/--disable-avocado and matching
> CONFIG_AVOCADO=y to config-host.mak
>
> - use it to enable/disable acceptance tests in tests/Makefile.include
>

And this is how you propose eliminating the need for an always-present
avocado builddep.


> - build the venv in configure and use the options to pick the right pip
> install
> commands, like
>
> has_python_module() {
>    $python -c "import $1" > /dev/null 2>&1
> }
>
> # do_pip VENV-PATH VAR PACKAGE [PATH] -- PIP-OPTIONS
> do_pip() {
>      local num_args source
>      num_args=5
>      test $4 = '--' && num_args=4
>      eval source=\$$2
>      # Try to resolve the package using a system install
>      case $source in
>        enabled|auto|system)
>          if has_python_module $3; then
>            source=system
>          elif test $source = system; then
>            error_exit "Python package $3 not installed"
>          fi
>      esac
>      # See if a bundled copy is present
>      case $source in
>        enabled|auto|internal)
>          if test $num_args = 5 && test -f $4/setup.cfg; then
>            source=internal
>          elif test $source = internal; then
>            error_exit "Sources for Python package $3 not found in the QEMU
> source tree"
>          fi
>      esac
>      # Install the bundled copy or let pip download the package
>      case $source in
>        internal)
>          # The Pip error message should be clear enough
>          (cd $1 && . bin/activate && pip install "$@") || exit 1
>        ;;
>        enabled|auto|pip)
>          shift $num_args
>          if (cd $1 && . bin/activate && pip install "$@"); then
>            source=pip
>          elif test $source = auto; then
>            source=disabled
>          else
>            # The Pip error message should be clear enough
>            exit 1
>          fi
>        ;;
>      esac
>      eval $2=\$source
> }
>
> rm -rf venv/
> $python -m venv venv/
> do_pip venv/ enable_python_qemu qemu.qmp python/qemu -- qemu.qmp
> do_pip venv/ enable_avocado avocado -- -r tests/requirements.txt
>

Won't this rebuild the venv like *all of the time*, basically whenever you
see the "configuration is stale" message?

That both seems like way too often, *and* it wouldn't cover cases when it
really ought to be refreshed.

On 5/13/22 17:39, John Snow wrote:
> On Fri, May 13, 2022, 6:21 AM Paolo Bonzini <pbonzini@redhat.com 
> <mailto:pbonzini@redhat.com>> wrote:
> 
>     On 5/13/22 02:06, John Snow wrote:
>      > The only downside I am really frowning at is that I will have to
>      > replicate some "update the venv if it's outdated" logic that is
>     usually
>      > handled by the Make system in the venv bootstrapper. Still, I
>     think it's
>      > probably the only way to hit all of the requirements here without
>     trying
>      > to concoct a fairly complex Makefile.
>      >
>      > any thoughts? If not, I'll just move on to trying to hack up that
>      > version next instead.
> 
>     I would not even bother with keeping the venv up to date.  Just
>     initialize
> 
> I'm worried about this idea being very inconvenient for iterative 
> development of the python code.

Wouldn't "-e" mostly avoid the inconvenience?

> I'm not sure this makes sense. python/qemu will continue to exist 
> in-tree and will only ever be "internal" in that sense. It won't be 
> something you can wholesale install from pip.
>
> i.e. I plan to continue to break off pieces and upstream them, but I 
> intend to leave several modules as internal only.

Oh, that's what I was missing.  I thought long term all of it would come 
from pip.  But anyway...

> So I'm not sure "internal" vs "pip" makes sense config-wise, it's 
> intended to be a mixture of both, really.

... then neither "system" nor "pip" would not cover the parts of 
"python-qemu" that are always internal, i.e. currently only 
"python-qemu-qmp".  And after

  $python -m venv venv/

you would have

  $python -m pip install -e python/

(That's probably a better way to invoke a pip that's related to $python, 
at least until the venv exists).

By the way, where would you put the python-qemu-qmp submodule?

> But, I suppose this is how you'd like to address different venv setup 
> behaviors to accommodate spec builds vs dev builds - with a configure 
> flag of some kind.

Yes.

> (I suppose you'd then like to see configure error out if it doesn't have 
> the necessary requisites given the venv-style chosen?)
> 
>     - use CONFIG_PYTHON_QEMU to enable/disable iotests in
>     tests/qemu-iotests/meson.build
> 
> So it's just skipped if you don't have the reqs to make the venv? (Not 
> an error?)

That's usually what we do with missing bits yes; you can use 
--enable-python-qemu to force an error in that case.

>     - add a configure option --enable-avocado=
>     {system,pip,auto,enabled}/--disable-avocado and matching
>     CONFIG_AVOCADO=y to config-host.mak
> 
>     - use it to enable/disable acceptance tests in tests/Makefile.include
> 
> And this is how you propose eliminating the need for an always-present 
> avocado builddep.

Yep.

>     rm -rf venv/
>     $python -m venv venv/
>     do_pip venv/ enable_python_qemu qemu.qmp python/qemu -- qemu.qmp
>     do_pip venv/ enable_avocado avocado -- -r tests/requirements.txt
> 
> Won't this rebuild the venv like *all of the time*, basically whenever 
> you see the "configuration is stale" message?

Yes, it would.  I think that's going to happen less and less since 
configure is on a serious diet; but it might still be annoying.  OTOH 
installing system packages (or also "pip install --user") will speed up 
creating the virtual env, because then pip will not be run in the venv.

> That both seems like way too often, *and* it wouldn't cover cases when 
> it really ought to be refreshed.

Which cases are those?

Paolo

On Thu, May 12, 2022 at 08:06:00PM -0400, John Snow wrote:
> RFC: This is a very early, crude attempt at switching over to an
> external Python package dependency for QMP. This series does not
> actually make the switch in and of itself, but instead just switches to
> the paradigm of using a venv in general to install the QEMU python
> packages instead of using PYTHONPATH to load them from the source tree.
> 
> (By installing the package, we can process dependencies.)
> 
> I'm sending it to the list so I can show you some of what's ugly so far
> and my notes on how I might make it less ugly.
> 
> (1) This doesn't trigger venv creation *from* iotests, it merely prints
> a friendly error message if "make check-venv" has not been run
> first. Not the greatest.

So if we run the sequence

  mkdir build
  cd build
  ../configure
  make
  ./tests/qemu-iotests/check 001

It won't work anymore, until we 'make check-venv' (or simply
'make check') ?

I'm somewhat inclined to say that venv should be created
unconditionally by default. ie a plain 'make' should always
everything needed to be able to invoke the tests directly.

> (2) This isn't acceptable for SRPM builds, because it uses PyPI to fetch
> packages just-in-time. My thought is to use an environment variable like
> QEMU_CHECK_NO_INTERNET that changes the behavior of the venv setup
> process. We can use "--system-site-packages" as an argument to venv
> creation and "--no-index" as an argument to pip installation to achieve
> good behavior in SRPM building scenarios. It'd be up to the spec-writer
> to opt into that behavior.

I think I'd expect --system-site-packages to be the default behaviour.
We expect QEMU to be compatible with the packages available in the
distros that we're targetting. So if the dev has the python packages
installed from their distro, we should be using them preferentially.

This is similar to how we bundle slirp/capstone/etc, but will
preferentially use the distro version if it is available.

> (3) Using one venv for *all* tests means that avocado comes as a pre-req
> for iotests -- which adds avocado as a BuildRequires for the Fedora
> SRPM. That's probably not ideal. It may be better to model the test venv
> as something that can be created in stages: the "core" venv first, and
> the avocado packages only when needed.
> 
> You can see in these patches that I wasn't really sure how to tie the
> check-venv step as a dependency of 'check' or 'check-block', and it
> winds up feeling kind of hacky and fragile as a result.

See above, I'm inclined to say the venv should be created unconditionally

> (Patches 6 and 7 feel particularly fishy.)
> 
> What I think I would like to do is replace the makefile logic with a
> Python bootstrapping script. This will allow me to add in environment
> variable logic to accommodate #2 pretty easily. It will also allow
> iotests to call into the bootstrap script whenever it detects the venv
> isn't set up, which it needed to do anyway in order to print a
> user-friendly error message. Lastly, it will make it easier to create a
> "tiered" venv that layers in the avocado dependencies only as-needed,
> which avoids us having to bloat the SRPM build dependencies.

The tests is an area where we still have too much taking place in
Makefiles, as opposed to meson. Can we put a rule in
tests/meson.build to trigger the ven creation ? Gets us closer to
being able to run ninja without using make as a wrapper.

> In the end, I think that approach will:
> 
> - Allow us to run iotests without having to run a manual prep step
> - Keep additional SRPM deps to a minimum
> - Keep makefile hacks to a minimum
> 
> The only downside I am really frowning at is that I will have to
> replicate some "update the venv if it's outdated" logic that is usually
> handled by the Make system in the venv bootstrapper. Still, I think it's
> probably the only way to hit all of the requirements here without trying
> to concoct a fairly complex Makefile.

The only reason we need to update the venv is if a python dependancy
changes right ? If we're using system packages by default that's
a non-issue. If we're using the python-qemu.qmp as a git submodule,
we presumably only need to re-create the venv if we see that the
git submodule hash has changed. IOW, we don't need to worry about
tracking whether individual python deps are outdated.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Fri, May 13, 2022, 4:35 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, May 12, 2022 at 08:06:00PM -0400, John Snow wrote:
> > RFC: This is a very early, crude attempt at switching over to an
> > external Python package dependency for QMP. This series does not
> > actually make the switch in and of itself, but instead just switches to
> > the paradigm of using a venv in general to install the QEMU python
> > packages instead of using PYTHONPATH to load them from the source tree.
> >
> > (By installing the package, we can process dependencies.)
> >
> > I'm sending it to the list so I can show you some of what's ugly so far
> > and my notes on how I might make it less ugly.
> >
> > (1) This doesn't trigger venv creation *from* iotests, it merely prints
> > a friendly error message if "make check-venv" has not been run
> > first. Not the greatest.
>
> So if we run the sequence
>
>   mkdir build
>   cd build
>   ../configure
>   make
>   ./tests/qemu-iotests/check 001
>
> It won't work anymore, until we 'make check-venv' (or simply
> 'make check') ?
>

In this RFC as-is, that's correct. I want to fix that, because I dislike it
too.

Several ways to go about that.

I'm somewhat inclined to say that venv should be created
> unconditionally by default. ie a plain 'make' should always
> everything needed to be able to invoke the tests directly.
>

I'm leaning to agree with you, but I see Kevin has some doubts. My #1 goal
for Python refactoring is usually minimizing interruption to the block
maintainers. I do like the idea of just having it always available and
always taken care of, though.

(This would be useful for making sure that any python scripts or utilities
that need access to qmp/machine can be made to work, too. We can discuss
this problem a little later - the scripts/qmp/ folder needs some work. It
will come up in the full series to make the switch.)

OTOH, A concern about unconditionally building the test venv is that it
might introduce new dependencies for lots of downstreams that don't even
run the tests yet. I think I am partial to having it install on-demand,
because then the dependencies are opt-in. mjt told me that Debian does not
run make check as part of its build yet, for example.

I guess I can see it working either way. I think in the very immediate term
I'm motivated to have it be on-demand, but long term I think "as part of
make" is the eventual goal.


> > (2) This isn't acceptable for SRPM builds, because it uses PyPI to fetch
> > packages just-in-time. My thought is to use an environment variable like
> > QEMU_CHECK_NO_INTERNET that changes the behavior of the venv setup
> > process. We can use "--system-site-packages" as an argument to venv
> > creation and "--no-index" as an argument to pip installation to achieve
> > good behavior in SRPM building scenarios. It'd be up to the spec-writer
> > to opt into that behavior.
>
> I think I'd expect --system-site-packages to be the default behaviour.
> We expect QEMU to be compatible with the packages available in the
> distros that we're targetting. So if the dev has the python packages
> installed from their distro, we should be using them preferentially.
>
> This is similar to how we bundle slirp/capstone/etc, but will
> preferentially use the distro version if it is available.
>

If you think that behavior should apply to tests as well, then OK. I shied
away from having it as the default because it's somewhat unusual to "cede
control" in a venv like this - the mere presence of certain packages in the
system environment may change behavior of certain python libraries. It is a
less well defined environment inherently.

I'll do some testing and I can try having it always do this. I'm curious
about cases where I might require "exactly mypy 0.780" and the user has
mypy 0.770 installed, or maybe even the other way around.

It may be surprising as to when the system packages get used and when they
don't - instinctively I like things that are less dynamic, but I see the
argument for wanting to prefer system packages when possible. At least for
the sake of downstream.

(I kind of feel like upstream should likewise prefer the upstream python
packages too, but ... You've got a lot more packaging experience than me,
so I'm willing to trust you on this point, but I'm personally a little
uncertain.)


> > (3) Using one venv for *all* tests means that avocado comes as a pre-req
> > for iotests -- which adds avocado as a BuildRequires for the Fedora
> > SRPM. That's probably not ideal. It may be better to model the test venv
> > as something that can be created in stages: the "core" venv first, and
> > the avocado packages only when needed.
> >
> > You can see in these patches that I wasn't really sure how to tie the
> > check-venv step as a dependency of 'check' or 'check-block', and it
> > winds up feeling kind of hacky and fragile as a result.
>
> See above, I'm inclined to say the venv should be created unconditionally
>
> > (Patches 6 and 7 feel particularly fishy.)
> >
> > What I think I would like to do is replace the makefile logic with a
> > Python bootstrapping script. This will allow me to add in environment
> > variable logic to accommodate #2 pretty easily. It will also allow
> > iotests to call into the bootstrap script whenever it detects the venv
> > isn't set up, which it needed to do anyway in order to print a
> > user-friendly error message. Lastly, it will make it easier to create a
> > "tiered" venv that layers in the avocado dependencies only as-needed,
> > which avoids us having to bloat the SRPM build dependencies.
>
> The tests is an area where we still have too much taking place in
> Makefiles, as opposed to meson. Can we put a rule in
> tests/meson.build to trigger the ven creation ? Gets us closer to
> being able to run ninja without using make as a wrapper.
>

Paolo has written a lot about this now, and he had some suggestions on
patches 6-8. I'll experiment with that and see if it feels less fragile.


> > In the end, I think that approach will:
> >
> > - Allow us to run iotests without having to run a manual prep step
> > - Keep additional SRPM deps to a minimum
> > - Keep makefile hacks to a minimum
> >
> > The only downside I am really frowning at is that I will have to
> > replicate some "update the venv if it's outdated" logic that is usually
> > handled by the Make system in the venv bootstrapper. Still, I think it's
> > probably the only way to hit all of the requirements here without trying
> > to concoct a fairly complex Makefile.
>
> The only reason we need to update the venv is if a python dependancy
> changes right ? If we're using system packages by default that's
> a non-issue. If we're using the python-qemu.qmp as a git submodule,
> we presumably only need to re-create the venv if we see that the
> git submodule hash has changed. IOW, we don't need to worry about
> tracking whether individual python deps are outdated.
>

The venv should probably not need to be updated very often, but it may
happen occasionally.

If tests/requirements.txt changes it should be updated, and if
python/setup.cfg|py changes it *might* need to be updated. (e.g. new or
removed subpackages, dependency updates, etc. An obvious one coming up is
the removal of qemu.qmp from in-tree and having that dependency be added to
setup.cfg.)

Using the editable installation mode, we won't need to reinstall the venv
if you edit any of the in-tree python modules (e.g. you add some debugging
prints to machine.py)

Even if we use system packages, we need to check that the version
requirements are fulfilled which involves at least re-running pip (not
necessarily recreating the whole venv) and allowing it the chance to fetch
new deps.

I have no plans to use git submodules.

With regards,
> Daniel
>

Thanks! I appreciate the feedback.

--js

Am 13.05.2022 um 10:35 hat Daniel P. Berrangé geschrieben:
> On Thu, May 12, 2022 at 08:06:00PM -0400, John Snow wrote:
> > RFC: This is a very early, crude attempt at switching over to an
> > external Python package dependency for QMP. This series does not
> > actually make the switch in and of itself, but instead just switches to
> > the paradigm of using a venv in general to install the QEMU python
> > packages instead of using PYTHONPATH to load them from the source tree.
> > 
> > (By installing the package, we can process dependencies.)
> > 
> > I'm sending it to the list so I can show you some of what's ugly so far
> > and my notes on how I might make it less ugly.
> > 
> > (1) This doesn't trigger venv creation *from* iotests, it merely prints
> > a friendly error message if "make check-venv" has not been run
> > first. Not the greatest.
> 
> So if we run the sequence
> 
>   mkdir build
>   cd build
>   ../configure
>   make
>   ./tests/qemu-iotests/check 001
> 
> It won't work anymore, until we 'make check-venv' (or simply
> 'make check') ?
> 
> I'm somewhat inclined to say that venv should be created
> unconditionally by default. ie a plain 'make' should always
> everything needed to be able to invoke the tests directly.

To be honest, I'm inclined to say that we already do too much of this.
When I change a source file, I usually just want qemu/qemu-img/qemu-io
updated to run my manual tests. If I want to run a test case, I have no
problem specifying this explicitly. This is what we had before the meson
switch. But what it does now is linking lots of test binaries (that I
won't run before I think that my change is finished and looking good)
and taking much longer than it should take.

Kevin

On Fri, May 13, 2022 at 09:35:23AM +0100, Daniel P. Berrangé wrote:
> On Thu, May 12, 2022 at 08:06:00PM -0400, John Snow wrote:
> > RFC: This is a very early, crude attempt at switching over to an
> > external Python package dependency for QMP. This series does not
> > actually make the switch in and of itself, but instead just switches to
> > the paradigm of using a venv in general to install the QEMU python
> > packages instead of using PYTHONPATH to load them from the source tree.
> > 
> > (By installing the package, we can process dependencies.)
> > 
> > I'm sending it to the list so I can show you some of what's ugly so far
> > and my notes on how I might make it less ugly.
> > 
> > (1) This doesn't trigger venv creation *from* iotests, it merely prints
> > a friendly error message if "make check-venv" has not been run
> > first. Not the greatest.
> 
> So if we run the sequence
> 
>   mkdir build
>   cd build
>   ../configure
>   make
>   ./tests/qemu-iotests/check 001
> 
> It won't work anymore, until we 'make check-venv' (or simply
> 'make check') ?
> 
> I'm somewhat inclined to say that venv should be created
> unconditionally by default. ie a plain 'make' should always
> everything needed to be able to invoke the tests directly.
> 
> > (2) This isn't acceptable for SRPM builds, because it uses PyPI to fetch
> > packages just-in-time. My thought is to use an environment variable like
> > QEMU_CHECK_NO_INTERNET that changes the behavior of the venv setup
> > process. We can use "--system-site-packages" as an argument to venv
> > creation and "--no-index" as an argument to pip installation to achieve
> > good behavior in SRPM building scenarios. It'd be up to the spec-writer
> > to opt into that behavior.
> 
> I think I'd expect --system-site-packages to be the default behaviour.
> We expect QEMU to be compatible with the packages available in the
> distros that we're targetting. So if the dev has the python packages
> installed from their distro, we should be using them preferentially.
> 
> This is similar to how we bundle slirp/capstone/etc, but will
> preferentially use the distro version if it is available.

AFAICT from testing it, when '--system-site-packages' is set
for the venv, then 'pip install' appears to end up being a
no-op if the package is already present in the host, but
installs it if missing.

IOW, if we default to --system-site-packages, but still
also run 'pip install', we should "do the right thing".
It'll use any  distro packages that are available, and
augment with stuff from pip. In the no-op case, pip will
still try to consult the pipy servers to fetch version
info IIUC, so we need to be able to stop that. So I'd
suggest a  --python-env arg taking three values

 * "auto" (the default) - add --system-site-packages, but
   also run 'pip install'. Good for developers day-to-day

 * "system" - add --system-site-packages but never run
   'pip install'.  Good for formal package builds.

 * "pip"  - don't add --system-site-packages, always run
   'pip install'. Good for testing compatibility with
   bleeding edge python versions, or if explicit full
   independance from host python install is desired.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Fri, May 13, 2022, 6:29 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, May 13, 2022 at 09:35:23AM +0100, Daniel P. Berrangé wrote:
> > On Thu, May 12, 2022 at 08:06:00PM -0400, John Snow wrote:
> > > RFC: This is a very early, crude attempt at switching over to an
> > > external Python package dependency for QMP. This series does not
> > > actually make the switch in and of itself, but instead just switches to
> > > the paradigm of using a venv in general to install the QEMU python
> > > packages instead of using PYTHONPATH to load them from the source tree.
> > >
> > > (By installing the package, we can process dependencies.)
> > >
> > > I'm sending it to the list so I can show you some of what's ugly so far
> > > and my notes on how I might make it less ugly.
> > >
> > > (1) This doesn't trigger venv creation *from* iotests, it merely prints
> > > a friendly error message if "make check-venv" has not been run
> > > first. Not the greatest.
> >
> > So if we run the sequence
> >
> >   mkdir build
> >   cd build
> >   ../configure
> >   make
> >   ./tests/qemu-iotests/check 001
> >
> > It won't work anymore, until we 'make check-venv' (or simply
> > 'make check') ?
> >
> > I'm somewhat inclined to say that venv should be created
> > unconditionally by default. ie a plain 'make' should always
> > everything needed to be able to invoke the tests directly.
> >
> > > (2) This isn't acceptable for SRPM builds, because it uses PyPI to
> fetch
> > > packages just-in-time. My thought is to use an environment variable
> like
> > > QEMU_CHECK_NO_INTERNET that changes the behavior of the venv setup
> > > process. We can use "--system-site-packages" as an argument to venv
> > > creation and "--no-index" as an argument to pip installation to achieve
> > > good behavior in SRPM building scenarios. It'd be up to the spec-writer
> > > to opt into that behavior.
> >
> > I think I'd expect --system-site-packages to be the default behaviour.
> > We expect QEMU to be compatible with the packages available in the
> > distros that we're targetting. So if the dev has the python packages
> > installed from their distro, we should be using them preferentially.
> >
> > This is similar to how we bundle slirp/capstone/etc, but will
> > preferentially use the distro version if it is available.
>
> AFAICT from testing it, when '--system-site-packages' is set
> for the venv, then 'pip install' appears to end up being a
> no-op if the package is already present in the host, but
> installs it if missing.
>
> IOW, if we default to --system-site-packages, but still
> also run 'pip install', we should "do the right thing".
> It'll use any  distro packages that are available, and
> augment with stuff from pip. In the no-op case, pip will
> still try to consult the pipy servers to fetch version
> info IIUC, so we need to be able to stop that. So I'd
> suggest a  --python-env arg taking three values
>
>  * "auto" (the default) - add --system-site-packages, but
>    also run 'pip install'. Good for developers day-to-day
>

Sounds like a decent balance...

...My only concern is that the system packages might be very old and it's
possible that the qemu packages will be "too new" or have conflicts with
the system deps.

I'll just have to test this.

e.g. what if I want to require mypy >= 0.900 for testing, but you have a
system package that requires mypy < 0.700?

I don't *know* that this is a problem, but my python-sense is tingling.

The python dep compatibility matrix I try to enforce and support for
testing is already feeling overly wide. This might force me to support an
even wider matrix, which I think is the precisely wrong direction for venvs
where we want tighter control as a rule.


>  * "system" - add --system-site-packages but never run
>    'pip install'.  Good for formal package builds.
>

We still have to install the in-tree qemu ns package, but we can use
--no-index to do it. It'll fail if the deps aren't met.


>  * "pip"  - don't add --system-site-packages, always run
>    'pip install'. Good for testing compatibility with
>    bleeding edge python versions, or if explicit full
>    independance from host python install is desired.
>

as arguments to configure, this spread of options makes sense to me than
paolo's version, but I've still got some doubt on mixing system and venv
packages.

I am also as of yet not sold on building the venv *from* configure, see my
response to Paolo on that topic.

I'll keep plugging away for now, but the big picture is still a tad murky
in my head.

--js

On Fri, May 13, 2022 at 11:55:18AM -0400, John Snow wrote:
> On Fri, May 13, 2022, 6:29 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Fri, May 13, 2022 at 09:35:23AM +0100, Daniel P. Berrangé wrote:
> > > On Thu, May 12, 2022 at 08:06:00PM -0400, John Snow wrote:
> > > > RFC: This is a very early, crude attempt at switching over to an
> > > > external Python package dependency for QMP. This series does not
> > > > actually make the switch in and of itself, but instead just switches to
> > > > the paradigm of using a venv in general to install the QEMU python
> > > > packages instead of using PYTHONPATH to load them from the source tree.
> > > >
> > > > (By installing the package, we can process dependencies.)
> > > >
> > > > I'm sending it to the list so I can show you some of what's ugly so far
> > > > and my notes on how I might make it less ugly.
> > > >
> > > > (1) This doesn't trigger venv creation *from* iotests, it merely prints
> > > > a friendly error message if "make check-venv" has not been run
> > > > first. Not the greatest.
> > >
> > > So if we run the sequence
> > >
> > >   mkdir build
> > >   cd build
> > >   ../configure
> > >   make
> > >   ./tests/qemu-iotests/check 001
> > >
> > > It won't work anymore, until we 'make check-venv' (or simply
> > > 'make check') ?
> > >
> > > I'm somewhat inclined to say that venv should be created
> > > unconditionally by default. ie a plain 'make' should always
> > > everything needed to be able to invoke the tests directly.
> > >
> > > > (2) This isn't acceptable for SRPM builds, because it uses PyPI to
> > fetch
> > > > packages just-in-time. My thought is to use an environment variable
> > like
> > > > QEMU_CHECK_NO_INTERNET that changes the behavior of the venv setup
> > > > process. We can use "--system-site-packages" as an argument to venv
> > > > creation and "--no-index" as an argument to pip installation to achieve
> > > > good behavior in SRPM building scenarios. It'd be up to the spec-writer
> > > > to opt into that behavior.
> > >
> > > I think I'd expect --system-site-packages to be the default behaviour.
> > > We expect QEMU to be compatible with the packages available in the
> > > distros that we're targetting. So if the dev has the python packages
> > > installed from their distro, we should be using them preferentially.
> > >
> > > This is similar to how we bundle slirp/capstone/etc, but will
> > > preferentially use the distro version if it is available.
> >
> > AFAICT from testing it, when '--system-site-packages' is set
> > for the venv, then 'pip install' appears to end up being a
> > no-op if the package is already present in the host, but
> > installs it if missing.
> >
> > IOW, if we default to --system-site-packages, but still
> > also run 'pip install', we should "do the right thing".
> > It'll use any  distro packages that are available, and
> > augment with stuff from pip. In the no-op case, pip will
> > still try to consult the pipy servers to fetch version
> > info IIUC, so we need to be able to stop that. So I'd
> > suggest a  --python-env arg taking three values
> >
> >  * "auto" (the default) - add --system-site-packages, but
> >    also run 'pip install'. Good for developers day-to-day
> >
> 
> Sounds like a decent balance...
> 
> ...My only concern is that the system packages might be very old and it's
> possible that the qemu packages will be "too new" or have conflicts with
> the system deps.
> 
> I'll just have to test this.
> 
> e.g. what if I want to require mypy >= 0.900 for testing, but you have a
> system package that requires mypy < 0.700?

I would expect us to not require packages that are not present in
the distros implied by 

  https://www.qemu.org/docs/master/about/build-platforms.html

if that was absolutely a must have, then gracefully skip tests
if the system version wasn't new enough. The user could always
pass --python-env=pip if they want to force new enough

> The python dep compatibility matrix I try to enforce and support for
> testing is already feeling overly wide. This might force me to support an
> even wider matrix, which I think is the precisely wrong direction for venvs
> where we want tighter control as a rule.

As above, from a QEMU POV we have clearly defined our targetted
platform matrix. Splitting off python packages isn't a reason
to change QEMU's platform matrix IMHO.

> >  * "system" - add --system-site-packages but never run
> >    'pip install'.  Good for formal package builds.
> >
> 
> We still have to install the in-tree qemu ns package, but we can use
> --no-index to do it. It'll fail if the deps aren't met.

> >  * "pip"  - don't add --system-site-packages, always run
> >    'pip install'. Good for testing compatibility with
> >    bleeding edge python versions, or if explicit full
> >    independance from host python install is desired.
> >
> 
> as arguments to configure, this spread of options makes sense to me than
> paolo's version, but I've still got some doubt on mixing system and venv
> packages.
> 
> I am also as of yet not sold on building the venv *from* configure, see my
> response to Paolo on that topic.
> 
> I'll keep plugging away for now, but the big picture is still a tad murky
> in my head.
> 
> --js

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 5/13/22 18:07, Daniel P. Berrangé wrote:
>> e.g. what if I want to require mypy >= 0.900 for testing, but you have a
>> system package that requires mypy < 0.700?
> I would expect us to not require packages that are not present in
> the distros implied by
> 
>    https://www.qemu.org/docs/master/about/build-platforms.html
> 
> if that was absolutely a must have, then gracefully skip tests
> if the system version wasn't new enough. The user could always
> pass --python-env=pip if they want to force new enough
> 

Consider that e.g. RHEL RPMs do not do mypy or pylint in %check, because 
the version of the linters in RHEL is usually older than what the 
upstream packages expect.

I don't think it's a good idea for QEMU to support what Red Hat 
packagers decided was a bad idea to support.

Paolo

On Fri, May 13, 2022, 12:50 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 5/13/22 18:07, Daniel P. Berrangé wrote:
> >> e.g. what if I want to require mypy >= 0.900 for testing, but you have a
> >> system package that requires mypy < 0.700?
> > I would expect us to not require packages that are not present in
> > the distros implied by
> >
> >    https://www.qemu.org/docs/master/about/build-platforms.html
> >
> > if that was absolutely a must have, then gracefully skip tests
> > if the system version wasn't new enough. The user could always
> > pass --python-env=pip if they want to force new enough
> >
>
> Consider that e.g. RHEL RPMs do not do mypy or pylint in %check, because
> the version of the linters in RHEL is usually older than what the
> upstream packages expect.
>
> I don't think it's a good idea for QEMU to support what Red Hat
> packagers decided was a bad idea to support.
>
> Paolo
>

Yeah, I have to insist that due to the way these packages are developed
upstream that it is simply not reasonable to expect that the local package
version will work. pylint changes behavior virtually every single release.
This series itself even has a patch that is playing whackamole to support a
mypy that's brand new while supporting older mypy versions.

It's a huge overhead for little gain.

Far preferable to just say "Oh, your linter version is too old, we can't
run this test locally."

the qemu (and qemu.qmp) packages do not express a runtime/install
dependency on mypy/pylint/isort/flake8/avocado/tox. These packages only get
pulled in for the [devel] option group, and for good reason.

What is really the outlier here is iotest 297, which brings a kind of
meta-test into the same category as functional/regression tests. Supporting
this on default system packages is not on my personal todo list. Moving
this test off to a "make check-python" and deleting iotest 297 might be an
option. This is a test that simply might need to be skipped by an SRPM
build. (it already isn't run, so there's no additional harm by continuing
to not run it.)

If we are running a test suite and we allow pypi via the config, then I
believe we ought to allow the pypi versions to supersede the system ones -
*iff* the system ones are insufficient. I will continue to endeavor to test
a very wide matrix of dependencies, I just have to be honest that I don't
think I will reasonably achieve the full breadth you are asking for here.

For no-internet configs, we may have to accept that some platforms simply
don't have new enough dependencies to run some tests. I don't see this as
violating our build platform promise. I don't believe the build platform
promise ever reasonably extended to a "development platform promise".

--js

>

On 5/13/22 10:35, Daniel P. Berrangé wrote:
> The tests is an area where we still have too much taking place in
> Makefiles, as opposed to meson. Can we put a rule in
> tests/meson.build to trigger the ven creation ? Gets us closer to
> being able to run ninja without using make as a wrapper.

I don't think this is or even should be a goal, because we have multiple 
projects under the QEMU tree:

- the QEMU binaries proper (emulators, tools, etc.)

- the firmware (pc-bios/{vof,s390-ccw,optionrom} with sources, the rest 
as submodules)

- tests/tcg


Each of these has its own build system and it's not possible to unify 
them under a single meson-based build:

- tests/tcg supports cross compilation for a different target, and 
pc-bios/ firmware will soon follow suit (which is why these directories 
haven't been converted to Meson, even though patches exist)

- firmware outside pc-bios/ consists of many external projects each with 
its own build system; right now it is not even buildable except by magic 
invocations that no one really knows

On top of this, there's support for building Docker images for 
cross-compilation which obviously doesn't fit the Meson usecases either.

In other words, Meson is the build system for QEMU *executables* (and 
that's why tests for the QEMU executables are being moved from configure 
to meson), but not for QEMU as a whole.


So I don't expect configure and Make to disappear.  Meson is great at 
building a C program as big as QEMU; but QEMU is not just a C program, 
and isolating the C parts into Meson lets Make handle the rest of the 
complexity better than before, for example with cross compiled firmware 
support.

Likewise, we're now using "meson test" for "make check" and a custom 
runner for "make check-block"; but perhaps one day Avocado could replace 
both runners via custom Avocado resolvers (basically JSON emitters 
similar to Meson introspection data).  That of course depends on whether 
Avocado has feature parity with "meson test".

Paolo