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

[Kevin Wolf <kwolf@redhat.com>]

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 <jsnow@redhat.com>
> ---
>  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 <http://www.gnu.org/licenses/>.
>  #
>  
> +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