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

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

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


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