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 X-Spam-Level: X-Spam-Status: No, score=-10.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B30B7C433F5 for ; Thu, 23 Sep 2021 15:48:26 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id F01B26109E for ; Thu, 23 Sep 2021 15:48:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org F01B26109E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:47982 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mTQxU-0007GG-WB for qemu-devel@archiver.kernel.org; Thu, 23 Sep 2021 11:48:25 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:48462) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mTQrq-0001aD-MB for qemu-devel@nongnu.org; Thu, 23 Sep 2021 11:42:34 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:38274) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mTQre-0005cr-SF for qemu-devel@nongnu.org; Thu, 23 Sep 2021 11:42:34 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1632411741; 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=j3x0Un7NBFPjI9REJDLoBE7tbGzUbXPMMK93EaxZ9W0=; b=HK67/VaAmpSHRLJE755ln/8EYLh17ajraxlXIVGn4BZZfZlEZrtvUQdbut28+L6AqR7CtV nurwksMJkHty9yFL7Q1+wI/36XXXynuoqiFL37sta432U4v47vIi2bBUzW6Ckw6WFw1h/C gTAf2mvJ+FWSvvfXx4BcSwEXUAOqwjc= Received: from mail-oo1-f72.google.com (mail-oo1-f72.google.com [209.85.161.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-124-aAls3UgBOnyxac067b8vhg-1; Thu, 23 Sep 2021 11:42:15 -0400 X-MC-Unique: aAls3UgBOnyxac067b8vhg-1 Received: by mail-oo1-f72.google.com with SMTP id f2-20020a4a2202000000b0028c8a8074deso4040660ooa.20 for ; Thu, 23 Sep 2021 08:42:15 -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=j3x0Un7NBFPjI9REJDLoBE7tbGzUbXPMMK93EaxZ9W0=; b=2nNbs88KLeLf63kN3H8HbMUIk3nJz+4ZA01qWbR6DeFAfKc6uk++cTw+z7UNx8X8CC r9gbXYSK/CWI/AKsn+h2rL4sL/cxSr8ycuy9ycTMN9ErqNUTHBMXnw7QK3eZnZJYvzaO g2JKuDrDtwmMHhi05xPQjSPqJyeter8V7iyZmrZxWz/r0PZmmarD5MN5fpAAWhWDn4oL qo+g31XQxPEkffjIKKGaCb36baMVQ21t1CBadCgVOfWSQAEmw8sgdngxSHTB36+bbFTP gdL5mMquc8KImq3+Aujy96r18D674AqgfmiOJY5WD+M4INv/BsS+tkA7LovVLNwKysqU zNuQ== X-Gm-Message-State: AOAM532bxNFtN/5YiDs5u7H5wy+7122+5cY2Fhrqhfvh558cuOPaLnk7 ADMMyXwbzyoiO0wpcRPtvddx/6+ixzHSdPWG2QuZADwenBtl7AymmZyMOmAZx4L/P+reub+V9Jq A0IZ1kd3C2YUbXm+Qedeu0EAFX/yFwGQ= X-Received: by 2002:aca:f257:: with SMTP id q84mr4092700oih.52.1632411734831; Thu, 23 Sep 2021 08:42:14 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxJBNKvr4ku78n5qxNu5xRwhy15Oj8y7Cn/MTDmYdh8HnXbMxpR78Ri0Sry/qutawKi3tUGN5/OXmPdT2lpng4= X-Received: by 2002:aca:f257:: with SMTP id q84mr4092685oih.52.1632411734493; Thu, 23 Sep 2021 08:42:14 -0700 (PDT) MIME-Version: 1.0 References: <20210923001625.3996451-1-jsnow@redhat.com> <20210923001625.3996451-3-jsnow@redhat.com> In-Reply-To: From: John Snow Date: Thu, 23 Sep 2021 11:42:03 -0400 Message-ID: Subject: Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages To: =?UTF-8?Q?Daniel_P=2E_Berrang=C3=A9?= Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=jsnow@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="0000000000008fe05b05ccab7910" 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: -42 X-Spam_score: -4.3 X-Spam_bar: ---- X-Spam_report: (-4.3 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-1.473, 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, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , Hanna Reitz , Vladimir Sementsov-Ogievskiy , qemu-devel , qemu-block@nongnu.org Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --0000000000008fe05b05ccab7910 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Sep 23, 2021 at 7:09 AM Daniel P. Berrang=C3=A9 wrote: > On Wed, Sep 22, 2021 at 08:16:21PM -0400, John Snow wrote: > > Add a warning for when 'iotests' runs against a qemu namespace that > > isn't the one in the source tree. This might occur if you have > > (accidentally) installed the Python namespace package to your local > > packages. > > IIUC, it is/was a valid use case to run the iotests on arbitrary > QEMU outside the source tree ie a distro packaged binary set. > > Are we not allowing the tests/qemu-iotests/* content to be > run outside the context of the main source tree for this > purpose ? > > eg consider if Fedora/RHEL builds put tests/qemu-iotests/* into > a 'qemu-iotests' RPM, which was installed and used with a distro > package python-qemu ? > > (1) "isn't the one in the source tree" is imprecise language here. The real key is that it must be the QEMU namespace located at " testenv.py/../../../python/qemu". This usually means the source tree, but it'd work in any identically structured filesystem hierarchy. (2) The preceding patch might help illustrate this. At present the iotests expect to be able to find the 'qemu' python packages by navigating directories starting from their own location (and not CWD). What patch 1 does is to centralize the logic that has to go "searching" for the python packages into the init_directories method in testenv.py so that each individual test doesn't have to repeat it. i.e. before patch 1, iotests.py does this: sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python')) so we'll always crawl to ../../python from wherever iotests.py is. After patch 1, we're going to crawl to the same location, but starting from testenv.py instead. testenv.py and iotests.py are in the same directory, so I expect whatever worked before (and in whatever environment) will continue working after. I can't say with full certainty in what circumstances we support running iotests, but I don't think I have introduced any new limitation with this. > > (I'm not going to say that this is because I bit myself with this, > > but. You can fill in the blanks.) > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/testenv.py | 21 ++++++++++++++++++++- > > 1 file changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/tests/qemu-iotests/testenv.py > b/tests/qemu-iotests/testenv.py > > index 88104dace90..8a43b193af5 100644 > > --- a/tests/qemu-iotests/testenv.py > > +++ b/tests/qemu-iotests/testenv.py > > @@ -16,6 +16,8 @@ > > # along with this program. If not, see = . > > # > > > > +import importlib.util > > +import logging > > import os > > import sys > > import tempfile > > @@ -25,7 +27,7 @@ > > import random > > import subprocess > > import glob > > -from typing import List, Dict, Any, Optional, ContextManager > > +from typing import List, Dict, Any, Optional, ContextManager, cast > > > > DEF_GDB_OPTIONS =3D 'localhost:12345' > > > > @@ -112,6 +114,22 @@ def init_directories(self) -> None: > > # Path where qemu goodies live in this source tree. > > qemu_srctree_path =3D Path(__file__, '../../../python').resolv= e() > > > > + # warn if we happen to be able to find 'qemu' packages from an > > + # unexpected location (i.e. the package is already installed i= n > > + # the user's environment) > > + qemu_spec =3D importlib.util.find_spec('qemu.qmp') > > + if qemu_spec: > > + spec_path =3D Path(cast(str, qemu_spec.origin)) > > + try: > > + _ =3D spec_path.relative_to(qemu_srctree_path) > > + except ValueError: > > + self._logger.warning( > > + "WARNING: 'qemu' package will be imported from > outside " > > + "the source tree!") > > + self._logger.warning( > > + "Importing from: '%s'", > > + spec_path.parents[1]) > > It feels to me like the scenario we're blocking here is actally > the scenario we want to aim for. > > It isn't blocking it, it's just a warning. At present, iotests expects that it's using the version of the python packages that are in the source tree / tarball / whatever. Since I converted those packages to be installable to your environment, I introduced an ambiguity about which version iotests is actually using: the version you installed, or the version in the source tree? This is just merely a warning to let you know that iotests is technically doing something new. ("Hey, you're running some other python code than what iotests has done for the past ten years. Are you cool with that?") You're right, though: my actual end-goal (after a lot of pending cleanups I have in-flight, I am getting to it ...!) is to eventually pivot iotests to always using qemu as an installed package and wean it off of using directory-crawling to find its dependencies. We are close to doing that. When we do transition to that, the warning can be dropped as there will no longer be an ambiguity over which version it is using. There are some questions for me to ponder first over exactly how the environment setup should be accomplished, though. > 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 :| > > --0000000000008fe05b05ccab7910 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Sep 23, 2021 at 7:09 AM Danie= l P. Berrang=C3=A9 <berrange@redh= at.com> wrote:
On Wed, Sep 22, 2021 at 08:16:21PM -0400, John Snow wrote:
> Add a warning for when 'iotests' runs against a qemu namespace= that
> isn't the one in the source tree. This might occur if you have
> (accidentally) installed the Python namespace package to your local > packages.

IIUC, it is/was a valid use case to run the iotests on arbitrary
QEMU outside the source tree ie a distro packaged binary set.

Are we not allowing the tests/qemu-iotests/* content to be
run outside the context of the main source tree for this
purpose ?

eg=C2=A0 consider if Fedora/RHEL builds put tests/qemu-iotests/* into
a 'qemu-iotests' RPM, which was installed and used with a distro package python-qemu ?


(1) "isn't the one in the sou= rce tree" is imprecise language here. The real key is that it must be = the QEMU namespace located at "testenv.py/../../../python/qemu". This usually means t= he source tree, but it'd work in any identically structured filesystem = hierarchy.

(2) The preceding patch might help illu= strate this. At present the iotests expect to be able to find the 'qemu= ' python packages by navigating directories starting from their own loc= ation (and not CWD). What patch 1 does is to centralize the logic that has = to go "searching" for the python packages into the init_directori= es method in testenv.py so that each individual test doesn't have to re= peat it.

i.e. before patch 1, iotests.py does = this:
sys.path.append(os.path.join(os.path.dirname(__file__), = 9;..', '..', 'python'))

so we&= #39;ll always crawl to ../../python from wherever iotests.py is.
=
After patch 1, we're going to crawl to the same location= , but starting from testenv.py instead. testenv.py and iotests.py are in th= e same directory, so I expect whatever worked before (and in whatever envir= onment) will continue working after. I can't say with full certainty in= what circumstances we support running iotests, but I don't think I hav= e introduced any new limitation with this.
=C2=A0
> (I'm not going to say that this is because I bit myself with this,=
> but. You can fill in the blanks.)
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>=C2=A0 tests/qemu-iotests/testenv.py | 21 ++++++++++++++++++++-
>=C2=A0 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testen= v.py
> index 88104dace90..8a43b193af5 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -16,6 +16,8 @@
>=C2=A0 # along with this program.=C2=A0 If not, see <http://www.g= nu.org/licenses/>.
>=C2=A0 #
>=C2=A0
> +import importlib.util
> +import logging
>=C2=A0 import os
>=C2=A0 import sys
>=C2=A0 import tempfile
> @@ -25,7 +27,7 @@
>=C2=A0 import random
>=C2=A0 import subprocess
>=C2=A0 import glob
> -from typing import List, Dict, Any, Optional, ContextManager
> +from typing import List, Dict, Any, Optional, ContextManager, cast >=C2=A0
>=C2=A0 DEF_GDB_OPTIONS =3D 'localhost:12345'
>=C2=A0
> @@ -112,6 +114,22 @@ def init_directories(self) -> None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # Path where qemu goodies live in th= is source tree.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_srctree_path =3D Path(__file__,= '../../../python').resolve()
>=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # warn if we happen to be able to find &#= 39;qemu' packages from an
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # unexpected location (i.e. the package i= s already installed in
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # the user's environment)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_spec =3D importlib.util.find_spec(&#= 39;qemu.qmp')
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if qemu_spec:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 spec_path =3D Path(cast(str= , qemu_spec.origin))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 try:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 _ =3D spec_pa= th.relative_to(qemu_srctree_path)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 except ValueError:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._logger.= warning(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= "WARNING: 'qemu' package will be imported from outside "=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= "the source tree!")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._logger.= warning(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= "Importing from: '%s'",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= spec_path.parents[1])

It feels to me like the scenario=C2=A0 we're blocking here is actally the scenario we want to aim for.


It isn't blo= cking it, it's just a warning. At present, iotests expects that it'= s using the version of the python packages that are in the source tree / ta= rball / whatever. Since I converted those packages to be installable to you= r environment, I introduced an ambiguity about which version iotests is act= ually using: the version you installed, or the version in the source tree? = This is just merely a warning to let you know that iotests is technically d= oing something new. ("Hey, you're running some other python code t= han what iotests has done for the past ten years. Are you cool with that?&q= uot;)

You're right, though: my actual end-goal (after a lot of pend= ing cleanups I have in-flight, I am getting to it ...!) is to eventually pi= vot iotests to always using qemu as an installed package and wean it off of= using directory-crawling to find its dependencies. We are close to doing t= hat. When we do transition to that, the warning can be dropped as there wil= l no longer be an ambiguity over which version it is using. There are some = questions for me to ponder first over exactly how the environment setup sho= uld be accomplished, though.
=C2= =A0
Regards,
Daniel
--
|: ht= tps://berrange.com=C2=A0 =C2=A0 =C2=A0 -o-=C2=A0 =C2=A0 h= ttps://www.flickr.com/photos/dberrange :|
|: htt= ps://libvirt.org=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0-o-=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 https://fstop138.berrange.com :|
|: https://entangle-photo.org=C2=A0 =C2=A0 -o-=C2=A0 =C2=A0 = https://www.instagram.com/dberrange :|

--0000000000008fe05b05ccab7910--