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

2021-09-24 Thread John Snow 
On Thu, Sep 23, 2021 at 4:27 PM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> 23.09.2021 21:44, John Snow wrote:
> >
> >
> > On Thu, Sep 23, 2021 at 2:32 PM Vladimir Sementsov-Ogievskiy <
> vsement...@virtuozzo.com > wrote:
> >
> > 23.09.2021 21:07, 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.
> >  >
> >  > (I'm not going to say that this is because I bit myself with this,
> >  > but you can fill in the blanks.)
> >  >
> >  > In the future, we will pivot to always preferring a specific
> installed
> >  > instance of qemu python packages managed directly by iotests. For
> now
> >  > simply warn if there is an ambiguity over which instance that
> iotests
> >  > might use.
> >  >
> >  > Example: If a user has navigated to ~/src/qemu/python and executed
> >  > `pip install .`, you will see output like this when running
> `./check`:
> >  >
> >  > WARNING: 'qemu' python packages will be imported from outside the
> source tree ('/home/jsnow/src/qemu/python')
> >  >   Importing instead from
> '/home/jsnow/.local/lib/python3.9/site-packages/qemu'
> >  >
> >  > Signed-off-by: John Snow  js...@redhat.com>>
> >  > ---
> >  >   tests/qemu-iotests/testenv.py | 24 
> >  >   1 file changed, 24 insertions(+)
> >  >
> >  > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> >  > index 99a57a69f3a..1c0f6358538 100644
> >  > --- a/tests/qemu-iotests/testenv.py
> >  > +++ b/tests/qemu-iotests/testenv.py
> >  > @@ -16,6 +16,8 @@
> >  >   # along with this program.  If not, see <
> http://www.gnu.org/licenses/ >.
> >  >   #
> >  >
> >  > +import importlib.util
> >  > +import logging
> >  >   import os
> >  >   import sys
> >  >   import tempfile
> >  > @@ -112,6 +114,27 @@ 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 namespace
> packages
> >  > +# (using qemu.qmp as a bellwether) from an unexpected
> location.
> >  > +# i.e. the package is already installed in the user's
> environment.
> >  > +try:
> >  > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> >  > +except ModuleNotFoundError:
> >  > +qemu_spec = None
> >  > +
> >  > +if qemu_spec and qemu_spec.origin:
> >  > +spec_path = Path(qemu_spec.origin)
> >  > +try:
> >  > +_ = spec_path.relative_to(qemu_srctree_path)
> >
> > It took some time and looking at specification trying to understand
> what's going on here :)
> >
> > Could we just use:
> >
> > if not Path(qemu_spec.origin).is_relative_to(qemu_srctree_path):
> >  ... logging ...
> >
> >
> > Nope, that's 3.9+ only. (I made the same mistake.)
>
> Oh :(
>
> OK
>
> >
> >
> >  > +except ValueError:
> >
> >  > +self._logger.warning(
> >  > +"WARNING: 'qemu' python packages will be
> imported from"
> >  > +" outside the source tree ('%s')",
> >  > +qemu_srctree_path)
>
> why not use f-strings ? :)
>
>
The logging subsystem still uses % formatting by default, you can see I'm
not actually using the % operator to apply the values -- the Python docs
claim this is for "efficiency" because the string doesn't have to be
evaluated unless the logging statement is actually emitted, but that gain
sounds mostly theoretical to me, because Python still has eager evaluation
of passed arguments ... unless I've missed something.

Either way, using f-strings on logging calls gives you a pylint warning
that I'd just have to then disable, so I just skip the hassle.

Now, the logging system *does* allow you to use new-style python strings [
https://docs.python.org/3/howto/logging-cookbook.html#formatting-styles ]
but given that the default is still %-style and virtually every library
uses that style, I thought it might cause more problems than it creates by
trying to use a different style for just one portion of the codebase. Mind
you, I've never even bothered to try, so my apprehensions might not be
strictly factual ;)


> >  > +self._logger.warning(
> >  > +" Importing instead from '%s'",
> >  > +spec_path.parents[1])
> >  > +
> >
> > Also, I'd move this new chunk o

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

2021-09-23 Thread Vladimir Sementsov-Ogievskiy 

23.09.2021 21:44, John Snow wrote:



On Thu, Sep 23, 2021 at 2:32 PM Vladimir Sementsov-Ogievskiy mailto:vsement...@virtuozzo.com>> wrote:

23.09.2021 21:07, 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.
 >
 > (I'm not going to say that this is because I bit myself with this,
 > but you can fill in the blanks.)
 >
 > In the future, we will pivot to always preferring a specific installed
 > instance of qemu python packages managed directly by iotests. For now
 > simply warn if there is an ambiguity over which instance that iotests
 > might use.
 >
 > Example: If a user has navigated to ~/src/qemu/python and executed
 > `pip install .`, you will see output like this when running `./check`:
 >
 > WARNING: 'qemu' python packages will be imported from outside the source 
tree ('/home/jsnow/src/qemu/python')
 >           Importing instead from 
'/home/jsnow/.local/lib/python3.9/site-packages/qemu'
 >
 > Signed-off-by: John Snow mailto:js...@redhat.com>>
 > ---
 >   tests/qemu-iotests/testenv.py | 24 
 >   1 file changed, 24 insertions(+)
 >
 > diff --git a/tests/qemu-iotests/testenv.py 
b/tests/qemu-iotests/testenv.py
 > index 99a57a69f3a..1c0f6358538 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
 > @@ -112,6 +114,27 @@ 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 namespace packages
 > +        # (using qemu.qmp as a bellwether) from an unexpected location.
 > +        # i.e. the package is already installed in the user's 
environment.
 > +        try:
 > +            qemu_spec = importlib.util.find_spec('qemu.qmp')
 > +        except ModuleNotFoundError:
 > +            qemu_spec = None
 > +
 > +        if qemu_spec and qemu_spec.origin:
 > +            spec_path = Path(qemu_spec.origin)
 > +            try:
 > +                _ = spec_path.relative_to(qemu_srctree_path)

It took some time and looking at specification trying to understand what's 
going on here :)

Could we just use:

if not Path(qemu_spec.origin).is_relative_to(qemu_srctree_path):
     ... logging ...


Nope, that's 3.9+ only. (I made the same mistake.)


Oh :(

OK




 > +            except ValueError:

 > +                self._logger.warning(
 > +                    "WARNING: 'qemu' python packages will be imported 
from"
 > +                    " outside the source tree ('%s')",
 > +                    qemu_srctree_path)


why not use f-strings ? :)


 > +                self._logger.warning(
 > +                    "         Importing instead from '%s'",
 > +                    spec_path.parents[1])
 > +

Also, I'd move this new chunk of code to a separate function (may be even 
out of class, as the only usage of self is self._logger, which you introduce 
with this patch. Still a method would be OK too). And then, just call it from 
__init__(). Just to keep init_directories() simpler. And with this new code we 
don't init any directories to pass to further test execution, it's just a check 
for runtime environment.


I wanted to keep the wiggly python import logic all in one place so that it was 
harder to accidentally forget a piece of it if/when we adjust it.


Hmm right.. I didn't look from that point of view.

So, we actually check the library we are using now is the same which we pass to 
called tests.

So, it's a right place for it. And it's about the fact that we are still 
hacking around importing libraries :) Hope for bright future.


I can create a standalone function for it, but I'd need to stash that 
qemu_srctree_path variable somewhere if we want to call that runtime check from 
somewhere else, because I don't want to compute it twice. Is it still worth 
doing in your opinion if I just create a method/function and pass it the 
qemu_srctree_path variable straight from init_directories()?


My first impression was that init_directories() is not a right place. But now I 
see that we want to check exactly this qemu_srctree_path, which we are going to 
pass to tests.

So, I'm OK as is.

Still, may be adding helper function like

def warn_if_module_loads_not_from(module_name, path):

worth d

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

2021-09-23 Thread John Snow 
On Thu, Sep 23, 2021 at 2:32 PM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> 23.09.2021 21:07, 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.
> >
> > (I'm not going to say that this is because I bit myself with this,
> > but you can fill in the blanks.)
> >
> > In the future, we will pivot to always preferring a specific installed
> > instance of qemu python packages managed directly by iotests. For now
> > simply warn if there is an ambiguity over which instance that iotests
> > might use.
> >
> > Example: If a user has navigated to ~/src/qemu/python and executed
> > `pip install .`, you will see output like this when running `./check`:
> >
> > WARNING: 'qemu' python packages will be imported from outside the source
> tree ('/home/jsnow/src/qemu/python')
> >   Importing instead from
> '/home/jsnow/.local/lib/python3.9/site-packages/qemu'
> >
> > Signed-off-by: John Snow 
> > ---
> >   tests/qemu-iotests/testenv.py | 24 
> >   1 file changed, 24 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> > index 99a57a69f3a..1c0f6358538 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
> > @@ -112,6 +114,27 @@ 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 namespace packages
> > +# (using qemu.qmp as a bellwether) from an unexpected location.
> > +# i.e. the package is already installed in the user's
> environment.
> > +try:
> > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> > +except ModuleNotFoundError:
> > +qemu_spec = None
> > +
> > +if qemu_spec and qemu_spec.origin:
> > +spec_path = Path(qemu_spec.origin)
> > +try:
> > +_ = spec_path.relative_to(qemu_srctree_path)
>
> It took some time and looking at specification trying to understand what's
> going on here :)
>
> Could we just use:
>
> if not Path(qemu_spec.origin).is_relative_to(qemu_srctree_path):
> ... logging ...
>
>
Nope, that's 3.9+ only. (I made the same mistake.)


> > +except ValueError:
>
> +self._logger.warning(
> > +"WARNING: 'qemu' python packages will be imported
> from"
> > +" outside the source tree ('%s')",
> > +qemu_srctree_path)
> > +self._logger.warning(
> > +" Importing instead from '%s'",
> > +spec_path.parents[1])
> > +
>
> Also, I'd move this new chunk of code to a separate function (may be even
> out of class, as the only usage of self is self._logger, which you
> introduce with this patch. Still a method would be OK too). And then, just
> call it from __init__(). Just to keep init_directories() simpler. And with
> this new code we don't init any directories to pass to further test
> execution, it's just a check for runtime environment.
>
>
I wanted to keep the wiggly python import logic all in one place so that it
was harder to accidentally forget a piece of it if/when we adjust it. I can
create a standalone function for it, but I'd need to stash that
qemu_srctree_path variable somewhere if we want to call that runtime check
from somewhere else, because I don't want to compute it twice. Is it still
worth doing in your opinion if I just create a method/function and pass it
the qemu_srctree_path variable straight from init_directories()?

Not adding _logger is valid, though. I almost removed it myself. I'll
squash that in.


> >   self.pythonpath = os.pathsep.join(filter(None, (
> >   self.source_iotests,
> >   str(qemu_srctree_path),
> > @@ -230,6 +253,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()
> >
> >
>
>
> --
> Best regards,
> Vladimir
>
>

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

2021-09-23 Thread Vladimir Sementsov-Ogievskiy 

23.09.2021 21:07, 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.

(I'm not going to say that this is because I bit myself with this,
but you can fill in the blanks.)

In the future, we will pivot to always preferring a specific installed
instance of qemu python packages managed directly by iotests. For now
simply warn if there is an ambiguity over which instance that iotests
might use.

Example: If a user has navigated to ~/src/qemu/python and executed
`pip install .`, you will see output like this when running `./check`:

WARNING: 'qemu' python packages will be imported from outside the source tree 
('/home/jsnow/src/qemu/python')
  Importing instead from 
'/home/jsnow/.local/lib/python3.9/site-packages/qemu'

Signed-off-by: John Snow 
---
  tests/qemu-iotests/testenv.py | 24 
  1 file changed, 24 insertions(+)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 99a57a69f3a..1c0f6358538 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
@@ -112,6 +114,27 @@ 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 namespace packages

+# (using qemu.qmp as a bellwether) from an unexpected location.
+# i.e. the package is already installed in the user's environment.
+try:
+qemu_spec = importlib.util.find_spec('qemu.qmp')
+except ModuleNotFoundError:
+qemu_spec = None
+
+if qemu_spec and qemu_spec.origin:
+spec_path = Path(qemu_spec.origin)
+try:
+_ = spec_path.relative_to(qemu_srctree_path)


It took some time and looking at specification trying to understand what's 
going on here :)

Could we just use:

if not Path(qemu_spec.origin).is_relative_to(qemu_srctree_path):
   ... logging ...



+except ValueError:
+self._logger.warning(
+"WARNING: 'qemu' python packages will be imported from"
+" outside the source tree ('%s')",
+qemu_srctree_path)
+self._logger.warning(
+" Importing instead from '%s'",
+spec_path.parents[1])
+


Also, I'd move this new chunk of code to a separate function (may be even out 
of class, as the only usage of self is self._logger, which you introduce with 
this patch. Still a method would be OK too). And then, just call it from 
__init__(). Just to keep init_directories() simpler. And with this new code we 
don't init any directories to pass to further test execution, it's just a check 
for runtime environment.


  self.pythonpath = os.pathsep.join(filter(None, (
  self.source_iotests,
  str(qemu_srctree_path),
@@ -230,6 +253,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()
  




--
Best regards,
Vladimir

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

2021-09-23 Thread John Snow 
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.)

In the future, we will pivot to always preferring a specific installed
instance of qemu python packages managed directly by iotests. For now
simply warn if there is an ambiguity over which instance that iotests
might use.

Example: If a user has navigated to ~/src/qemu/python and executed
`pip install .`, you will see output like this when running `./check`:

WARNING: 'qemu' python packages will be imported from outside the source tree 
('/home/jsnow/src/qemu/python')
 Importing instead from 
'/home/jsnow/.local/lib/python3.9/site-packages/qemu'

Signed-off-by: John Snow 
---
 tests/qemu-iotests/testenv.py | 24 
 1 file changed, 24 insertions(+)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 99a57a69f3a..1c0f6358538 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
@@ -112,6 +114,27 @@ 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 namespace packages
+# (using qemu.qmp as a bellwether) from an unexpected location.
+# i.e. the package is already installed in the user's environment.
+try:
+qemu_spec = importlib.util.find_spec('qemu.qmp')
+except ModuleNotFoundError:
+qemu_spec = None
+
+if qemu_spec and qemu_spec.origin:
+spec_path = Path(qemu_spec.origin)
+try:
+_ = spec_path.relative_to(qemu_srctree_path)
+except ValueError:
+self._logger.warning(
+"WARNING: 'qemu' python packages will be imported from"
+" outside the source tree ('%s')",
+qemu_srctree_path)
+self._logger.warning(
+" Importing instead from '%s'",
+spec_path.parents[1])
+
 self.pythonpath = os.pathsep.join(filter(None, (
 self.source_iotests,
 str(qemu_srctree_path),
@@ -230,6 +253,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()
 
-- 
2.31.1