[PATCH v2 0/6] iotests: update environment and linting configuration

[PATCH v2 0/6] iotests: update environment and linting configuration

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt1
CI: https://gitlab.com/jsnow/qemu/-/pipelines/376236687

This series partially supersedes:
  [PATCH v3 00/16] python/iotests: Run iotest linters during Python CI'

Howdy, this is good stuff we want even if we aren't yet in agreement
about the best way to run iotest 297 from CI.

- Update linting config to tolerate pylint 2.11.1
- Eliminate sys.path hacking in individual test files
- make mypy execution in test 297 faster

The rest of the actual "run at CI time" stuff can get handled separately
and later pending some discussion on the other series.

V2:

001/6:[0011] [FC] 'iotests: add 'qemu' package location to PYTHONPATH in testenv'
002/6:[0025] [FC] 'iotests: add warning for rogue 'qemu' packages'

- Squashed in a small optimization from Vladimir to 001, kept R-Bs.
- Fixed the package detection logic to not panic if it can't find
  'qemu' at all (kwolf)
- Updated commit messages for the first two patches.

--js

John Snow (6):
  iotests: add 'qemu' package location to PYTHONPATH in testenv
  iotests: add warning for rogue 'qemu' packages
  iotests/linters: check mypy files all at once
  iotests/mirror-top-perms: Adjust imports
  iotests/migrate-bitmaps-test: delint
  iotests: Update for pylint 2.11.1

 tests/qemu-iotests/235                        |  2 -
 tests/qemu-iotests/297                        | 50 ++++++++-----------
 tests/qemu-iotests/300                        |  7 ++-
 tests/qemu-iotests/iotests.py                 |  2 -
 tests/qemu-iotests/pylintrc                   |  6 ++-
 tests/qemu-iotests/testenv.py                 | 39 ++++++++++++---
 tests/qemu-iotests/testrunner.py              |  7 +--
 tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++++++++++--------
 tests/qemu-iotests/tests/mirror-top-perms     | 12 ++---
 9 files changed, 99 insertions(+), 76 deletions(-)

-- 
2.31.1

[PATCH v2 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv

[John Snow]

We can drop the sys.path hacking in various places by doing
this. Additionally, by doing it in one place right up top, we can print
interesting warnings in case the environment does not look correct. (See
next commit.)

If we ever decide to change how the environment is crafted, all of the
"help me find my python packages" goop is all in one place, right in one
function.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/235                    |  2 --
 tests/qemu-iotests/297                    |  6 ------
 tests/qemu-iotests/300                    |  7 +++----
 tests/qemu-iotests/iotests.py             |  2 --
 tests/qemu-iotests/testenv.py             | 15 +++++++++------
 tests/qemu-iotests/tests/mirror-top-perms |  7 +++----
 6 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index 8aed45f9a76..4de920c3801 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -24,8 +24,6 @@ import os
 import iotests
 from iotests import qemu_img_create, qemu_io, file_path, log
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-
 from qemu.machine import QEMUMachine
 
 iotests.script_initialize(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b04cba53667..467b712280e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -68,12 +68,6 @@ def run_linters():
     # Todo notes are fine, but fixme's or xxx's should probably just be
     # fixed (in tests, at least)
     env = os.environ.copy()
-    qemu_module_path = os.path.join(os.path.dirname(__file__),
-                                    '..', '..', 'python')
-    try:
-        env['PYTHONPATH'] += os.pathsep + qemu_module_path
-    except KeyError:
-        env['PYTHONPATH'] = qemu_module_path
     subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
                    env=env, check=False)
 
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index fe94de84edd..10f9f2a8da6 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -24,12 +24,11 @@ import random
 import re
 from typing import Dict, List, Optional
 
-import iotests
-
-# Import qemu after iotests.py has amended sys.path
-# pylint: disable=wrong-import-order
 from qemu.machine import machine
 
+import iotests
+
+
 BlockBitmapMapping = List[Dict[str, object]]
 
 mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ce06cf56304..b06ad76e0c5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -36,8 +36,6 @@
 
 from contextlib import contextmanager
 
-# pylint: disable=import-error, wrong-import-position
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.machine import qtest
 from qemu.qmp import QMPMessage
 
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 70da0d60c80..99a57a69f3a 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -108,12 +108,15 @@ def init_directories(self) -> None:
              SAMPLE_IMG_DIR
              OUTPUT_DIR
         """
-        self.pythonpath = os.getenv('PYTHONPATH')
-        if self.pythonpath:
-            self.pythonpath = self.source_iotests + os.pathsep + \
-                self.pythonpath
-        else:
-            self.pythonpath = self.source_iotests
+
+        # Path where qemu goodies live in this source tree.
+        qemu_srctree_path = Path(__file__, '../../../python').resolve()
+
+        self.pythonpath = os.pathsep.join(filter(None, (
+            self.source_iotests,
+            str(qemu_srctree_path),
+            os.getenv('PYTHONPATH'),
+        )))
 
         self.test_dir = os.getenv('TEST_DIR',
                                   os.path.join(os.getcwd(), 'scratch'))
diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index 2fc8dd66e0a..73138a0ef91 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -20,13 +20,12 @@
 #
 
 import os
+
+import qemu
+
 import iotests
 from iotests import qemu_img
 
-# Import qemu after iotests.py has amended sys.path
-# pylint: disable=wrong-import-order
-import qemu
-
 
 image_size = 1 * 1024 * 1024
 source = os.path.join(iotests.test_dir, 'source.img')
-- 
2.31.1

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

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

[PATCH v2 3/6] iotests/linters: check mypy files all at once

[John Snow]

We can circumvent the '__main__' redefinition problem by passing
--scripts-are-modules. Take mypy out of the loop per-filename and check
everything in one go: it's quite a bit faster.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/297 | 44 +++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 467b712280e..91ec34d9521 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -74,32 +74,28 @@ def run_linters():
     print('=== mypy ===')
     sys.stdout.flush()
 
-    # We have to call mypy separately for each file.  Otherwise, it
-    # will interpret all given files as belonging together (i.e., they
-    # may not both define the same classes, etc.; most notably, they
-    # must not both define the __main__ module).
     env['MYPYPATH'] = env['PYTHONPATH']
-    for filename in files:
-        p = subprocess.run(('mypy',
-                            '--warn-unused-configs',
-                            '--disallow-subclassing-any',
-                            '--disallow-any-generics',
-                            '--disallow-incomplete-defs',
-                            '--disallow-untyped-decorators',
-                            '--no-implicit-optional',
-                            '--warn-redundant-casts',
-                            '--warn-unused-ignores',
-                            '--no-implicit-reexport',
-                            '--namespace-packages',
-                            filename),
-                           env=env,
-                           check=False,
-                           stdout=subprocess.PIPE,
-                           stderr=subprocess.STDOUT,
-                           universal_newlines=True)
+    p = subprocess.run(('mypy',
+                        '--warn-unused-configs',
+                        '--disallow-subclassing-any',
+                        '--disallow-any-generics',
+                        '--disallow-incomplete-defs',
+                        '--disallow-untyped-decorators',
+                        '--no-implicit-optional',
+                        '--warn-redundant-casts',
+                        '--warn-unused-ignores',
+                        '--no-implicit-reexport',
+                        '--namespace-packages',
+                        '--scripts-are-modules',
+                        *files),
+                       env=env,
+                       check=False,
+                       stdout=subprocess.PIPE,
+                       stderr=subprocess.STDOUT,
+                       universal_newlines=True)
 
-        if p.returncode != 0:
-            print(p.stdout)
+    if p.returncode != 0:
+        print(p.stdout)
 
 
 for linter in ('pylint-3', 'mypy'):
-- 
2.31.1

[PATCH v2 4/6] iotests/mirror-top-perms: Adjust imports

[John Snow]

We need to import subpackages from the qemu namespace package; importing
the namespace package alone doesn't bring the subpackages with it --
unless someone else (like iotests.py) imports them too.

Adjust the imports.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/tests/mirror-top-perms | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index 73138a0ef91..3d475aa3a54 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,7 +21,8 @@
 
 import os
 
-import qemu
+from qemu import qmp
+from qemu.machine import machine
 
 import iotests
 from iotests import qemu_img
@@ -46,7 +47,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
     def tearDown(self):
         try:
             self.vm.shutdown()
-        except qemu.machine.machine.AbnormalShutdown:
+        except machine.AbnormalShutdown:
             pass
 
         if self.vm_b is not None:
@@ -101,7 +102,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
             self.vm_b.launch()
             print('ERROR: VM B launched successfully, this should not have '
                   'happened')
-        except qemu.qmp.QMPConnectError:
+        except qmp.QMPConnectError:
             assert 'Is another process using the image' in self.vm_b.get_log()
 
         result = self.vm.qmp('block-job-cancel',
-- 
2.31.1

[PATCH v2 5/6] iotests/migrate-bitmaps-test: delint

[John Snow]

Mostly uninteresting stuff. Move the test injections under a function
named main() so that the variables used during that process aren't in
the global scope.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++++++++++--------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test b/tests/qemu-iotests/tests/migrate-bitmaps-test
index dc431c35b35..c23df3d75c7 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-test
@@ -19,10 +19,11 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
-import os
 import itertools
 import operator
+import os
 import re
+
 import iotests
 from iotests import qemu_img, qemu_img_create, Timeout
 
@@ -224,25 +225,6 @@ def inject_test_case(klass, suffix, method, *args, **kwargs):
     setattr(klass, 'test_' + method + suffix, lambda self: mc(self))
 
 
-for cmb in list(itertools.product((True, False), repeat=5)):
-    name = ('_' if cmb[0] else '_not_') + 'persistent_'
-    name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
-    name += '_online' if cmb[2] else '_offline'
-    name += '_shared' if cmb[3] else '_nonshared'
-    if cmb[4]:
-        name += '__pre_shutdown'
-
-    inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
-                     *list(cmb))
-
-for cmb in list(itertools.product((True, False), repeat=2)):
-    name = ('_' if cmb[0] else '_not_') + 'persistent_'
-    name += ('_' if cmb[1] else '_not_') + 'migbitmap'
-
-    inject_test_case(TestDirtyBitmapMigration, name,
-                     'do_test_migration_resume_source', *list(cmb))
-
-
 class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
     def setUp(self):
         qemu_img_create('-f', iotests.imgfmt, base_a, size)
@@ -304,6 +286,30 @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
 
+def main() -> None:
+    for cmb in list(itertools.product((True, False), repeat=5)):
+        name = ('_' if cmb[0] else '_not_') + 'persistent_'
+        name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
+        name += '_online' if cmb[2] else '_offline'
+        name += '_shared' if cmb[3] else '_nonshared'
+        if cmb[4]:
+            name += '__pre_shutdown'
+
+        inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
+                         *list(cmb))
+
+    for cmb in list(itertools.product((True, False), repeat=2)):
+        name = ('_' if cmb[0] else '_not_') + 'persistent_'
+        name += ('_' if cmb[1] else '_not_') + 'migbitmap'
+
+        inject_test_case(TestDirtyBitmapMigration, name,
+                         'do_test_migration_resume_source', *list(cmb))
+
+    iotests.main(
+        supported_fmts=['qcow2'],
+        supported_protocols=['file']
+    )
+
+
 if __name__ == '__main__':
-    iotests.main(supported_fmts=['qcow2'],
-                 supported_protocols=['file'])
+    main()
-- 
2.31.1

[PATCH v2 6/6] iotests: Update for pylint 2.11.1

[John Snow]

1. Ignore the new f-strings warning, we're not interested in doing a
   full conversion at this time.

2. Just mute the unbalanced-tuple-unpacking warning, it's not a real
   error in this case and muting the dozens of callsites is just not
   worth it.

3. Add encodings to read_text().

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/pylintrc      | 6 +++++-
 tests/qemu-iotests/testrunner.py | 7 ++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index f2c0b522ac0..8cb4e1d6a6d 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -19,13 +19,17 @@ disable=invalid-name,
         too-many-public-methods,
         # pylint warns about Optional[] etc. as unsubscriptable in 3.9
         unsubscriptable-object,
+        # pylint's static analysis causes false positivies for file_path();
+        # If we really care to make it statically knowable, we'll use mypy.
+        unbalanced-tuple-unpacking,
         # Sometimes we need to disable a newly introduced pylint warning.
         # Doing so should not produce a warning in older versions of pylint.
         bad-option-value,
         # These are temporary, and should be removed:
         missing-docstring,
         too-many-return-statements,
-        too-many-statements
+        too-many-statements,
+        consider-using-f-string,
 
 [FORMAT]
 
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 4a6ec421ed6..a56b6da3968 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -266,12 +266,13 @@ def do_run_test(self, test: str) -> TestResult:
                               diff=file_diff(str(f_reference), str(f_bad)))
 
         if f_notrun.exists():
-            return TestResult(status='not run',
-                              description=f_notrun.read_text().strip())
+            return TestResult(
+                status='not run',
+                description=f_notrun.read_text(encoding='utf-8').strip())
 
         casenotrun = ''
         if f_casenotrun.exists():
-            casenotrun = f_casenotrun.read_text()
+            casenotrun = f_casenotrun.read_text(encoding='utf-8')
 
         diff = file_diff(str(f_reference), str(f_bad))
         if diff:
-- 
2.31.1

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

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

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

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 4683 bytes --]

On Thu, Sep 23, 2021 at 2:32 PM Vladimir Sementsov-Ogievskiy <
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>
> > ---
> >   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.)


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

[-- Attachment #2: Type: text/html, Size: 6546 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

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

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

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 8519 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 11615 bytes --]

Re: [PATCH v2 0/6] iotests: update environment and linting configuration

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 2185 bytes --]

On Thu, Sep 23, 2021 at 2:07 PM John Snow <jsnow@redhat.com> wrote:

> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt1
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/376236687
>
> This series partially supersedes:
>   [PATCH v3 00/16] python/iotests: Run iotest linters during Python CI'
>
> Howdy, this is good stuff we want even if we aren't yet in agreement
> about the best way to run iotest 297 from CI.
>
> - Update linting config to tolerate pylint 2.11.1
> - Eliminate sys.path hacking in individual test files
> - make mypy execution in test 297 faster
>
> The rest of the actual "run at CI time" stuff can get handled separately
> and later pending some discussion on the other series.
>
> V2:
>
> 001/6:[0011] [FC] 'iotests: add 'qemu' package location to PYTHONPATH in
> testenv'
> 002/6:[0025] [FC] 'iotests: add warning for rogue 'qemu' packages'
>
> - Squashed in a small optimization from Vladimir to 001, kept R-Bs.
> - Fixed the package detection logic to not panic if it can't find
>   'qemu' at all (kwolf)
> - Updated commit messages for the first two patches.
>
> --js
>
> John Snow (6):
>   iotests: add 'qemu' package location to PYTHONPATH in testenv
>   iotests: add warning for rogue 'qemu' packages
>   iotests/linters: check mypy files all at once
>   iotests/mirror-top-perms: Adjust imports
>   iotests/migrate-bitmaps-test: delint
>   iotests: Update for pylint 2.11.1
>
>  tests/qemu-iotests/235                        |  2 -
>  tests/qemu-iotests/297                        | 50 ++++++++-----------
>  tests/qemu-iotests/300                        |  7 ++-
>  tests/qemu-iotests/iotests.py                 |  2 -
>  tests/qemu-iotests/pylintrc                   |  6 ++-
>  tests/qemu-iotests/testenv.py                 | 39 ++++++++++++---
>  tests/qemu-iotests/testrunner.py              |  7 +--
>  tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++++++++++--------
>  tests/qemu-iotests/tests/mirror-top-perms     | 12 ++---
>  9 files changed, 99 insertions(+), 76 deletions(-)
>
> --
> 2.31.1
>
>
>
Patch 2 can just be dropped, and everything else is reviewed, so I think
this can be staged at your leisure.

--js

[-- Attachment #2: Type: text/html, Size: 3089 bytes --]

Re: [PATCH v2 0/6] iotests: update environment and linting configuration

[Kevin Wolf]

Am 24.09.2021 um 20:13 hat John Snow geschrieben:
> On Thu, Sep 23, 2021 at 2:07 PM John Snow <jsnow@redhat.com> wrote:
> 
> > GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt1
> > CI: https://gitlab.com/jsnow/qemu/-/pipelines/376236687
> >
> > This series partially supersedes:
> >   [PATCH v3 00/16] python/iotests: Run iotest linters during Python CI'
> >
> > Howdy, this is good stuff we want even if we aren't yet in agreement
> > about the best way to run iotest 297 from CI.
> >
> > - Update linting config to tolerate pylint 2.11.1
> > - Eliminate sys.path hacking in individual test files
> > - make mypy execution in test 297 faster
> >
> > The rest of the actual "run at CI time" stuff can get handled separately
> > and later pending some discussion on the other series.
> >
> > V2:
> >
> > 001/6:[0011] [FC] 'iotests: add 'qemu' package location to PYTHONPATH in
> > testenv'
> > 002/6:[0025] [FC] 'iotests: add warning for rogue 'qemu' packages'
> >
> > - Squashed in a small optimization from Vladimir to 001, kept R-Bs.
> > - Fixed the package detection logic to not panic if it can't find
> >   'qemu' at all (kwolf)
> > - Updated commit messages for the first two patches.

> Patch 2 can just be dropped, and everything else is reviewed, so I think
> this can be staged at your leisure.

Thanks, applied to the block branch (without patch 2).

Kevin