Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[John Snow]

On Thu, Sep 23, 2021 at 7:09 AM Daniel P. Berrangé 
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 = 'localhost:12345'
> >
> > @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> >  # Path where qemu goodies live in this source tree.
> >  qemu_srctree_path = Path(__file__, '../../../python').resolve()
> >
> > +# warn if we happen to be able to find 'qemu' packages from an
> > +# unexpected location (i.e. the package is already installed in
> > +# the user's environment)
> > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> > +if qemu_spec:
> > +spec_path = Path(cast(str, qemu_spec.origin))
> > +try:
> > +_ = 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 

Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[John Snow]

On Thu, Sep 23, 2021 at 7:17 AM Kevin Wolf  wrote:

> Am 23.09.2021 um 12:57 hat Kevin Wolf geschrieben:
> > Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> > > 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.
> > >
> > > (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 = 'localhost:12345'
> > >
> > > @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> > >  # Path where qemu goodies live in this source tree.
> > >  qemu_srctree_path = Path(__file__,
> '../../../python').resolve()
> > >
> > > +# warn if we happen to be able to find 'qemu' packages from an
> > > +# unexpected location (i.e. the package is already installed
> in
> > > +# the user's environment)
> > > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> > > +if qemu_spec:
> > > +spec_path = Path(cast(str, qemu_spec.origin))
> >
> > You're casting Optional[str] to str here. The source type is not obvious
> > from the local code, so unless you spend some time actively figuring it
> > out, this could be casting anything to str. But even knowing this, just
> > casting Optional away looks fishy anyway.
> >
> > Looking up the ModuleSpec docs, it seems okay to assume that it's never
> > None in our case, but wouldn't it be much cleaner to just 'assert
> > qemu_spec.origin' first and then use it without the cast?
> >
>

OK. I suppose I was thinking: "It's always going to be a string, and if it
isn't, something else will explode below, surely, so the assertion is
redundant", but I don't want code that makes people wonder, so principle of
least surprise it is.


> > > +try:
> > > +_ = 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])
> > > +
> > >  self.pythonpath = os.getenv('PYTHONPATH')
> > >  self.pythonpath = os.pathsep.join(filter(None, (
> > >  self.source_iotests,
> > > @@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str,
> aiomode: str,
> > >
> > >  self.build_root = os.path.join(self.build_iotests, '..', '..')
> > >
> > > +self._logger = logging.getLogger('qemu.iotests')
> > >  self.init_directories()
> > >  self.init_binaries()
> >
> > Looks good otherwise.
>
> Well, actually there is a major problem: We'll pass the right PYTHONPATH
> for the test scripts that we're calling, but this script itself doesn't
> use it yet. So what I get is:
>
> Traceback (most recent call last):
>   File "/home/kwolf/source/qemu/tests/qemu-iotests/build/check", line 120,
> in 
> env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
>   File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 253,
> in __init__
> self.init_directories()
>   File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 120,
> in init_directories
> qemu_spec = importlib.util.find_spec('qemu.qmp')
>   File "/usr/lib64/python3.9/importlib/util.py", line 94, in find_spec
> parent = __import__(parent_name, fromlist=['__path__'])
> ModuleNotFoundError: No module named 'qemu'
>
> Kevin
>
>
Tch. So, it turns out with namespace packages that once you remove the
subpackages (pip uninstall qemu), it leaves the empty namespace folder
behind. This is also the reason I directly use 'qemu.qmp' as a bellwether
here, to make sure we're prodding a package and not just an empty namespace
with nothing in it.

I'll fix these, thanks.

Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[Kevin Wolf]

Am 23.09.2021 um 12:57 hat Kevin Wolf geschrieben:
> Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> > 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.
> > 
> > (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 = 'localhost:12345'
> >  
> > @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> >  # Path where qemu goodies live in this source tree.
> >  qemu_srctree_path = Path(__file__, '../../../python').resolve()
> >  
> > +# warn if we happen to be able to find 'qemu' packages from an
> > +# unexpected location (i.e. the package is already installed in
> > +# the user's environment)
> > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> > +if qemu_spec:
> > +spec_path = Path(cast(str, qemu_spec.origin))
> 
> You're casting Optional[str] to str here. The source type is not obvious
> from the local code, so unless you spend some time actively figuring it
> out, this could be casting anything to str. But even knowing this, just
> casting Optional away looks fishy anyway.
> 
> Looking up the ModuleSpec docs, it seems okay to assume that it's never
> None in our case, but wouldn't it be much cleaner to just 'assert
> qemu_spec.origin' first and then use it without the cast?
> 
> > +try:
> > +_ = 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])
> > +
> >  self.pythonpath = os.getenv('PYTHONPATH')
> >  self.pythonpath = os.pathsep.join(filter(None, (
> >  self.source_iotests,
> > @@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: 
> > str,
> >  
> >  self.build_root = os.path.join(self.build_iotests, '..', '..')
> >  
> > +self._logger = logging.getLogger('qemu.iotests')
> >  self.init_directories()
> >  self.init_binaries()
> 
> Looks good otherwise.

Well, actually there is a major problem: We'll pass the right PYTHONPATH
for the test scripts that we're calling, but this script itself doesn't
use it yet. So what I get is:

Traceback (most recent call last):
  File "/home/kwolf/source/qemu/tests/qemu-iotests/build/check", line 120, in 

env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
  File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 253, in 
__init__
self.init_directories()
  File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 120, in 
init_directories
qemu_spec = importlib.util.find_spec('qemu.qmp')
  File "/usr/lib64/python3.9/importlib/util.py", line 94, in find_spec
parent = __import__(parent_name, fromlist=['__path__'])
ModuleNotFoundError: No module named 'qemu'

Kevin

Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[Daniel P . Berrangé]

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 ?

> (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 = 'localhost:12345'
>  
> @@ -112,6 +114,22 @@ def init_directories(self) -> None:
>  # Path where qemu goodies live in this source tree.
>  qemu_srctree_path = Path(__file__, '../../../python').resolve()
>  
> +# warn if we happen to be able to find 'qemu' packages from an
> +# unexpected location (i.e. the package is already installed in
> +# the user's environment)
> +qemu_spec = importlib.util.find_spec('qemu.qmp')
> +if qemu_spec:
> +spec_path = Path(cast(str, qemu_spec.origin))
> +try:
> +_ = 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.

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 :|

Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[Kevin Wolf]

Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> 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.
> 
> (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 = 'localhost:12345'
>  
> @@ -112,6 +114,22 @@ def init_directories(self) -> None:
>  # Path where qemu goodies live in this source tree.
>  qemu_srctree_path = Path(__file__, '../../../python').resolve()
>  
> +# warn if we happen to be able to find 'qemu' packages from an
> +# unexpected location (i.e. the package is already installed in
> +# the user's environment)
> +qemu_spec = importlib.util.find_spec('qemu.qmp')
> +if qemu_spec:
> +spec_path = Path(cast(str, qemu_spec.origin))

You're casting Optional[str] to str here. The source type is not obvious
from the local code, so unless you spend some time actively figuring it
out, this could be casting anything to str. But even knowing this, just
casting Optional away looks fishy anyway.

Looking up the ModuleSpec docs, it seems okay to assume that it's never
None in our case, but wouldn't it be much cleaner to just 'assert
qemu_spec.origin' first and then use it without the cast?

> +try:
> +_ = 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])
> +
>  self.pythonpath = os.getenv('PYTHONPATH')
>  self.pythonpath = os.pathsep.join(filter(None, (
>  self.source_iotests,
> @@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: 
> str,
>  
>  self.build_root = os.path.join(self.build_iotests, '..', '..')
>  
> +self._logger = logging.getLogger('qemu.iotests')
>  self.init_directories()
>  self.init_binaries()

Looks good otherwise.

Kevin