From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 76B0DC433F5 for ; Fri, 13 May 2022 15:26:46 +0000 (UTC) Received: from localhost ([::1]:49718 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1npXBl-0004LR-CV for qemu-devel@archiver.kernel.org; Fri, 13 May 2022 11:26:45 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:41834) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1npXAi-0003PY-1X for qemu-devel@nongnu.org; Fri, 13 May 2022 11:25:40 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:39894) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1npXAe-0005Lk-KC for qemu-devel@nongnu.org; Fri, 13 May 2022 11:25:38 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1652455535; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=1sBwbFef93k3hALU1ZS025aivluJ34iv+BJ0l39wlmc=; b=C/hKRPowNMcYV7JqnR3IoYj5mDtgplb58K6epRY4OlposlHdxVnKU8nK5bzpjJxGEdOxKd gTtzUbZsivHVGmuHkAZdwlVDmj4wpHiizQUVfOCtZO2L3P3/s50isJKRP/DjSDic4Om6PA 7Vbw7znQm75fwR2GyZ9kvlz7Z5ojf+8= Received: from mail-ua1-f70.google.com (mail-ua1-f70.google.com [209.85.222.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-300-7D6NAUeAMHOdU7tb_JWTNQ-1; Fri, 13 May 2022 11:25:34 -0400 X-MC-Unique: 7D6NAUeAMHOdU7tb_JWTNQ-1 Received: by mail-ua1-f70.google.com with SMTP id c19-20020ab023d3000000b003627f7c63d4so3886995uan.13 for ; Fri, 13 May 2022 08:25:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=1sBwbFef93k3hALU1ZS025aivluJ34iv+BJ0l39wlmc=; b=EVPsu1RavyIIaZkRITwCppnMofywFBDQDW1ZTc9cLdpZjm3LETo+0qHKgNVkuiFd0n sZzdJWiGSycTBiGBbppkhqsR9KNHUVoBVcRJ5Znv2fYWUlSg/iURJLwuJNCc4g6c6Y9u a70lo/4/pGXunKDXdY40TcK6eGLtd5MO0mpi6T+HNJcAEAeL4g+RQIAc1+j3iPDJycon UcmNk1Zn4hWDTzMaSbM9co3xdTC1PICEtsKSxCPxGj16s4mfDNGKrg9K0TNd7Pdta1LT vQuCcQo/G4xgXpKY7HLex1Bxtgelts7aJdeKNUdqcEm1PGVWRyfAgggJWCstjlW7iif4 X5/Q== X-Gm-Message-State: AOAM533bf/JkeY+uz20jW4H/bqT7fadCVQ6DZCrNUFykhyMj6h3ATiOj st7LcUQmtHIUVCRqG62H4lbgm7Eu5uZ+b9Ap5LX/67Pr+qhtdFH0UvYk7BDgj1Ridiq3YinIw9C tPljZ78ac2xx2GUNhXvMPlRPt7l012vI= X-Received: by 2002:a1f:ce46:0:b0:34e:b018:c8a4 with SMTP id e67-20020a1fce46000000b0034eb018c8a4mr2405954vkg.26.1652455533648; Fri, 13 May 2022 08:25:33 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzI3+Wtm9Wzq1S32YPRoarG9zgaHgXJOebDc4BiDjZzvDwths7SnpUuMf8Gz71WSdjLzLUpP/N2+H2zubqEJ48= X-Received: by 2002:a1f:ce46:0:b0:34e:b018:c8a4 with SMTP id e67-20020a1fce46000000b0034eb018c8a4mr2405942vkg.26.1652455533369; Fri, 13 May 2022 08:25:33 -0700 (PDT) MIME-Version: 1.0 References: <20220513000609.197906-1-jsnow@redhat.com> In-Reply-To: From: John Snow Date: Fri, 13 May 2022 11:25:22 -0400 Message-ID: Subject: Re: [RFC PATCH 0/9] tests: run python tests under the build/tests/venv environment To: =?UTF-8?Q?Daniel_P=2E_Berrang=C3=A9?= Cc: qemu-devel , Qemu-block , Cleber Rosa , =?UTF-8?B?QWxleCBCZW5uw6ll?= , Hanna Reitz , Thomas Huth , Kevin Wolf , Paolo Bonzini , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , Wainer dos Santos Moschetta , Beraldo Leal Content-Type: multipart/alternative; boundary="00000000000012f9af05dee64908" Received-SPF: pass client-ip=170.10.133.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -28 X-Spam_score: -2.9 X-Spam_bar: -- X-Spam_report: (-2.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --00000000000012f9af05dee64908 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, May 13, 2022, 4:35 AM Daniel P. Berrang=C3=A9 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 fetc= h > > packages just-in-time. My thought is to use an environment variable lik= e > > 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-re= q > > 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 ven= v > > 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 tryin= g > > 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 --00000000000012f9af05dee64908 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Fri, May 13, 2022, 4:35 AM Daniel P. Berrang=C3=A9 = <berrange@redhat.com> wrot= e:
On Thu, May 12, 2022 at 08:06:00= PM -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 t= o
> 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 ug= ly so far
> and my notes on how I might make it less ugly.
>
> (1) This doesn't trigger venv creation *from* iotests, it merely p= rints
> a friendly error message if "make check-venv" has not been r= un
> first. Not the greatest.

So if we run the sequence

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

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

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

Several ways to go about that.

<= div dir=3D"auto">
= 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 t= o agree with you, but I see Kevin has some doubts. My #1 goal for Python re= factoring 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 acces= s to qmp/machine can be made to work, too. We can discuss this problem a li= ttle later - the scripts/qmp/ folder needs some work. It will come up in th= e 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&#= 39;t even run the tests yet. I think I am partial to having it install on-d= emand, because then the dependencies are opt-in. mjt told me that Debian do= es not run make check as part of its build yet, for example.

I guess I can see it working either w= ay. I think in the very immediate term I'm motivated to have it be on-d= emand, but long term I think "as part of make" is the eventual go= al.


> (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 li= ke
> 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-w= riter
> to opt into that behavior.

I think I'd expect --system-site-packages to be the default behaviour.<= br> 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.
<= /div>

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

I'll do some testing and I can try having it always do t= his. 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 w= ay around.

It may be sur= prising as to when the system packages get used and when they don't - i= nstinctively I like things that are less dynamic, but I see the argument fo= r 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 uncert= ain.)


> (3) Using one venv for *all* tests means that avocado comes as a pre-r= eq
> for iotests -- which adds avocado as a BuildRequires for the Fedora > SRPM. That's probably not ideal. It may be better to model the tes= t 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 unconditional= ly

> (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<= br> > 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 a= s-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 e= xperiment 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 tryi= ng
> 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 probab= ly not need to be updated very often, but it may happen occasionally.
=

If tests/requirements.txt cha= nges it should be updated, and if python/setup.cfg|py changes it *might* ne= ed 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 havi= ng 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. y= ou add some debugging prints to machine.py)

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

I have = no plans to use git submodules.

With re= gards,
Daniel

Thanks! I appreciate the feedback.

<= div dir=3D"auto">--js
--00000000000012f9af05dee64908--