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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

23.09.2021 21:44, John Snow wrote:
> 
> 
> On Thu, Sep 23, 2021 at 2:32 PM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com <mailto:vsementsov@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 <jsnow@redhat.com <mailto:jsnow@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/ <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 ? :)

>      > +                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 doing.. I'm not sure, up to you.


Another question comes to my mind:

You say "'qemu' python packages will be imported from". But are you sure? We pass correct PYTHONPATH, where qemu_srctree_path goes first, doesn't it guarantee that qemu package will be loaded from it?

I now read in spec about PYTHONPATH:

   The default search path is installation dependent, but generally begins with prefix/lib/pythonversion (see PYTHONHOME above). It is always appended to PYTHONPATH.


So, if do warn something, it seems correct to say that "System version of qemu is {spec_path.parents[1]}, but sorry, we prefer our own (and better) version at {qemu_srctree_path}".

Or what I miss? In commit message it's not clean did you really see such problem or not :)

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


-- 
Best regards,
Vladimir