qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [PATCH v2 2/6] iotests: add warning for rogue 'qemu' packages
Date: Fri, 24 Sep 2021 14:11:17 -0400



On Thu, Sep 23, 2021 at 4:27 PM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
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 ? :)


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


It's your code, I'd much rather defer to your judgment in terms of organization. I will make the change.
 

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


Hm, you didn't miss anything! I wrote this patch before the prior patch, and I didn't realize that PYTHONPATH supersedes the default sys.path internals. I thought it *appended* them. So uh. actually! with the previous change ... this patch isn't really needed at all ... ! We can just drop it entirely.
 
>
> 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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]