[PATCH v3 00/16] python/iotests: Run iotest linters during Python CI

[PATCH v3 00/16] python/iotests: Run iotest linters during Python CI

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest
CI: https://gitlab.com/jsnow/qemu/-/pipelines/371611883
Based-On: <20210915175318.853225-1-hreitz@redhat.com>
          "[PULL 00/32] Block patches"

Since iotests are such a heavy and prominent user of the Python qemu.qmp
and qemu.machine packages, it would be convenient if the Python linting
suite also checked this client for any possible regressions introduced
by shifting around signatures, types, or interfaces in these packages.

(We'd eventually find those problems when iotest 297 ran, but with
increasing distance between Python development and Block development,
the risk of an accidental breakage in this regard increases. I,
personally, know to run iotests (and especially 297) after changing
Python code, but not everyone in the future might. Plus, I am lazy, and
I like only having to push one button.)

Add the ability for the Python CI to run the iotest linters too, which
means that the iotest linters would be checked against:

- Python 3.6, using a frozen set of linting packages at their oldest
  supported versions, using 'pipenv'
- Python 3.6 through Python 3.10 inclusive, using 'tox' and the latest
  versions of mypy/pylint that happen to be installed during test
  time. This CI test is allowed to fail with a warning, and can serve
  as a bellwether for when new incompatible changes may disrupt the
  linters. Testing against old and new Python interpreters alike can
  help surface incompatibility issues we may need to be aware of.)

Here are example outputs of those CI jobs with this series applied:
 - "check-python-pipenv": https://gitlab.com/jsnow/qemu/-/jobs/1377735087
 - "check-python-tox": https://gitlab.com/jsnow/qemu/-/jobs/1377735088

You can also run these same tests locally from ./python, plus one more:

- "make check-dev" to test against whatever python you have.
- "make check-pipenv", if you have Python 3.6 and pipenv installed.
- "make check-tox", to test against multiple python versions you have installed,
                    from 3.6 to 3.10 inclusive. (CI tests against all 5.)

See the old commit message for more sample output, etc.

https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg07056.html

V3:
 - Added patch 1 which has been submitted separately upstream,
   but was necessary for testing.
 - Rebased on top of hreitz/block, which fixed some linting issues.
 - Added a workaround for a rather nasty mypy bug ... >:(

V2:
 - Added patches 1-5 which do some more delinting.
 - Added patch 8, which scans subdirs for tests to lint.
 - Added patch 17, which improves the speed of mypy analysis.
 - Patch 14 is different because of the new patch 8.

John Snow (16):
  python: Update for pylint 2.10
  iotests/mirror-top-perms: Adjust imports
  iotests/migrate-bitmaps-postcopy-test: declare instance variables
  iotests/migrate-bitmaps-test: delint
  iotests/297: modify is_python_file to work from any CWD
  iotests/297: Add get_files() function
  iotests/297: Don't rely on distro-specific linter binaries
  iotests/297: Create main() function
  iotests/297: Separate environment setup from test execution
  iotests/297: Add 'directory' argument to run_linters
  iotests/297: return error code from run_linters()
  iotests/297: split linters.py off from 297
  iotests/linters: Add entry point for Python CI linters
  iotests/linters: Add workaround for mypy bug #9852
  python: Add iotest linters to test suite
  iotests/linters: check mypy files all at once

 python/qemu/machine/machine.py                |   9 +-
 python/setup.cfg                              |   1 +
 python/tests/iotests.sh                       |   4 +
 tests/qemu-iotests/297                        |  81 ++---------
 tests/qemu-iotests/linters.py                 | 129 ++++++++++++++++++
 .../tests/migrate-bitmaps-postcopy-test       |   3 +
 tests/qemu-iotests/tests/migrate-bitmaps-test |  50 ++++---
 tests/qemu-iotests/tests/mirror-top-perms     |   7 +-
 8 files changed, 186 insertions(+), 98 deletions(-)
 create mode 100755 python/tests/iotests.sh
 create mode 100755 tests/qemu-iotests/linters.py

-- 
2.31.1

[PATCH v3 01/16] python: Update for pylint 2.10

[John Snow]

A few new annoyances. Of note is the new warning for an unspecified
encoding when opening a text file, which actually does indicate a
potentially real problem; see
https://www.python.org/dev/peps/pep-0597/#motivation

Use LC_CTYPE to determine an encoding to use for interpreting QEMU's
terminal output. Note that Python states: "language code and encoding
may be None if their values cannot be determined" -- use a platform
default as a backup.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 9 ++++++++-
 python/setup.cfg               | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a7081b1845..51b6e79a13 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -19,6 +19,7 @@
 
 import errno
 from itertools import chain
+import locale
 import logging
 import os
 import shutil
@@ -290,8 +291,14 @@ def get_pid(self) -> Optional[int]:
         return self._subp.pid
 
     def _load_io_log(self) -> None:
+        # Assume that the output encoding of QEMU's terminal output
+        # is defined by our locale. If indeterminate, use a platform default.
+        _, encoding = locale.getlocale()
+        if encoding is None:
+            encoding = locale.getpreferredencoding(do_setlocale=False)
         if self._qemu_log_path is not None:
-            with open(self._qemu_log_path, "r") as iolog:
+            with open(self._qemu_log_path, "r",
+                      encoding=encoding) as iolog:
                 self._iolog = iolog.read()
 
     @property
diff --git a/python/setup.cfg b/python/setup.cfg
index 83909c1c97..0f0cab098f 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -104,6 +104,7 @@ good-names=i,
 [pylint.similarities]
 # Ignore imports when computing similarities.
 ignore-imports=yes
+ignore-signatures=yes
 
 # Minimum lines number of a similarity.
 # TODO: Remove after we opt in to Pylint 2.8.3. See commit msg.
-- 
2.31.1

[PATCH v3 02/16] iotests/mirror-top-perms: Adjust imports

[John Snow]

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

Adjust the imports.

Signed-off-by: John Snow <jsnow@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 2fc8dd66e0..de18182590 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -25,7 +25,8 @@ from iotests import qemu_img
 
 # Import qemu after iotests.py has amended sys.path
 # pylint: disable=wrong-import-order
-import qemu
+from qemu import qmp
+from qemu.machine import machine
 
 
 image_size = 1 * 1024 * 1024
@@ -47,7 +48,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:
@@ -102,7 +103,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 v3 03/16] iotests/migrate-bitmaps-postcopy-test: declare instance variables

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
index 00ebb5c251..9c00ae61c8 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
@@ -115,6 +115,9 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
         self.vm_a_events = []
         self.vm_b_events = []
 
+        self.discards1_sha256: str
+        self.all_discards_sha256: str
+
     def start_postcopy(self):
         """ Run migration until RESUME event on target. Return this event. """
         for i in range(nb_bitmaps):
-- 
2.31.1

[PATCH v3 04/16] 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>
---
 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 dc431c35b3..c23df3d75c 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 v3 05/16] iotests/297: modify is_python_file to work from any CWD

[John Snow]

Add a directory argument to is_python_file to allow it to work correctly
no matter what CWD we happen to run it from. This is done in
anticipation of running the iotests from another directory (./python/).

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b04cba5366..3e86f788fc 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -39,14 +39,16 @@ SKIP_FILES = (
 )
 
 
-def is_python_file(filename):
-    if not os.path.isfile(filename):
+def is_python_file(filename: str, directory: str = '.') -> bool:
+    filepath = os.path.join(directory, filename)
+
+    if not os.path.isfile(filepath):
         return False
 
     if filename.endswith('.py'):
         return True
 
-    with open(filename, encoding='utf-8') as f:
+    with open(filepath, encoding='utf-8') as f:
         try:
             first_line = f.readline()
             return re.match('^#!.*python', first_line) is not None
-- 
2.31.1

[PATCH v3 06/16] iotests/297: Add get_files() function

[John Snow]

Split out file discovery into its own method to begin separating out the
"environment setup" and "test execution" phases.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297 | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 3e86f788fc..b3a56a3a29 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,6 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
+from typing import List
 
 import iotests
 
@@ -56,10 +57,15 @@ def is_python_file(filename: str, directory: str = '.') -> bool:
             return False
 
 
+def get_test_files(directory: str = '.') -> List[str]:
+    named_test_dir = os.path.join(directory, 'tests')
+    named_tests = [f"tests/{entry}" for entry in os.listdir(named_test_dir)]
+    check_tests = set(os.listdir(directory) + named_tests) - set(SKIP_FILES)
+    return list(filter(lambda f: is_python_file(f, directory), check_tests))
+
+
 def run_linters():
-    named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
-    check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-    files = [filename for filename in check_tests if is_python_file(filename)]
+    files = get_test_files()
 
     iotests.logger.debug('Files to be checked:')
     iotests.logger.debug(', '.join(sorted(files)))
-- 
2.31.1

[PATCH v3 07/16] iotests/297: Don't rely on distro-specific linter binaries

[John Snow]

'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
mypy" to access these scripts instead. This style of invocation will
prefer the "correct" tool when run in a virtual environment.

Note that we still check for "pylint-3" before the test begins -- this
check is now "overly strict", but shouldn't cause anything that was
already running correctly to start failing.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qemu-iotests/297 | 45 ++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b3a56a3a29..01dd8147d5 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -82,8 +82,11 @@ def run_linters():
         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)
+    subprocess.run(
+        ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+        env=env,
+        check=False,
+    )
 
     print('=== mypy ===')
     sys.stdout.flush()
@@ -94,23 +97,27 @@ def run_linters():
     # 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(
+            (
+                'python3', '-m', '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
+        )
 
         if p.returncode != 0:
             print(p.stdout)
-- 
2.31.1

[PATCH v3 08/16] iotests/297: Create main() function

[John Snow]

Instead of running "run_linters" directly, create a main() function that
will be responsible for environment setup, leaving run_linters()
responsible only for execution of the linters.

(That environment setup will be moved over in forthcoming commits.)

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 01dd8147d5..6df952808d 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -123,8 +123,12 @@ def run_linters():
             print(p.stdout)
 
 
-for linter in ('pylint-3', 'mypy'):
-    if shutil.which(linter) is None:
-        iotests.notrun(f'{linter} not found')
+def main() -> None:
+    for linter in ('pylint-3', 'mypy'):
+        if shutil.which(linter) is None:
+            iotests.notrun(f'{linter} not found')
 
-iotests.script_main(run_linters)
+    run_linters()
+
+
+iotests.script_main(main)
-- 
2.31.1

[PATCH v3 09/16] iotests/297: Separate environment setup from test execution

[John Snow]

Move environment setup into main(), leaving pure test execution behind
in run_linters().

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 6df952808d..08d2b87108 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,7 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
-from typing import List
+from typing import List, Mapping, Optional
 
 import iotests
 
@@ -64,24 +64,16 @@ def get_test_files(directory: str = '.') -> List[str]:
     return list(filter(lambda f: is_python_file(f, directory), check_tests))
 
 
-def run_linters():
-    files = get_test_files()
-
-    iotests.logger.debug('Files to be checked:')
-    iotests.logger.debug(', '.join(sorted(files)))
+def run_linters(
+    files: List[str],
+    env: Optional[Mapping[str, str]] = None,
+) -> None:
 
     print('=== pylint ===')
     sys.stdout.flush()
 
     # 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(
         ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
         env=env,
@@ -95,7 +87,6 @@ def run_linters():
     # 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(
             (
@@ -128,7 +119,22 @@ def main() -> None:
         if shutil.which(linter) is None:
             iotests.notrun(f'{linter} not found')
 
-    run_linters()
+    files = get_test_files()
+
+    iotests.logger.debug('Files to be checked:')
+    iotests.logger.debug(', '.join(sorted(files)))
+
+    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
+
+    env['MYPYPATH'] = env['PYTHONPATH']
+
+    run_linters(files, env=env)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH v3 10/16] iotests/297: Add 'directory' argument to run_linters

[John Snow]

Allow run_linters to work well if it's executed from a different
directory.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 08d2b87108..e05c99972e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -66,6 +66,7 @@ def get_test_files(directory: str = '.') -> List[str]:
 
 def run_linters(
     files: List[str],
+    directory: str = '.',
     env: Optional[Mapping[str, str]] = None,
 ) -> None:
 
@@ -76,6 +77,7 @@ def run_linters(
     # fixed (in tests, at least)
     subprocess.run(
         ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+        cwd=directory,
         env=env,
         check=False,
     )
@@ -103,6 +105,7 @@ def run_linters(
                 '--namespace-packages',
                 filename,
             ),
+            cwd=directory,
             env=env,
             check=False,
             stdout=subprocess.PIPE,
-- 
2.31.1

[PATCH v3 11/16] iotests/297: return error code from run_linters()

[John Snow]

This turns run_linters() into a bit of a hybrid test; returning non-zero
on failed execution while also printing diffable information. This is
done for the benefit of the avocado simple test runner, which will soon
be attempting to execute this test from a different environment.

(Note: universal_newlines is added to the pylint invocation for type
consistency with the mypy run -- it's not strictly necessary, but it
avoids some typing errors caused by our re-use of the 'p' variable.)

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index e05c99972e..f9ddfb53a0 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -68,19 +68,22 @@ def run_linters(
     files: List[str],
     directory: str = '.',
     env: Optional[Mapping[str, str]] = None,
-) -> None:
+) -> int:
+    ret = 0
 
     print('=== pylint ===')
     sys.stdout.flush()
 
     # Todo notes are fine, but fixme's or xxx's should probably just be
     # fixed (in tests, at least)
-    subprocess.run(
+    p = subprocess.run(
         ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
         cwd=directory,
         env=env,
         check=False,
+        universal_newlines=True,
     )
+    ret += p.returncode
 
     print('=== mypy ===')
     sys.stdout.flush()
@@ -113,9 +116,12 @@ def run_linters(
             universal_newlines=True
         )
 
+        ret += p.returncode
         if p.returncode != 0:
             print(p.stdout)
 
+    return ret
+
 
 def main() -> None:
     for linter in ('pylint-3', 'mypy'):
-- 
2.31.1

[PATCH v3 12/16] iotests/297: split linters.py off from 297

[John Snow]

Split the linter execution itself out from 297, leaving just environment
setup in 297. This is done so that non-iotest code can invoke the
linters without needing to worry about imports of unpackaged iotest
code.

Eventually, it should be possible to replace linters.py with mypy.ini
and pylintrc files that instruct these tools how to run properly in this
directory, but ... not yet, and not in this series.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297        | 105 ++----------------------------
 tests/qemu-iotests/linters.py | 117 ++++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+), 101 deletions(-)
 create mode 100755 tests/qemu-iotests/linters.py

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index f9ddfb53a0..3d29af5b78 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,110 +17,13 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import re
 import shutil
-import subprocess
-import sys
-from typing import List, Mapping, Optional
 
 import iotests
+import linters
 
 
-# TODO: Empty this list!
-SKIP_FILES = (
-    '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
-    '096', '118', '124', '132', '136', '139', '147', '148', '149',
-    '151', '152', '155', '163', '165', '194', '196', '202',
-    '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
-    '218', '219', '224', '228', '234', '235', '236', '237', '238',
-    '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
-    '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
-    '299', '302', '303', '304', '307',
-    'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
-)
-
-
-def is_python_file(filename: str, directory: str = '.') -> bool:
-    filepath = os.path.join(directory, filename)
-
-    if not os.path.isfile(filepath):
-        return False
-
-    if filename.endswith('.py'):
-        return True
-
-    with open(filepath, encoding='utf-8') as f:
-        try:
-            first_line = f.readline()
-            return re.match('^#!.*python', first_line) is not None
-        except UnicodeDecodeError:  # Ignore binary files
-            return False
-
-
-def get_test_files(directory: str = '.') -> List[str]:
-    named_test_dir = os.path.join(directory, 'tests')
-    named_tests = [f"tests/{entry}" for entry in os.listdir(named_test_dir)]
-    check_tests = set(os.listdir(directory) + named_tests) - set(SKIP_FILES)
-    return list(filter(lambda f: is_python_file(f, directory), check_tests))
-
-
-def run_linters(
-    files: List[str],
-    directory: str = '.',
-    env: Optional[Mapping[str, str]] = None,
-) -> int:
-    ret = 0
-
-    print('=== pylint ===')
-    sys.stdout.flush()
-
-    # Todo notes are fine, but fixme's or xxx's should probably just be
-    # fixed (in tests, at least)
-    p = subprocess.run(
-        ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
-        cwd=directory,
-        env=env,
-        check=False,
-        universal_newlines=True,
-    )
-    ret += p.returncode
-
-    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).
-    for filename in files:
-        p = subprocess.run(
-            (
-                'python3', '-m', '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,
-            ),
-            cwd=directory,
-            env=env,
-            check=False,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT,
-            universal_newlines=True
-        )
-
-        ret += p.returncode
-        if p.returncode != 0:
-            print(p.stdout)
-
-    return ret
+# Looking for the list of excluded tests? See linters.py !
 
 
 def main() -> None:
@@ -128,7 +31,7 @@ def main() -> None:
         if shutil.which(linter) is None:
             iotests.notrun(f'{linter} not found')
 
-    files = get_test_files()
+    files = linters.get_test_files()
 
     iotests.logger.debug('Files to be checked:')
     iotests.logger.debug(', '.join(sorted(files)))
@@ -143,7 +46,7 @@ def main() -> None:
 
     env['MYPYPATH'] = env['PYTHONPATH']
 
-    run_linters(files, env=env)
+    linters.run_linters(files, env=env)
 
 
 iotests.script_main(main)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100755
index 0000000000..e263b7cbee
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,117 @@
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import re
+import subprocess
+import sys
+from typing import List, Mapping, Optional
+
+
+# TODO: Empty this list!
+SKIP_FILES = (
+    '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
+    '096', '118', '124', '132', '136', '139', '147', '148', '149',
+    '151', '152', '155', '163', '165', '194', '196', '202',
+    '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
+    '218', '219', '224', '228', '234', '235', '236', '237', '238',
+    '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
+    '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
+    '299', '302', '303', '304', '307',
+    'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
+)
+
+
+def is_python_file(filename: str, directory: str = '.') -> bool:
+    filepath = os.path.join(directory, filename)
+
+    if not os.path.isfile(filepath):
+        return False
+
+    if filename.endswith('.py'):
+        return True
+
+    with open(filepath, encoding='utf-8') as f:
+        try:
+            first_line = f.readline()
+            return re.match('^#!.*python', first_line) is not None
+        except UnicodeDecodeError:  # Ignore binary files
+            return False
+
+
+def get_test_files(directory: str = '.') -> List[str]:
+    named_test_dir = os.path.join(directory, 'tests')
+    named_tests = [f"tests/{entry}" for entry in os.listdir(named_test_dir)]
+    check_tests = set(os.listdir(directory) + named_tests) - set(SKIP_FILES)
+    return list(filter(lambda f: is_python_file(f, directory), check_tests))
+
+
+def run_linters(
+    files: List[str],
+    directory: str = '.',
+    env: Optional[Mapping[str, str]] = None,
+) -> int:
+    ret = 0
+
+    print('=== pylint ===')
+    sys.stdout.flush()
+
+    # Todo notes are fine, but fixme's or xxx's should probably just be
+    # fixed (in tests, at least)
+    p = subprocess.run(
+        ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+        cwd=directory,
+        env=env,
+        check=False,
+        universal_newlines=True,
+    )
+    ret += p.returncode
+
+    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).
+    for filename in files:
+        p = subprocess.run(
+            (
+                'python3', '-m', '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,
+            ),
+            cwd=directory,
+            env=env,
+            check=False,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
+            universal_newlines=True
+        )
+
+        ret += p.returncode
+        if p.returncode != 0:
+            print(p.stdout)
+
+    return ret
-- 
2.31.1

[PATCH v3 13/16] iotests/linters: Add entry point for Python CI linters

[John Snow]

Add a main() function to linters.py so that the Python CI infrastructure
has something it can run.

Now, linters.py represents an invocation of the linting scripts that
more resembles a "normal" execution of pylint/mypy, like you'd expect to
use if 'qemu' was a bona-fide package you obtained from PyPI.

297, by contrast, now represents the iotests-specific configuration bits
you need to get it to function correctly as a part of iotests, and with
'qemu' as a namespace package that isn't "installed" to the current
environment, but just lives elsewhere in our source tree.

By doing this, we will able to run the same linting configuration from
the Python CI tests without calling iotest logging functions or messing
around with PYTHONPATH / MYPYPATH.

iotest 297 continues to operate in a standalone fashion for now --
presumably, it's convenient for block maintainers and contributors to
run in this manner. We can either remove this functionality at a later
date if everyone is happy with the Python CI, or we can opt to continue
to maintain it. Please let me know how you feel.

See the following commit for how this is used from the Python packaging side.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/linters.py | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index e263b7cbee..4df062a973 100755
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -115,3 +115,16 @@ def run_linters(
             print(p.stdout)
 
     return ret
+
+
+def main() -> int:
+    """
+    Used by the Python CI system as an entry point to run these linters.
+    """
+    directory = os.path.dirname(os.path.realpath(__file__))
+    files = get_test_files(directory)
+    return run_linters(files, directory)
+
+
+if __name__ == '__main__':
+    sys.exit(main())
-- 
2.31.1

[PATCH v3 14/16] iotests/linters: Add workaround for mypy bug #9852

[John Snow]

This one is insidious: if you use the invocation
"from {namespace} import {subpackage}" as mirror-top-perms does,
mypy will fail on every-other invocation *if* the package being
imported is a package.

Now, I could just edit mirror-top-perms to avoid this invocation, but
since I tripped on a landmine, I might as well head it off at the pass
and make sure nobody else trips on the same landmine.

It seems to have something to do with the order in which files are
checked as well, meaning the random order in which set(os.listdir())
produces the list of files to test will cause problems intermittently.

mypy >= 0.920 isn't released yet, so add this workaround for now.

See also:
 https://github.com/python/mypy/issues/11010
 https://github.com/python/mypy/issues/9852

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/linters.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 4df062a973..9c97324e87 100755
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -100,6 +100,9 @@ def run_linters(
                 '--warn-unused-ignores',
                 '--no-implicit-reexport',
                 '--namespace-packages',
+                # Until we can use mypy >= 0.920, see
+                # https://github.com/python/mypy/issues/9852
+                '--no-incremental',
                 filename,
             ),
             cwd=directory,
-- 
2.31.1

[PATCH v3 15/16] python: Add iotest linters to test suite

[John Snow]

As a convenience, since iotests is an extremely prominent user of the
qemu.qmp and qemu.machine packages and already implements a linting
regime, run those tests as well so that it's very hard to miss
regressions caused by changes to the python library.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/tests/iotests.sh | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100755 python/tests/iotests.sh

diff --git a/python/tests/iotests.sh b/python/tests/iotests.sh
new file mode 100755
index 0000000000..70324540cf
--- /dev/null
+++ b/python/tests/iotests.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+
+cd ../tests/qemu-iotests/
+python3 -m linters
-- 
2.31.1

[PATCH v3 16/16] 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>
---
 tests/qemu-iotests/linters.py | 62 ++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 9c97324e87..ac9f77c5ac 100755
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -82,40 +82,36 @@ 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).
-    for filename in files:
-        p = subprocess.run(
-            (
-                'python3', '-m', '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',
-                # Until we can use mypy >= 0.920, see
-                # https://github.com/python/mypy/issues/9852
-                '--no-incremental',
-                filename,
-            ),
-            cwd=directory,
-            env=env,
-            check=False,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT,
-            universal_newlines=True
-        )
+    p = subprocess.run(
+        (
+            'python3', '-m', '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',
+            # Until we can use mypy >= 0.920, see
+            # https://github.com/python/mypy/issues/9852
+            '--no-incremental',
+            '--scripts-are-modules',
+            *files,
+        ),
+        cwd=directory,
+        env=env,
+        check=False,
+        stdout=subprocess.PIPE,
+        stderr=subprocess.STDOUT,
+        universal_newlines=True
+    )
 
-        ret += p.returncode
-        if p.returncode != 0:
-            print(p.stdout)
+    ret += p.returncode
+    if p.returncode != 0:
+        print(p.stdout)
 
     return ret
 
-- 
2.31.1

Re: [PATCH v3 02/16] iotests/mirror-top-perms: Adjust imports

[Philippe Mathieu-Daudé]

On 9/16/21 6:09 AM, John Snow wrote:
> We need to import things from the qemu namespace; importing the
> namespace alone doesn't bring the submodules with it -- unless someone
> else (like iotests.py) imports them too.
> 
> Adjust the imports.
> 
> Signed-off-by: John Snow <jsnow@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 2fc8dd66e0..de18182590 100755
> --- a/tests/qemu-iotests/tests/mirror-top-perms
> +++ b/tests/qemu-iotests/tests/mirror-top-perms
> @@ -25,7 +25,8 @@ from iotests import qemu_img
>  
>  # Import qemu after iotests.py has amended sys.path
>  # pylint: disable=wrong-import-order
> -import qemu
> +from qemu import qmp
> +from qemu.machine import machine

Not straight-forward import name...

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 04/16] iotests/migrate-bitmaps-test: delint

[Philippe Mathieu-Daudé]

On 9/16/21 6:09 AM, John Snow wrote:
> 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>
> ---
>  tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++++++++++--------
>  1 file changed, 28 insertions(+), 22 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 08/16] iotests/297: Create main() function

[Philippe Mathieu-Daudé]

On 9/16/21 6:09 AM, John Snow wrote:
> Instead of running "run_linters" directly, create a main() function that
> will be responsible for environment setup, leaving run_linters()
> responsible only for execution of the linters.
> 
> (That environment setup will be moved over in forthcoming commits.)
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/297 | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 13/16] iotests/linters: Add entry point for Python CI linters

[Philippe Mathieu-Daudé]

On 9/16/21 6:09 AM, John Snow wrote:
> Add a main() function to linters.py so that the Python CI infrastructure
> has something it can run.
> 
> Now, linters.py represents an invocation of the linting scripts that
> more resembles a "normal" execution of pylint/mypy, like you'd expect to
> use if 'qemu' was a bona-fide package you obtained from PyPI.
> 
> 297, by contrast, now represents the iotests-specific configuration bits
> you need to get it to function correctly as a part of iotests, and with
> 'qemu' as a namespace package that isn't "installed" to the current
> environment, but just lives elsewhere in our source tree.
> 
> By doing this, we will able to run the same linting configuration from
> the Python CI tests without calling iotest logging functions or messing
> around with PYTHONPATH / MYPYPATH.
> 
> iotest 297 continues to operate in a standalone fashion for now --
> presumably, it's convenient for block maintainers and contributors to
> run in this manner. We can either remove this functionality at a later
> date if everyone is happy with the Python CI, or we can opt to continue
> to maintain it. Please let me know how you feel.
> 
> See the following commit for how this is used from the Python packaging side.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/linters.py | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 01/16] python: Update for pylint 2.10

[Alex Bennée]

John Snow <jsnow@redhat.com> writes:

> A few new annoyances. Of note is the new warning for an unspecified
> encoding when opening a text file, which actually does indicate a
> potentially real problem; see
> https://www.python.org/dev/peps/pep-0597/#motivation
>
> Use LC_CTYPE to determine an encoding to use for interpreting QEMU's
> terminal output. Note that Python states: "language code and encoding
> may be None if their values cannot be determined" -- use a platform
> default as a backup.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

I've cherry-picked this one into my testing/next because it fixes a
failure but in case you get it merged elsewhere:

Tested-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH v3 02/16] iotests/mirror-top-perms: Adjust imports

[John Snow]

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

On Thu, Sep 16, 2021 at 12:27 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 9/16/21 6:09 AM, John Snow wrote:
> > We need to import things from the qemu namespace; importing the
> > namespace alone doesn't bring the submodules with it -- unless someone
> > else (like iotests.py) imports them too.
> >
> > Adjust the imports.
> >
> > Signed-off-by: John Snow <jsnow@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 2fc8dd66e0..de18182590 100755
> > --- a/tests/qemu-iotests/tests/mirror-top-perms
> > +++ b/tests/qemu-iotests/tests/mirror-top-perms
> > @@ -25,7 +25,8 @@ from iotests import qemu_img
> >
> >  # Import qemu after iotests.py has amended sys.path
> >  # pylint: disable=wrong-import-order
> > -import qemu
> > +from qemu import qmp
> > +from qemu.machine import machine
>
> Not straight-forward import name...
>
>
You mean the 'qemu.machine.machine' path? If so, I agree. It will be fixed
when I refactor QEMUMachine. A/QMP happens first.


> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>

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

Re: [PATCH v3 01/16] python: Update for pylint 2.10

[John Snow]

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

On Thu, Sep 16, 2021 at 9:30 AM Alex Bennée <alex.bennee@linaro.org> wrote:

>
> John Snow <jsnow@redhat.com> writes:
>
> > A few new annoyances. Of note is the new warning for an unspecified
> > encoding when opening a text file, which actually does indicate a
> > potentially real problem; see
> > https://www.python.org/dev/peps/pep-0597/#motivation
> >
> > Use LC_CTYPE to determine an encoding to use for interpreting QEMU's
> > terminal output. Note that Python states: "language code and encoding
> > may be None if their values cannot be determined" -- use a platform
> > default as a backup.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> I've cherry-picked this one into my testing/next because it fixes a
> failure but in case you get it merged elsewhere:
>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> --
> Alex Bennée
>
>
Dan had some comments here:
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg04031.html

I'm waiting for his reply, but feel free to use this as a build fix in the
meantime for testing, I don't think it should hurt anything.

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

Re: [PATCH v3 02/16] iotests/mirror-top-perms: Adjust imports

[Philippe Mathieu-Daudé]

On 9/16/21 4:27 PM, John Snow wrote:
> On Thu, Sep 16, 2021 at 12:27 AM Philippe Mathieu-Daudé
> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> 
>     On 9/16/21 6:09 AM, John Snow wrote:
>     > We need to import things from the qemu namespace; importing the
>     > namespace alone doesn't bring the submodules with it -- unless someone
>     > else (like iotests.py) imports them too.
>     >
>     > Adjust the imports.
>     >
>     > Signed-off-by: John Snow <jsnow@redhat.com <mailto:jsnow@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 2fc8dd66e0..de18182590 100755
>     > --- a/tests/qemu-iotests/tests/mirror-top-perms
>     > +++ b/tests/qemu-iotests/tests/mirror-top-perms
>     > @@ -25,7 +25,8 @@ from iotests import qemu_img
>     > 
>     >  # Import qemu after iotests.py has amended sys.path
>     >  # pylint: disable=wrong-import-order
>     > -import qemu
>     > +from qemu import qmp
>     > +from qemu.machine import machine
> 
>     Not straight-forward import name...
> 
> 
> You mean the 'qemu.machine.machine' path? If so, I agree. It will be
> fixed when I refactor QEMUMachine. A/QMP happens first.

Good news!

Re: [PATCH v3 00/16] python/iotests: Run iotest linters during Python CI

[John Snow]

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

On Thu, Sep 16, 2021 at 12:10 AM John Snow <jsnow@redhat.com> wrote:

> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/371611883
> Based-On: <20210915175318.853225-1-hreitz@redhat.com>
>           "[PULL 00/32] Block patches"
>
> Since iotests are such a heavy and prominent user of the Python qemu.qmp
> and qemu.machine packages, it would be convenient if the Python linting
> suite also checked this client for any possible regressions introduced
> by shifting around signatures, types, or interfaces in these packages.
>
> (We'd eventually find those problems when iotest 297 ran, but with
> increasing distance between Python development and Block development,
> the risk of an accidental breakage in this regard increases. I,
> personally, know to run iotests (and especially 297) after changing
> Python code, but not everyone in the future might. Plus, I am lazy, and
> I like only having to push one button.)
>
> Add the ability for the Python CI to run the iotest linters too, which
> means that the iotest linters would be checked against:
>
> - Python 3.6, using a frozen set of linting packages at their oldest
>   supported versions, using 'pipenv'
> - Python 3.6 through Python 3.10 inclusive, using 'tox' and the latest
>   versions of mypy/pylint that happen to be installed during test
>   time. This CI test is allowed to fail with a warning, and can serve
>   as a bellwether for when new incompatible changes may disrupt the
>   linters. Testing against old and new Python interpreters alike can
>   help surface incompatibility issues we may need to be aware of.)
>
> Here are example outputs of those CI jobs with this series applied:
>  - "check-python-pipenv": https://gitlab.com/jsnow/qemu/-/jobs/1377735087
>  - "check-python-tox": https://gitlab.com/jsnow/qemu/-/jobs/1377735088
>
> You can also run these same tests locally from ./python, plus one more:
>
> - "make check-dev" to test against whatever python you have.
> - "make check-pipenv", if you have Python 3.6 and pipenv installed.
> - "make check-tox", to test against multiple python versions you have
> installed,
>                     from 3.6 to 3.10 inclusive. (CI tests against all 5.)
>
> See the old commit message for more sample output, etc.
>
> https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg07056.html
>
> V3:
>  - Added patch 1 which has been submitted separately upstream,
>    but was necessary for testing.
>  - Rebased on top of hreitz/block, which fixed some linting issues.
>  - Added a workaround for a rather nasty mypy bug ... >:(
>
> V2:
>  - Added patches 1-5 which do some more delinting.
>  - Added patch 8, which scans subdirs for tests to lint.
>  - Added patch 17, which improves the speed of mypy analysis.
>  - Patch 14 is different because of the new patch 8.
>
> John Snow (16):
>   python: Update for pylint 2.10
>   iotests/mirror-top-perms: Adjust imports
>   iotests/migrate-bitmaps-postcopy-test: declare instance variables
>   iotests/migrate-bitmaps-test: delint
>   iotests/297: modify is_python_file to work from any CWD
>   iotests/297: Add get_files() function
>   iotests/297: Don't rely on distro-specific linter binaries
>   iotests/297: Create main() function
>   iotests/297: Separate environment setup from test execution
>   iotests/297: Add 'directory' argument to run_linters
>   iotests/297: return error code from run_linters()
>   iotests/297: split linters.py off from 297
>   iotests/linters: Add entry point for Python CI linters
>   iotests/linters: Add workaround for mypy bug #9852
>   python: Add iotest linters to test suite
>   iotests/linters: check mypy files all at once
>
>  python/qemu/machine/machine.py                |   9 +-
>  python/setup.cfg                              |   1 +
>  python/tests/iotests.sh                       |   4 +
>  tests/qemu-iotests/297                        |  81 ++---------
>  tests/qemu-iotests/linters.py                 | 129 ++++++++++++++++++
>  .../tests/migrate-bitmaps-postcopy-test       |   3 +
>  tests/qemu-iotests/tests/migrate-bitmaps-test |  50 ++++---
>  tests/qemu-iotests/tests/mirror-top-perms     |   7 +-
>  8 files changed, 186 insertions(+), 98 deletions(-)
>  create mode 100755 python/tests/iotests.sh
>  create mode 100755 tests/qemu-iotests/linters.py
>
> --
> 2.31.1
>
>
>
FWIW: I sent a new version of a pull request that adds pylint 2.10 *and*
2.11 support; the 2.11 release happened just yesterday, so I am going to
rebase this series.

Additionally, I found a new way to avoid sys.path hacking in all of our
test files entirely, so I will include that in this series, rebase, and
resend extremely soon. If you have difficulties applying this patchset or
testing it, sit tight for a refreshed version -- but most of these patches
can still be reviewed on their own merits in the meantime.

Thanks,
--js

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

Re: [PATCH v3 02/16] iotests/mirror-top-perms: Adjust imports

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> We need to import things from the qemu namespace; importing the
> namespace alone doesn't bring the submodules with it -- unless someone
> else (like iotests.py) imports them too.
>
> Adjust the imports.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/tests/mirror-top-perms | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 03/16] iotests/migrate-bitmaps-postcopy-test: declare instance variables

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> index 00ebb5c251..9c00ae61c8 100755
> --- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> +++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> @@ -115,6 +115,9 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
>           self.vm_a_events = []
>           self.vm_b_events = []
>   
> +        self.discards1_sha256: str
> +        self.all_discards_sha256: str
> +
>       def start_postcopy(self):
>           """ Run migration until RESUME event on target. Return this event. """
>           for i in range(nb_bitmaps):

Isn’t this obsolete after e2ad17a62d9da45fbcddc3faa3d6ced519a9453a?

Hanna

Re: [PATCH v3 04/16] iotests/migrate-bitmaps-test: delint

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> 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>
> ---
>   tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++++++++++--------
>   1 file changed, 28 insertions(+), 22 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 05/16] iotests/297: modify is_python_file to work from any CWD

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> Add a directory argument to is_python_file to allow it to work correctly
> no matter what CWD we happen to run it from. This is done in
> anticipation of running the iotests from another directory (./python/).

“the iotests” or just 297?  All of the iotests would sound like an 
ambitious goal.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 06/16] iotests/297: Add get_files() function

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> Split out file discovery into its own method to begin separating out the
> "environment setup" and "test execution" phases.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 3e86f788fc..b3a56a3a29 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -21,6 +21,7 @@ import re
>   import shutil
>   import subprocess
>   import sys
> +from typing import List
>   
>   import iotests
>   
> @@ -56,10 +57,15 @@ def is_python_file(filename: str, directory: str = '.') -> bool:
>               return False
>   
>   
> +def get_test_files(directory: str = '.') -> List[str]:
> +    named_test_dir = os.path.join(directory, 'tests')
> +    named_tests = [f"tests/{entry}" for entry in os.listdir(named_test_dir)]
> +    check_tests = set(os.listdir(directory) + named_tests) - set(SKIP_FILES)
> +    return list(filter(lambda f: is_python_file(f, directory), check_tests))

Seeing a filter() makes me immensely happy, but I thought that was 
unpythonic?

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 07/16] iotests/297: Don't rely on distro-specific linter binaries

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> 'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
> mypy" to access these scripts instead. This style of invocation will
> prefer the "correct" tool when run in a virtual environment.
>
> Note that we still check for "pylint-3" before the test begins -- this
> check is now "overly strict", but shouldn't cause anything that was
> already running correctly to start failing.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/qemu-iotests/297 | 45 ++++++++++++++++++++++++------------------
>   1 file changed, 26 insertions(+), 19 deletions(-)

I know it sounds silly, but to be honest I have no idea if replacing 
`mypy` by `python3 -m mypy` is correct, because no mypy documentation 
seems to suggest it.

 From what I understand, that’s generally how Python “binaries” work, 
though (i.e., installed as a module invokable with `python -m`, and then 
providing some stub binary that, well, effectively does this, but kind 
of in a weird way, I just don’t understand it), and none of the 
parameters seem to be hurt in this conversion, so:

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 08/16] iotests/297: Create main() function

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> Instead of running "run_linters" directly, create a main() function that
> will be responsible for environment setup, leaving run_linters()
> responsible only for execution of the linters.
>
> (That environment setup will be moved over in forthcoming commits.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/297 | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 09/16] iotests/297: Separate environment setup from test execution

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> Move environment setup into main(), leaving pure test execution behind
> in run_linters().
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/297 | 36 +++++++++++++++++++++---------------
>   1 file changed, 21 insertions(+), 15 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 10/16] iotests/297: Add 'directory' argument to run_linters

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> Allow run_linters to work well if it's executed from a different
> directory.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/297 | 3 +++
>   1 file changed, 3 insertions(+)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 11/16] iotests/297: return error code from run_linters()

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> This turns run_linters() into a bit of a hybrid test; returning non-zero
> on failed execution while also printing diffable information. This is
> done for the benefit of the avocado simple test runner, which will soon
> be attempting to execute this test from a different environment.
>
> (Note: universal_newlines is added to the pylint invocation for type
> consistency with the mypy run -- it's not strictly necessary, but it
> avoids some typing errors caused by our re-use of the 'p' variable.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/297 | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)

I don’t think I like this very much.  Returning an integer error code 
seems archaic.

(You can perhaps already see that this is going to be one of these 
reviews of mine where I won’t say anything is really wrong, but where I 
will just lament subjectively missing beauty.)


As you say, run_linters() to me seems very iotests-specific still: It 
emits a specific output that is compared against a reference output.  
Fine for 297, but not fine for a function provided by a linters.py, I’d say.

I’d prefer run_linters() to return something like a Map[str, 
Optional[str]], that would map a tool to its output in case of error, 
i.e. ideally:

`{'pylint': None, 'mypy': None}`

297 could format it something like

```
for tool, output in run_linters().items():
     print(f'=== {tool} ===')
     if output is not None:
         print(output)
```

Or something.

To check for error, you could put a Python script in python/tests that 
checks `any(output is not None for output in run_linters().values())` or 
something (and on error print the output).


Pulling out run_linters() into an external file and having it print 
something to stdout just seems too iotests-centric to me.  I suppose as 
long as the return code is right (which this patch is for) it should 
work for Avocado’s simple tests, too (which I don’t like very much 
either, by the way, because they too seem archaic to me), but, well.  It 
almost seems like the Avocado test should just run ./check then.

Come to think of it, to be absolutely blasphemous, why not.  I could say 
all of this seems like quite some work that could be done by a 
python/tests script that does this:

```
#!/bin/sh
set -e

cat >/tmp/qemu-parrot.sh <<EOF
#!/bin/sh
echo ': qcow2'
echo ': qcow2'
echo 'virtio-blk'
EOF

cd $QEMU_DIR/tests/qemu-iotests

QEMU_PROG="/tmp/qemu-parrot.sh" \
QEMU_IMG_PROG=$(which false) \
QEMU_IO_PROG=$(which false) \
QEMU_NBD_PROG=$(which false) \
QSD_PROG=$(which false) \
./check 297
```

And, no, I don’t want that!  But the point of this series seems to just 
be to rip the core of 297 out so it can run without ./check, because 
./check requires some environment variables to be set. Doing that seems 
just seems wrong to me.

Like, can’t we have a Python script in python/tests that imports 
linters.py, invokes run_linters() and sensibly checks the output? Or, 
you know, at the very least not have run_linters() print anything to 
stdout and not have it return an integer code. linters.py:main() can do 
that conversion.


Or, something completely different, perhaps my problem is that you put 
linters.py as a fully standalone test into the iotests directory, 
without it being an iotest.  So, I think I could also agree on putting 
linters.py into python/tests, and then having 297 execute that.  Or you 
know, we just drop 297 altogether, as you suggest in patch 13 – if 
that’s what it takes, then so be it.

Hanna


PS: Also, summing up processes’ return codes makes me feel not good.

> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index e05c99972e..f9ddfb53a0 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -68,19 +68,22 @@ def run_linters(
>       files: List[str],
>       directory: str = '.',
>       env: Optional[Mapping[str, str]] = None,
> -) -> None:
> +) -> int:
> +    ret = 0
>   
>       print('=== pylint ===')
>       sys.stdout.flush()
>   
>       # Todo notes are fine, but fixme's or xxx's should probably just be
>       # fixed (in tests, at least)
> -    subprocess.run(
> +    p = subprocess.run(
>           ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
>           cwd=directory,
>           env=env,
>           check=False,
> +        universal_newlines=True,
>       )
> +    ret += p.returncode
>   
>       print('=== mypy ===')
>       sys.stdout.flush()
> @@ -113,9 +116,12 @@ def run_linters(
>               universal_newlines=True
>           )
>   
> +        ret += p.returncode
>           if p.returncode != 0:
>               print(p.stdout)
>   
> +    return ret
> +
>   
>   def main() -> None:
>       for linter in ('pylint-3', 'mypy'):

Re: [PATCH v3 14/16] iotests/linters: Add workaround for mypy bug #9852

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> This one is insidious: if you use the invocation
> "from {namespace} import {subpackage}" as mirror-top-perms does,
> mypy will fail on every-other invocation *if* the package being
> imported is a package.
>
> Now, I could just edit mirror-top-perms to avoid this invocation, but
> since I tripped on a landmine, I might as well head it off at the pass
> and make sure nobody else trips on the same landmine.
>
> It seems to have something to do with the order in which files are
> checked as well, meaning the random order in which set(os.listdir())
> produces the list of files to test will cause problems intermittently.
>
> mypy >= 0.920 isn't released yet, so add this workaround for now.
>
> See also:
>   https://github.com/python/mypy/issues/11010
>   https://github.com/python/mypy/issues/9852
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/linters.py | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
> index 4df062a973..9c97324e87 100755
> --- a/tests/qemu-iotests/linters.py
> +++ b/tests/qemu-iotests/linters.py
> @@ -100,6 +100,9 @@ def run_linters(
>                   '--warn-unused-ignores',
>                   '--no-implicit-reexport',
>                   '--namespace-packages',
> +                # Until we can use mypy >= 0.920, see
> +                # https://github.com/python/mypy/issues/9852
> +                '--no-incremental',
>                   filename,
>               ),

I’m afraid I still don’t really understand this, but I’m happy with this 
given as the reported workaround and you saying it works.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Question is, when “can we use” mypy >= 0.920?  Should we check the 
version string and append this switch as required?

Hanna

Re: [PATCH v3 16/16] iotests/linters: check mypy files all at once

[Hanna Reitz]

On 16.09.21 06:09, John Snow wrote:
> 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.

Is it possible to pull this to the beginning of the series?  Just 
because patch 14 has to make everything quite slow (which might be a 
tiny nuisance in bisecting some day?).

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/linters.py | 62 ++++++++++++++++-------------------
>   1 file changed, 29 insertions(+), 33 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 03/16] iotests/migrate-bitmaps-postcopy-test: declare instance variables

[John Snow]

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

On Fri, Sep 17, 2021 at 4:37 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 16.09.21 06:09, John Snow wrote:
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> > index 00ebb5c251..9c00ae61c8 100755
> > --- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> > +++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> > @@ -115,6 +115,9 @@ class
> TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
> >           self.vm_a_events = []
> >           self.vm_b_events = []
> >
> > +        self.discards1_sha256: str
> > +        self.all_discards_sha256: str
> > +
> >       def start_postcopy(self):
> >           """ Run migration until RESUME event on target. Return this
> event. """
> >           for i in range(nb_bitmaps):
>
> Isn’t this obsolete after e2ad17a62d9da45fbcddc3faa3d6ced519a9453a?
>
> Hanna
>
>
Yes, my apologies.

--js

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

Re: [PATCH v3 06/16] iotests/297: Add get_files() function

[John Snow]

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

On Fri, Sep 17, 2021 at 5:24 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 16.09.21 06:09, John Snow wrote:
> > Split out file discovery into its own method to begin separating out the
> > "environment setup" and "test execution" phases.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/297 | 12 +++++++++---
> >   1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 3e86f788fc..b3a56a3a29 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -21,6 +21,7 @@ import re
> >   import shutil
> >   import subprocess
> >   import sys
> > +from typing import List
> >
> >   import iotests
> >
> > @@ -56,10 +57,15 @@ def is_python_file(filename: str, directory: str =
> '.') -> bool:
> >               return False
> >
> >
> > +def get_test_files(directory: str = '.') -> List[str]:
> > +    named_test_dir = os.path.join(directory, 'tests')
> > +    named_tests = [f"tests/{entry}" for entry in
> os.listdir(named_test_dir)]
> > +    check_tests = set(os.listdir(directory) + named_tests) -
> set(SKIP_FILES)
> > +    return list(filter(lambda f: is_python_file(f, directory),
> check_tests))
>
> Seeing a filter() makes me immensely happy, but I thought that was
> unpythonic?
>
>
Eh, depends on what you're doing, I guess?

The alternative spelling is:
list(f for f in check_tests if is_python_file(f, directory))

... which I guess *is* shorter and skips the lambda. but, I suppose I have
some mild revulsion to seeing "f for f in ..." constructs.
If I ever tell you not to use a filter, feel free to cite this patch and
then just tell me to shut up. I apologize for any inconsistencies in my
style, real or imagined.

--js

Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>

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

Re: [PATCH v3 07/16] iotests/297: Don't rely on distro-specific linter binaries

[John Snow]

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

(This email just explains python packaging stuff. No action items in here.
Skim away.)

On Fri, Sep 17, 2021 at 5:43 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 16.09.21 06:09, John Snow wrote:
> > 'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
> > mypy" to access these scripts instead. This style of invocation will
> > prefer the "correct" tool when run in a virtual environment.
> >
> > Note that we still check for "pylint-3" before the test begins -- this
> > check is now "overly strict", but shouldn't cause anything that was
> > already running correctly to start failing.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >   tests/qemu-iotests/297 | 45 ++++++++++++++++++++++++------------------
> >   1 file changed, 26 insertions(+), 19 deletions(-)
>
> I know it sounds silly, but to be honest I have no idea if replacing
> `mypy` by `python3 -m mypy` is correct, because no mypy documentation
> seems to suggest it.
>
>
Right, I don't think it's necessarily documented that you can do this. It
just happens to be a very convenient way to invoke the same script without
needing to know *where* mypy is. You let python figure out where it's going
to import mypy from, and it handles the rest.

(This also makes it easier to use things like mypy, pylint etc with an
explicitly specified PYTHON interpreter. I don't happen to do that in this
patch, but ... we could.)

>
>  From what I understand, that’s generally how Python “binaries” work,
> though (i.e., installed as a module invokable with `python -m`, and then
> providing some stub binary that, well, effectively does this, but kind
> of in a weird way, I just don’t understand it), and none of the
> parameters seem to be hurt in this conversion, so:
>
>
Right. Technically, any python package can ask for any number of
executables to be installed, but the setuptools packaging ecosystem
provides a way to "generate" these based on package configuration. I use a
few in our own Python packages. If you look in python/setup.cfg, you'll see
stuff like this:

[options.entry_points]
console_scripts =
    qom = qemu.qmp.qom:main
    qom-set = qemu.qmp.qom:QOMSet.entry_point
    qom-get = qemu.qmp.qom:QOMGet.entry_point
    qom-list = qemu.qmp.qom:QOMList.entry_point
    qom-tree = qemu.qmp.qom:QOMTree.entry_point
    qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
    qemu-ga-client = qemu.qmp.qemu_ga_client:main
    qmp-shell = qemu.qmp.qmp_shell:main

These entries cause those weird little binary wrapper scripts to be
generated for you when the package is *installed*. So our packages will put
'qmp-shell' and 'qom-tree' into your $PATH*.The stuff to the right of the
equals sign is just a pointer to a function that can be executed that
implements the CLI command. qemu.qmp.qmp_shell points to the module to
import, and ':main' points to the function to run.

The last bit of this is that many, though not all (and there's zero
requirement this has to be true), python packages that implement CLI
commands will often have a stanza in their __init__.py module that says
something like this:

if __name__ == '__main__':
    do_the_command_line_stuff()

Alternatively, a package can include a literal __main__.py file that python
knows to check for, and this module is the one that gets executed for
`python3 -m mypackage` invocations. This is what mypy does.

Those are the magical blurbs that allow you to execute a module as if it
were a script by running "python3 -m mymodule" -- that hooks directly into
that little execution stanza. For python code distributed as packages,
that's the real reason to have that little magic stanza -- it provides a
convenient way to run stuff without needing to write the incredibly more
tedious:

python3 -c "from mypy.__main__ import console_entry; console_entry();"

... which is quite a bit more porcelain too, depending on how they
re/factor the code inside of the package.

Seeing as how mypy explicitly includes a __main__.py file:
https://github.com/python/mypy/blob/master/mypy/__main__.py, I am taking it
as a given that they are fully aware of invoking mypy in this fashion, and
believe it safe to rely on.

There will be a quiz later.
(There will not be a quiz.)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>

Thanks!

--js

*If your $PATH is configured to include, say, ~/.local/bin/, that is. On
Fedora, this is where executable shims for python get deposited when you do
"pip3 install --user somepackage". Python virtual environments will
configure your $PATH to include packages installed in that venv, etc etc
etc. Not every distro is configured such that pip packages are on the PATH,
so you can't rely on them being there when writing other scripts, usually.
Invoking the python interpreter directly seems far more flexible.

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

Re: [PATCH v3 11/16] iotests/297: return error code from run_linters()

[John Snow]

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

On Fri, Sep 17, 2021 at 7:00 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 16.09.21 06:09, John Snow wrote:
> > This turns run_linters() into a bit of a hybrid test; returning non-zero
> > on failed execution while also printing diffable information. This is
> > done for the benefit of the avocado simple test runner, which will soon
> > be attempting to execute this test from a different environment.
> >
> > (Note: universal_newlines is added to the pylint invocation for type
> > consistency with the mypy run -- it's not strictly necessary, but it
> > avoids some typing errors caused by our re-use of the 'p' variable.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   tests/qemu-iotests/297 | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
>
> I don’t think I like this very much.  Returning an integer error code
> seems archaic.
>
> (You can perhaps already see that this is going to be one of these
> reviews of mine where I won’t say anything is really wrong, but where I
> will just lament subjectively missing beauty.)
>
>
Haha. It's fine, Vladimir didn't like the smell of this one either. I just
didn't want to prematurely optimize or overcomplicate.


> As you say, run_linters() to me seems very iotests-specific still: It
> emits a specific output that is compared against a reference output.
> Fine for 297, but not fine for a function provided by a linters.py, I’d
> say.
>
> I’d prefer run_linters() to return something like a Map[str,
> Optional[str]], that would map a tool to its output in case of error,
> i.e. ideally:
>
> `{'pylint': None, 'mypy': None}`
>
>
Splitting the test apart into two sub-tests is completely reasonable.
Python CI right now has individual tests for pylint, mypy, etc.


> 297 could format it something like
>
> ```
> for tool, output in run_linters().items():
>      print(f'=== {tool} ===')
>      if output is not None:
>          print(output)
> ```
>
> Or something.
>
> To check for error, you could put a Python script in python/tests that
> checks `any(output is not None for output in run_linters().values())` or
> something (and on error print the output).
>
>
> Pulling out run_linters() into an external file and having it print
> something to stdout just seems too iotests-centric to me.  I suppose as
> long as the return code is right (which this patch is for) it should
> work for Avocado’s simple tests, too (which I don’t like very much
> either, by the way, because they too seem archaic to me), but, well.  It
> almost seems like the Avocado test should just run ./check then.
>
>
Yeh. Ideally, we'd just have a mypy.ini and a pylintrc that configures the
tests adequately, and then 297 (or whomever else) would just call the
linters which would in turn read the same configuration. This series is
somewhat of a stop-gap to measure the temperature of the room to see how
important it was to leave 297 inside of iotests. So, I did the conservative
thing that's faster to review even if it now looks *slightly* fishy.

As for things being archaic or not ... possibly, but why involve extra
complexity if it isn't warranted? a simple pass/fail works perfectly well.
(And the human can read the output to understand WHY it failed.) If you
want more rigorous analytics for some reason, we can discuss the use cases
and figure out how to represent that information, but that's definitely a
bit beyond scope here.

>
> Come to think of it, to be absolutely blasphemous, why not.  I could say
> all of this seems like quite some work that could be done by a
> python/tests script that does this:
>
> ```
> #!/bin/sh
> set -e
>
> cat >/tmp/qemu-parrot.sh <<EOF
> #!/bin/sh
> echo ': qcow2'
> echo ': qcow2'
> echo 'virtio-blk'
> EOF
>
> cd $QEMU_DIR/tests/qemu-iotests
>
> QEMU_PROG="/tmp/qemu-parrot.sh" \
> QEMU_IMG_PROG=$(which false) \
> QEMU_IO_PROG=$(which false) \
> QEMU_NBD_PROG=$(which false) \
> QSD_PROG=$(which false) \
> ./check 297
> ```
>
> And, no, I don’t want that!  But the point of this series seems to just
> be to rip the core of 297 out so it can run without ./check, because
> ./check requires some environment variables to be set. Doing that seems
> just seems wrong to me.
>
>
Right, the point of this series is effectively to split out the linting
configuration and separate it from the "test" which executes the linters
with that configuration. Simplest possible thing was to just take the
configuration as it exists in its current form -- hardcoded in a python
script -- and isolate it. To my delight, it worked quite well!


> Like, can’t we have a Python script in python/tests that imports
> linters.py, invokes run_linters() and sensibly checks the output? Or,
> you know, at the very least not have run_linters() print anything to
> stdout and not have it return an integer code. linters.py:main() can do
> that conversion.
>
>
Well, I certainly don't want to bother parsing output from python tools and
trying to make sure that it works sensibly cross-version and cross-distro.
The return code being 0/non-zero is vastly simpler. Let the human read the
output log on failure cases to get a sense of what exactly went wrong. Is
there some reason why parsing the output would be beneficial to anyone?

(The Python GitLab CI hooks don't even bother printing output to the
console unless it returns non-zero, and then it will just print whatever it
saw. Seems to work well in practice.)


>
> Or, something completely different, perhaps my problem is that you put
> linters.py as a fully standalone test into the iotests directory,
> without it being an iotest.  So, I think I could also agree on putting
> linters.py into python/tests, and then having 297 execute that.  Or you
> know, we just drop 297 altogether, as you suggest in patch 13 – if
> that’s what it takes, then so be it.
>
>
I can definitely drop 297 if you'd like, and work on making the linter
configuration more declarative. I erred on the side of less movement
instead of more so that disruption would be minimal. It might take me some
extra time to work out how to un-scriptify the test, though. I'd like to
get a quick temperature check from kwolf on this before I put the work in.


> Hanna
>
>
> PS: Also, summing up processes’ return codes makes me feel not good.
>
>
That's the part Vladimir didn't like. There was no real reason for it,
other than it was "easy". I can make it a binary 0/1 return instead, if
that'd grease the wheels.

(I'll sit on respinning the series for now until we've had some time to
discuss it. I would rather like a chance to involve kwolf as the other
major user of this subsystem.)


> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index e05c99972e..f9ddfb53a0 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -68,19 +68,22 @@ def run_linters(
> >       files: List[str],
> >       directory: str = '.',
> >       env: Optional[Mapping[str, str]] = None,
> > -) -> None:
> > +) -> int:
> > +    ret = 0
> >
> >       print('=== pylint ===')
> >       sys.stdout.flush()
> >
> >       # Todo notes are fine, but fixme's or xxx's should probably just be
> >       # fixed (in tests, at least)
> > -    subprocess.run(
> > +    p = subprocess.run(
> >           ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX',
> *files),
> >           cwd=directory,
> >           env=env,
> >           check=False,
> > +        universal_newlines=True,
> >       )
> > +    ret += p.returncode
> >
> >       print('=== mypy ===')
> >       sys.stdout.flush()
> > @@ -113,9 +116,12 @@ def run_linters(
> >               universal_newlines=True
> >           )
> >
> > +        ret += p.returncode
> >           if p.returncode != 0:
> >               print(p.stdout)
> >
> > +    return ret
> > +
> >
> >   def main() -> None:
> >       for linter in ('pylint-3', 'mypy'):
>
>

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

Re: [PATCH v3 14/16] iotests/linters: Add workaround for mypy bug #9852

[John Snow]

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

On Fri, Sep 17, 2021 at 7:16 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 16.09.21 06:09, John Snow wrote:
> > This one is insidious: if you use the invocation
> > "from {namespace} import {subpackage}" as mirror-top-perms does,
> > mypy will fail on every-other invocation *if* the package being
> > imported is a package.
> >
> > Now, I could just edit mirror-top-perms to avoid this invocation, but
> > since I tripped on a landmine, I might as well head it off at the pass
> > and make sure nobody else trips on the same landmine.
> >
> > It seems to have something to do with the order in which files are
> > checked as well, meaning the random order in which set(os.listdir())
> > produces the list of files to test will cause problems intermittently.
> >
> > mypy >= 0.920 isn't released yet, so add this workaround for now.
> >
> > See also:
> >   https://github.com/python/mypy/issues/11010
> >   https://github.com/python/mypy/issues/9852
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/linters.py | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/linters.py
> b/tests/qemu-iotests/linters.py
> > index 4df062a973..9c97324e87 100755
> > --- a/tests/qemu-iotests/linters.py
> > +++ b/tests/qemu-iotests/linters.py
> > @@ -100,6 +100,9 @@ def run_linters(
> >                   '--warn-unused-ignores',
> >                   '--no-implicit-reexport',
> >                   '--namespace-packages',
> > +                # Until we can use mypy >= 0.920, see
> > +                # https://github.com/python/mypy/
> <https://github.com/python/mypy/issues/9852>issues
> <https://github.com/python/mypy/issues/9852>/9852
> <https://github.com/python/mypy/issues/9852>
> > +                '--no-incremental',
> >                   filename,
> >               ),
>
> I’m afraid I still don’t really understand this, but I’m happy with this
> given as the reported workaround and you saying it works.
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
> Question is, when “can we use” mypy >= 0.920?  Should we check the
> version string and append this switch as required?
>
>
The answer to that question depends on how the block maintainers feel about
what environments they expect this test to be runnable under. I lightly
teased kwolf once about an "ancient" version of pylint they were running,
but felt kind of bad about it in retrospect: the tests I write should "just
work" for everyone without them needing to really know anything about
python or setting up or managing their dependencies, environments, etc.

(1) We can use it the very moment it is released if you're OK with running
'make check-dev' from the python/ directory. That script sets up its own
virtual environment and manages its own dependencies. If I set a new
minimum version, it will always use it. (Same story for 'make check-tox',
or 'make check-pipenv'. The differences between the tests are primarily on
what other dependencies they have on your environment -- the details are
boring, see python/Makefile for further reading if desired.)

(2) Otherwise, it depends on how people feel about being able to run this
test directly from iotests and what versions of mypy/pylint they are using.
Fedora 33 for instance has 0.782-2.fc33 still, so I can't really "expect"
people to have a bleeding-edge version of mypy unless they went out of
their way to install one themselves. (pip3 install --user --upgrade mypy,
by the way.) Since people are used to running these python scripts
*outside* of a managed environment (using their OS packages directly), I
have largely made every effort to support versions as old as I reasonably
can -- to avoid disruption whenever I possibly can.

So, basically, it kind of depends on if you want to keep 297 or not.
Keeping it implies some additional cost for the sake of maximizing
compatibility. If we ditch it, you can let the scripts in ./python do their
thing and set up their own environments to run tests that should probably
"just work" for everyone.297 could even just be updated to a bash script
that just hopped over to ./python and ran a special avocado command that
ran /only/ the iotest linters, if you wanted. I just felt that step #1 was
to change as little as possible, prove the new approach worked, and then
when folks were comfortable with it drop the old approach.


> Hanna
>
>

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

Re: [PATCH v3 14/16] iotests/linters: Add workaround for mypy bug #9852

[John Snow]

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

On Wed, Sep 22, 2021 at 4:37 PM John Snow <jsnow@redhat.com> wrote:

>
> On Fri, Sep 17, 2021 at 7:16 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
>>
>> Question is, when “can we use” mypy >= 0.920?  Should we check the
>> version string and append this switch as required?
>>
>>
> The answer to that question depends on how the block maintainers feel
> about what environments they expect this test to be runnable under. I
> lightly teased kwolf once about an "ancient" version of pylint they were
> running, but felt kind of bad about it in retrospect: the tests I write
> should "just work" for everyone without them needing to really know
> anything about python or setting up or managing their dependencies,
> environments, etc.
>
> (1) We can use it the very moment it is released if you're OK with running
> 'make check-dev' from the python/ directory. That script sets up its own
> virtual environment and manages its own dependencies. If I set a new
> minimum version, it will always use it. (Same story for 'make check-tox',
> or 'make check-pipenv'. The differences between the tests are primarily on
> what other dependencies they have on your environment -- the details are
> boring, see python/Makefile for further reading if desired.)
>
> (2) Otherwise, it depends on how people feel about being able to run this
> test directly from iotests and what versions of mypy/pylint they are using.
> Fedora 33 for instance has 0.782-2.fc33 still, so I can't really "expect"
> people to have a bleeding-edge version of mypy unless they went out of
> their way to install one themselves. (pip3 install --user --upgrade mypy,
> by the way.) Since people are used to running these python scripts
> *outside* of a managed environment (using their OS packages directly), I
> have largely made every effort to support versions as old as I reasonably
> can -- to avoid disruption whenever I possibly can.
>
> So, basically, it kind of depends on if you want to keep 297 or not.
> Keeping it implies some additional cost for the sake of maximizing
> compatibility. If we ditch it, you can let the scripts in ./python do their
> thing and set up their own environments to run tests that should probably
> "just work" for everyone.297 could even just be updated to a bash script
> that just hopped over to ./python and ran a special avocado command that
> ran /only/ the iotest linters, if you wanted. I just felt that step #1 was
> to change as little as possible, prove the new approach worked, and then
> when folks were comfortable with it drop the old approach.
>
>

Oh, uh, and to answer your more concrete question: Nah, we don't need to
conditionally append this workaround. The speed lost from making the check
incremental is made up for by not invoking the tool 20 times, so it's OK to
just unconditionally add it for now.

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

Re: [PATCH v3 16/16] iotests/linters: check mypy files all at once

[John Snow]

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

On Fri, Sep 17, 2021 at 7:23 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 16.09.21 06:09, John Snow wrote:
> > 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.
>
> Is it possible to pull this to the beginning of the series?  Just
> because patch 14 has to make everything quite slow (which might be a
> tiny nuisance in bisecting some day?).
>
>
Reasonable. Yes.

--js

>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/linters.py | 62 ++++++++++++++++-------------------
> >   1 file changed, 29 insertions(+), 33 deletions(-)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>

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

Re: [PATCH v3 11/16] iotests/297: return error code from run_linters()

[Hanna Reitz]

On 22.09.21 22:18, John Snow wrote:
>
>
> On Fri, Sep 17, 2021 at 7:00 AM Hanna Reitz <hreitz@redhat.com 
> <mailto:hreitz@redhat.com>> wrote:

[...]

>
>     As you say, run_linters() to me seems very iotests-specific still: It
>     emits a specific output that is compared against a reference output.
>     Fine for 297, but not fine for a function provided by a
>     linters.py, I’d say.
>
>     I’d prefer run_linters() to return something like a Map[str,
>     Optional[str]], that would map a tool to its output in case of error,
>     i.e. ideally:
>
>     `{'pylint': None, 'mypy': None}`
>
>
> Splitting the test apart into two sub-tests is completely reasonable. 
> Python CI right now has individual tests for pylint, mypy, etc.
>
>     297 could format it something like
>
>     ```
>     for tool, output in run_linters().items():
>          print(f'=== {tool} ===')
>          if output is not None:
>              print(output)
>     ```
>
>     Or something.
>
>     To check for error, you could put a Python script in python/tests
>     that
>     checks `any(output is not None for output in
>     run_linters().values())` or
>     something (and on error print the output).
>
>
>     Pulling out run_linters() into an external file and having it print
>     something to stdout just seems too iotests-centric to me.  I
>     suppose as
>     long as the return code is right (which this patch is for) it should
>     work for Avocado’s simple tests, too (which I don’t like very much
>     either, by the way, because they too seem archaic to me), but,
>     well.  It
>     almost seems like the Avocado test should just run ./check then.
>
>
> Yeh. Ideally, we'd just have a mypy.ini and a pylintrc that configures 
> the tests adequately, and then 297 (or whomever else) would just call 
> the linters which would in turn read the same configuration. This 
> series is somewhat of a stop-gap to measure the temperature of the 
> room to see how important it was to leave 297 inside of iotests. So, I 
> did the conservative thing that's faster to review even if it now 
> looks *slightly* fishy.
>
> As for things being archaic or not ... possibly, but why involve extra 
> complexity if it isn't warranted?

My opinion is that I find an interface of “prints something to stdout 
and returns an integer status code” to be non-intuitive and thus rather 
complex actually.  That’s why I’d prefer different complexity, namely 
one which has a more intuitive interface.

I’m also not sure where the extra complexity would be for a 
`run_linters() -> Map[str, Optional[str]]`, because 297 just needs the 
loop suggested above over `run_linters().items()`, and as for the 
Avocado test...

> a simple pass/fail works perfectly well.

I don’t find `any(error_msg for error_msg in run_linters().values())` 
much more complex than pass/fail.

(Note: Above, I called it `output`.  Probably should have called it 
`error_msg` like I did now to clarify that it’s `None` in case of 
success and a string otherwise.)

> (And the human can read the output to understand WHY it failed.) If 
> you want more rigorous analytics for some reason, we can discuss the 
> use cases and figure out how to represent that information, but that's 
> definitely a bit beyond scope here.

[...]

>     Like, can’t we have a Python script in python/tests that imports
>     linters.py, invokes run_linters() and sensibly checks the output? Or,
>     you know, at the very least not have run_linters() print anything to
>     stdout and not have it return an integer code. linters.py:main()
>     can do
>     that conversion.
>
>
> Well, I certainly don't want to bother parsing output from python 
> tools and trying to make sure that it works sensibly cross-version and 
> cross-distro. The return code being 0/non-zero is vastly simpler. Let 
> the human read the output log on failure cases to get a sense of what 
> exactly went wrong. Is there some reason why parsing the output would 
> be beneficial to anyone?

Perhaps there was a misunderstanding here, because there’s no need to 
parse the output in my suggestion.  `run_linters() -> Map[str, 
Optional[str]]` would map a tool name to its potential error output; if 
the tool exited successfully (exit code 0), then that `Optional[str]` 
error output would be `None`.  It would only be something if there was 
an error.

> (The Python GitLab CI hooks don't even bother printing output to the 
> console unless it returns non-zero, and then it will just print 
> whatever it saw. Seems to work well in practice.)
>
>
>     Or, something completely different, perhaps my problem is that you
>     put
>     linters.py as a fully standalone test into the iotests directory,
>     without it being an iotest.  So, I think I could also agree on
>     putting
>     linters.py into python/tests, and then having 297 execute that. 
>     Or you
>     know, we just drop 297 altogether, as you suggest in patch 13 – if
>     that’s what it takes, then so be it.
>
>
> I can definitely drop 297 if you'd like, and work on making the linter 
> configuration more declarative. I erred on the side of less movement 
> instead of more so that disruption would be minimal. It might take me 
> some extra time to work out how to un-scriptify the test, though. I'd 
> like to get a quick temperature check from kwolf on this before I put 
> the work in.

So since we seem to want to keep 297, would it be possible to have 297 
run a linters.py that’s in python/tests?

>     Hanna
>
>
>     PS: Also, summing up processes’ return codes makes me feel not good.
>
>
> That's the part Vladimir didn't like. There was no real reason for it, 
> other than it was "easy".

I very much don’t find it easy, because it’s semantically wrong and thus 
comparatively hard to understand.

> I can make it a binary 0/1 return instead, if that'd grease the wheels.

Well, while I consider it necessary, it doesn’t really make the patch 
more palatable to me.

Hanna

Re: [PATCH v3 07/16] iotests/297: Don't rely on distro-specific linter binaries

[Hanna Reitz]

On 22.09.21 21:53, John Snow wrote:
> (This email just explains python packaging stuff. No action items in 
> here. Skim away.)
>
> On Fri, Sep 17, 2021 at 5:43 AM Hanna Reitz <hreitz@redhat.com 
> <mailto:hreitz@redhat.com>> wrote:
>
>     On 16.09.21 06:09, John Snow wrote:
>     > 'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or
>     "python3 -m
>     > mypy" to access these scripts instead. This style of invocation will
>     > prefer the "correct" tool when run in a virtual environment.
>     >
>     > Note that we still check for "pylint-3" before the test begins
>     -- this
>     > check is now "overly strict", but shouldn't cause anything that was
>     > already running correctly to start failing.
>     >
>     > Signed-off-by: John Snow <jsnow@redhat.com
>     <mailto:jsnow@redhat.com>>
>     > Reviewed-by: Vladimir Sementsov-Ogievskiy
>     <vsementsov@virtuozzo.com <mailto:vsementsov@virtuozzo.com>>
>     > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     > ---
>     >   tests/qemu-iotests/297 | 45
>     ++++++++++++++++++++++++------------------
>     >   1 file changed, 26 insertions(+), 19 deletions(-)
>
>     I know it sounds silly, but to be honest I have no idea if replacing
>     `mypy` by `python3 -m mypy` is correct, because no mypy documentation
>     seems to suggest it.
>
>
> Right, I don't think it's necessarily documented that you can do this. 
> It just happens to be a very convenient way to invoke the same script 
> without needing to know *where* mypy is. You let python figure out 
> where it's going to import mypy from, and it handles the rest.
>
> (This also makes it easier to use things like mypy, pylint etc with an 
> explicitly specified PYTHON interpreter. I don't happen to do that in 
> this patch, but ... we could.)
>
>      From what I understand, that’s generally how Python “binaries” work,
>     though (i.e., installed as a module invokable with `python -m`,
>     and then
>     providing some stub binary that, well, effectively does this, but
>     kind
>     of in a weird way, I just don’t understand it), and none of the
>     parameters seem to be hurt in this conversion, so:
>
>
> Right. Technically, any python package can ask for any number of 
> executables to be installed, but the setuptools packaging ecosystem 
> provides a way to "generate" these based on package configuration. I 
> use a few in our own Python packages. If you look in python/setup.cfg, 
> you'll see stuff like this:
>
> [options.entry_points]
> console_scripts =
>     qom = qemu.qmp.qom:main
>     qom-set = qemu.qmp.qom:QOMSet.entry_point
>     qom-get = qemu.qmp.qom:QOMGet.entry_point
>     qom-list = qemu.qmp.qom:QOMList.entry_point
>     qom-tree = qemu.qmp.qom:QOMTree.entry_point
>     qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
>     qemu-ga-client = qemu.qmp.qemu_ga_client:main
>     qmp-shell = qemu.qmp.qmp_shell:main
>
> These entries cause those weird little binary wrapper scripts to be 
> generated for you when the package is *installed*. So our packages 
> will put 'qmp-shell' and 'qom-tree' into your $PATH*.The stuff to the 
> right of the equals sign is just a pointer to a function that can be 
> executed that implements the CLI command. qemu.qmp.qmp_shell points to 
> the module to import, and ':main' points to the function to run.
>
> The last bit of this is that many, though not all (and there's zero 
> requirement this has to be true), python packages that implement CLI 
> commands will often have a stanza in their __init__.py module that 
> says something like this:
>
> if __name__ == '__main__':
>     do_the_command_line_stuff()
>
> Alternatively, a package can include a literal __main__.py file that 
> python knows to check for, and this module is the one that gets 
> executed for `python3 -m mypackage` invocations. This is what mypy does.
>
> Those are the magical blurbs that allow you to execute a module as if 
> it were a script by running "python3 -m mymodule" -- that hooks 
> directly into that little execution stanza. For python code 
> distributed as packages, that's the real reason to have that little 
> magic stanza -- it provides a convenient way to run stuff without 
> needing to write the incredibly more tedious:
>
> python3 -c "from mypy.__main__ import console_entry; console_entry();"
>
> ... which is quite a bit more porcelain too, depending on how they 
> re/factor the code inside of the package.
>
> Seeing as how mypy explicitly includes a __main__.py file: 
> https://github.com/python/mypy/blob/master/mypy/__main__.py 
> <https://github.com/python/mypy/blob/master/mypy/__main__.py>, I am 
> taking it as a given that they are fully aware of invoking mypy in 
> this fashion, and believe it safe to rely on.

Wow, thanks a lot for this detailed explanation!
> There will be a quiz later.
> (There will not be a quiz.)

I’m ready to fail any test on Python so one day I can get a “Officially 
knows nothing about Python” badge.

Hanna

Re: [PATCH v3 14/16] iotests/linters: Add workaround for mypy bug #9852

[Hanna Reitz]

On 22.09.21 22:37, John Snow wrote:
>
>
> On Fri, Sep 17, 2021 at 7:16 AM Hanna Reitz <hreitz@redhat.com 
> <mailto:hreitz@redhat.com>> wrote:
>
>     On 16.09.21 06:09, John Snow wrote:
>     > This one is insidious: if you use the invocation
>     > "from {namespace} import {subpackage}" as mirror-top-perms does,
>     > mypy will fail on every-other invocation *if* the package being
>     > imported is a package.
>     >
>     > Now, I could just edit mirror-top-perms to avoid this
>     invocation, but
>     > since I tripped on a landmine, I might as well head it off at
>     the pass
>     > and make sure nobody else trips on the same landmine.
>     >
>     > It seems to have something to do with the order in which files are
>     > checked as well, meaning the random order in which set(os.listdir())
>     > produces the list of files to test will cause problems
>     intermittently.
>     >
>     > mypy >= 0.920 isn't released yet, so add this workaround for now.
>     >
>     > See also:
>     > https://github.com/python/mypy/issues/11010
>     <https://github.com/python/mypy/issues/11010>
>     > https://github.com/python/mypy/issues/9852
>     <https://github.com/python/mypy/issues/9852>
>     >
>     > Signed-off-by: John Snow <jsnow@redhat.com
>     <mailto:jsnow@redhat.com>>
>     > ---
>     >   tests/qemu-iotests/linters.py | 3 +++
>     >   1 file changed, 3 insertions(+)
>     >
>     > diff --git a/tests/qemu-iotests/linters.py
>     b/tests/qemu-iotests/linters.py
>     > index 4df062a973..9c97324e87 100755
>     > --- a/tests/qemu-iotests/linters.py
>     > +++ b/tests/qemu-iotests/linters.py
>     > @@ -100,6 +100,9 @@ def run_linters(
>     >                   '--warn-unused-ignores',
>     >                   '--no-implicit-reexport',
>     >                   '--namespace-packages',
>     > +                # Until we can use mypy >= 0.920, see
>     > +                # https://github.com/python/mypy/
>     <https://github.com/python/mypy/issues/9852>issues
>     <https://github.com/python/mypy/issues/9852>/9852
>     <https://github.com/python/mypy/issues/9852>
>     > +                '--no-incremental',
>     >                   filename,
>     >               ),
>
>     I’m afraid I still don’t really understand this, but I’m happy
>     with this
>     given as the reported workaround and you saying it works.
>
>     Reviewed-by: Hanna Reitz <hreitz@redhat.com
>     <mailto:hreitz@redhat.com>>
>
>     Question is, when “can we use” mypy >= 0.920?  Should we check the
>     version string and append this switch as required?
>
>
> The answer to that question depends on how the block maintainers feel 
> about what environments they expect this test to be runnable under. I 
> lightly teased kwolf once about an "ancient" version of pylint they 
> were running, but felt kind of bad about it in retrospect: the tests I 
> write should "just work" for everyone without them needing to really 
> know anything about python or setting up or managing their 
> dependencies, environments, etc.
>
> (1) We can use it the very moment it is released if you're OK with 
> running 'make check-dev' from the python/ directory. That script sets 
> up its own virtual environment and manages its own dependencies. If I 
> set a new minimum version, it will always use it. (Same story for 
> 'make check-tox', or 'make check-pipenv'. The differences between the 
> tests are primarily on what other dependencies they have on your 
> environment -- the details are boring, see python/Makefile for further 
> reading if desired.)
>
> (2) Otherwise, it depends on how people feel about being able to run 
> this test directly from iotests and what versions of mypy/pylint they 
> are using. Fedora 33 for instance has 0.782-2.fc33 still, so I can't 
> really "expect" people to have a bleeding-edge version of mypy unless 
> they went out of their way to install one themselves. (pip3 install 
> --user --upgrade mypy, by the way.) Since people are used to running 
> these python scripts *outside* of a managed environment (using their 
> OS packages directly), I have largely made every effort to support 
> versions as old as I reasonably can -- to avoid disruption whenever I 
> possibly can.
>
> So, basically, it kind of depends on if you want to keep 297 or not. 
> Keeping it implies some additional cost for the sake of maximizing 
> compatibility. If we ditch it, you can let the scripts in ./python do 
> their thing and set up their own environments to run tests that should 
> probably "just work" for everyone.297 could even just be updated to a 
> bash script that just hopped over to ./python and ran a special 
> avocado command that ran /only/ the iotest linters, if you wanted. I 
> just felt that step #1 was to change as little as possible, prove the 
> new approach worked, and then when folks were comfortable with it drop 
> the old approach.

Hm.  So the gist is, since we apparently want to keep 297, we have to 
wait for some indefinite time until in some years or so someone 
remembers this workaround and removes it?

Doesn’t sound quite ideal, but well...

Hanna

Re: [PATCH v3 14/16] iotests/linters: Add workaround for mypy bug #9852

[Hanna Reitz]

On 22.09.21 22:38, John Snow wrote:
>
>
> On Wed, Sep 22, 2021 at 4:37 PM John Snow <jsnow@redhat.com 
> <mailto:jsnow@redhat.com>> wrote:
>
>
>     On Fri, Sep 17, 2021 at 7:16 AM Hanna Reitz <hreitz@redhat.com
>     <mailto:hreitz@redhat.com>> wrote:
>
>
>         Question is, when “can we use” mypy >= 0.920? Should we check the
>         version string and append this switch as required?
>
>
>     The answer to that question depends on how the block maintainers
>     feel about what environments they expect this test to be runnable
>     under. I lightly teased kwolf once about an "ancient" version of
>     pylint they were running, but felt kind of bad about it in
>     retrospect: the tests I write should "just work" for everyone
>     without them needing to really know anything about python or
>     setting up or managing their dependencies, environments, etc.
>
>     (1) We can use it the very moment it is released if you're OK with
>     running 'make check-dev' from the python/ directory. That script
>     sets up its own virtual environment and manages its own
>     dependencies. If I set a new minimum version, it will always use
>     it. (Same story for 'make check-tox', or 'make check-pipenv'. The
>     differences between the tests are primarily on what other
>     dependencies they have on your environment -- the details are
>     boring, see python/Makefile for further reading if desired.)
>
>     (2) Otherwise, it depends on how people feel about being able to
>     run this test directly from iotests and what versions of
>     mypy/pylint they are using. Fedora 33 for instance has
>     0.782-2.fc33 still, so I can't really "expect" people to have a
>     bleeding-edge version of mypy unless they went out of their way to
>     install one themselves. (pip3 install --user --upgrade mypy, by
>     the way.) Since people are used to running these python scripts
>     *outside* of a managed environment (using their OS packages
>     directly), I have largely made every effort to support versions as
>     old as I reasonably can -- to avoid disruption whenever I possibly
>     can.
>
>     So, basically, it kind of depends on if you want to keep 297 or
>     not. Keeping it implies some additional cost for the sake of
>     maximizing compatibility. If we ditch it, you can let the scripts
>     in ./python do their thing and set up their own environments to
>     run tests that should probably "just work" for everyone.297 could
>     even just be updated to a bash script that just hopped over to
>     ./python and ran a special avocado command that ran /only/ the
>     iotest linters, if you wanted. I just felt that step #1 was to
>     change as little as possible, prove the new approach worked, and
>     then when folks were comfortable with it drop the old approach.
>
>
> Oh, uh, and to answer your more concrete question: Nah, we don't need 
> to conditionally append this workaround. The speed lost from making 
> the check incremental is made up for by not invoking the tool 20 
> times, so it's OK to just unconditionally add it for now.

Well, yes, but it could be even faster!

And also it would save us from the pain of waiting and judging when it’s 
reasonable to drop `--no-incremental`, and/or just forgetting that this 
workaround even exists.  If we checked the version string, it would be 
OK to just forget about it, I think.

Hanna

Re: [PATCH v3 07/16] iotests/297: Don't rely on distro-specific linter binaries

[John Snow]

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

On Mon, Oct 4, 2021 at 4:17 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 22.09.21 21:53, John Snow wrote:
> > (This email just explains python packaging stuff. No action items in
> > here. Skim away.)
> >
> > On Fri, Sep 17, 2021 at 5:43 AM Hanna Reitz <hreitz@redhat.com
> > <mailto:hreitz@redhat.com>> wrote:
> >
> >     On 16.09.21 06:09, John Snow wrote:
> >     > 'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or
> >     "python3 -m
> >     > mypy" to access these scripts instead. This style of invocation
> will
> >     > prefer the "correct" tool when run in a virtual environment.
> >     >
> >     > Note that we still check for "pylint-3" before the test begins
> >     -- this
> >     > check is now "overly strict", but shouldn't cause anything that was
> >     > already running correctly to start failing.
> >     >
> >     > Signed-off-by: John Snow <jsnow@redhat.com
> >     <mailto:jsnow@redhat.com>>
> >     > Reviewed-by: Vladimir Sementsov-Ogievskiy
> >     <vsementsov@virtuozzo.com <mailto:vsementsov@virtuozzo.com>>
> >     > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> >     <mailto:philmd@redhat.com>>
> >     > ---
> >     >   tests/qemu-iotests/297 | 45
> >     ++++++++++++++++++++++++------------------
> >     >   1 file changed, 26 insertions(+), 19 deletions(-)
> >
> >     I know it sounds silly, but to be honest I have no idea if replacing
> >     `mypy` by `python3 -m mypy` is correct, because no mypy documentation
> >     seems to suggest it.
> >
> >
> > Right, I don't think it's necessarily documented that you can do this.
> > It just happens to be a very convenient way to invoke the same script
> > without needing to know *where* mypy is. You let python figure out
> > where it's going to import mypy from, and it handles the rest.
> >
> > (This also makes it easier to use things like mypy, pylint etc with an
> > explicitly specified PYTHON interpreter. I don't happen to do that in
> > this patch, but ... we could.)
> >
> >      From what I understand, that’s generally how Python “binaries” work,
> >     though (i.e., installed as a module invokable with `python -m`,
> >     and then
> >     providing some stub binary that, well, effectively does this, but
> >     kind
> >     of in a weird way, I just don’t understand it), and none of the
> >     parameters seem to be hurt in this conversion, so:
> >
> >
> > Right. Technically, any python package can ask for any number of
> > executables to be installed, but the setuptools packaging ecosystem
> > provides a way to "generate" these based on package configuration. I
> > use a few in our own Python packages. If you look in python/setup.cfg,
> > you'll see stuff like this:
> >
> > [options.entry_points]
> > console_scripts =
> >     qom = qemu.qmp.qom:main
> >     qom-set = qemu.qmp.qom:QOMSet.entry_point
> >     qom-get = qemu.qmp.qom:QOMGet.entry_point
> >     qom-list = qemu.qmp.qom:QOMList.entry_point
> >     qom-tree = qemu.qmp.qom:QOMTree.entry_point
> >     qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
> >     qemu-ga-client = qemu.qmp.qemu_ga_client:main
> >     qmp-shell = qemu.qmp.qmp_shell:main
> >
> > These entries cause those weird little binary wrapper scripts to be
> > generated for you when the package is *installed*. So our packages
> > will put 'qmp-shell' and 'qom-tree' into your $PATH*.The stuff to the
> > right of the equals sign is just a pointer to a function that can be
> > executed that implements the CLI command. qemu.qmp.qmp_shell points to
> > the module to import, and ':main' points to the function to run.
> >
> > The last bit of this is that many, though not all (and there's zero
> > requirement this has to be true), python packages that implement CLI
> > commands will often have a stanza in their __init__.py module that
> > says something like this:
> >
> > if __name__ == '__main__':
> >     do_the_command_line_stuff()
> >
> > Alternatively, a package can include a literal __main__.py file that
> > python knows to check for, and this module is the one that gets
> > executed for `python3 -m mypackage` invocations. This is what mypy does.
> >
> > Those are the magical blurbs that allow you to execute a module as if
> > it were a script by running "python3 -m mymodule" -- that hooks
> > directly into that little execution stanza. For python code
> > distributed as packages, that's the real reason to have that little
> > magic stanza -- it provides a convenient way to run stuff without
> > needing to write the incredibly more tedious:
> >
> > python3 -c "from mypy.__main__ import console_entry; console_entry();"
> >
> > ... which is quite a bit more porcelain too, depending on how they
> > re/factor the code inside of the package.
> >
> > Seeing as how mypy explicitly includes a __main__.py file:
> > https://github.com/python/mypy/blob/master/mypy/__main__.py
> > <https://github.com/python/mypy/blob/master/mypy/__main__.py>, I am
> > taking it as a given that they are fully aware of invoking mypy in
> > this fashion, and believe it safe to rely on.
>
> Wow, thanks a lot for this detailed explanation!
> > There will be a quiz later.
> > (There will not be a quiz.)
>
> I’m ready to fail any test on Python so one day I can get a “Officially
> knows nothing about Python” badge.
>
>
I can respect that ;)

--js

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

Re: [PATCH v3 11/16] iotests/297: return error code from run_linters()

[John Snow]

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

On Mon, Oct 4, 2021 at 3:45 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 22.09.21 22:18, John Snow wrote:
> >
> >
> > On Fri, Sep 17, 2021 at 7:00 AM Hanna Reitz <hreitz@redhat.com
> > <mailto:hreitz@redhat.com>> wrote:
>
> [...]
>
> >
> >     As you say, run_linters() to me seems very iotests-specific still: It
> >     emits a specific output that is compared against a reference output.
> >     Fine for 297, but not fine for a function provided by a
> >     linters.py, I’d say.
> >
>
>     I’d prefer run_linters() to return something like a Map[str,
> >     Optional[str]], that would map a tool to its output in case of error,
> >     i.e. ideally:
> >
> >     `{'pylint': None, 'mypy': None}`
> >
>
>
> > Splitting the test apart into two sub-tests is completely reasonable.
> > Python CI right now has individual tests for pylint, mypy, etc.
> >
> >     297 could format it something like
> >
> >     ```
> >     for tool, output in run_linters().items():
> >          print(f'=== {tool} ===')
> >          if output is not None:
> >              print(output)
> >     ```
> >
> >     Or something.
> >
> >     To check for error, you could put a Python script in python/tests
> >     that
> >     checks `any(output is not None for output in
> >     run_linters().values())` or
> >     something (and on error print the output).
> >
> >
> >     Pulling out run_linters() into an external file and having it print
> >     something to stdout just seems too iotests-centric to me.  I
> >     suppose as
> >     long as the return code is right (which this patch is for) it should
> >     work for Avocado’s simple tests, too (which I don’t like very much
> >     either, by the way, because they too seem archaic to me), but,
> >     well.  It
> >     almost seems like the Avocado test should just run ./check then.
> >
> >
> > Yeh. Ideally, we'd just have a mypy.ini and a pylintrc that configures
> > the tests adequately, and then 297 (or whomever else) would just call
> > the linters which would in turn read the same configuration. This
> > series is somewhat of a stop-gap to measure the temperature of the
> > room to see how important it was to leave 297 inside of iotests. So, I
> > did the conservative thing that's faster to review even if it now
> > looks *slightly* fishy.
> >
> > As for things being archaic or not ... possibly, but why involve extra
> > complexity if it isn't warranted?
>
> My opinion is that I find an interface of “prints something to stdout
> and returns an integer status code” to be non-intuitive and thus rather
> complex actually.  That’s why I’d prefer different complexity, namely
> one which has a more intuitive interface.
>
>
I'm not sure I follow, though, because ultimately what we're trying to do
is run terminal commands as part of a broader test suite. Returning status
codes and printing output is what they do. We can't escape that paradigm,
so is it really necessary to abstract away from it?


> I’m also not sure where the extra complexity would be for a
> `run_linters() -> Map[str, Optional[str]]`, because 297 just needs the
> loop suggested above over `run_linters().items()`, and as for the
> Avocado test...
>
> > a simple pass/fail works perfectly well.
>
> I don’t find `any(error_msg for error_msg in run_linters().values())`
> much more complex than pass/fail.
>
> (Note: Above, I called it `output`.  Probably should have called it
> `error_msg` like I did now to clarify that it’s `None` in case of
> success and a string otherwise.)
>
> > (And the human can read the output to understand WHY it failed.) If
> > you want more rigorous analytics for some reason, we can discuss the
> > use cases and figure out how to represent that information, but that's
> > definitely a bit beyond scope here.
>
> [...]
>
> >     Like, can’t we have a Python script in python/tests that imports
> >     linters.py, invokes run_linters() and sensibly checks the output? Or,
> >     you know, at the very least not have run_linters() print anything to
> >     stdout and not have it return an integer code. linters.py:main()
> >     can do
> >     that conversion.
> >
> >
> > Well, I certainly don't want to bother parsing output from python
> > tools and trying to make sure that it works sensibly cross-version and
> > cross-distro. The return code being 0/non-zero is vastly simpler. Let
> > the human read the output log on failure cases to get a sense of what
> > exactly went wrong. Is there some reason why parsing the output would
> > be beneficial to anyone?
>
> Perhaps there was a misunderstanding here, because there’s no need to
> parse the output in my suggestion.  `run_linters() -> Map[str,
> Optional[str]]` would map a tool name to its potential error output; if
> the tool exited successfully (exit code 0), then that `Optional[str]`
> error output would be `None`.  It would only be something if there was
> an error.
>
>
Misunderstood based on "checks the output." I might still be approaching
this from the standpoint of "I don't see a reason to capture the output" --
beyond letting iotests use it for the diff phase at the end, but I don't
think I need to encapsulate it in a return value anywhere for that to
happen -- I can just let it print to sys.[stdout|stderr] and let the diff
handle the rest, right?

Is there specific value in replicating that 'diff' feature ourselves? We
already don't do that, so is it really necessary for me to begin doing it?


> > (The Python GitLab CI hooks don't even bother printing output to the
> > console unless it returns non-zero, and then it will just print
> > whatever it saw. Seems to work well in practice.)
> >
> >
> >     Or, something completely different, perhaps my problem is that you
> >     put
> >     linters.py as a fully standalone test into the iotests directory,
> >     without it being an iotest.  So, I think I could also agree on
> >     putting
> >     linters.py into python/tests, and then having 297 execute that.
> >     Or you
> >     know, we just drop 297 altogether, as you suggest in patch 13 – if
> >     that’s what it takes, then so be it.
> >
> >
> > I can definitely drop 297 if you'd like, and work on making the linter
> > configuration more declarative. I erred on the side of less movement
> > instead of more so that disruption would be minimal. It might take me
> > some extra time to work out how to un-scriptify the test, though. I'd
> > like to get a quick temperature check from kwolf on this before I put
> > the work in.
>
> So since we seem to want to keep 297, would it be possible to have 297
> run a linters.py that’s in python/tests?
>
>
Maybe ... I felt like maybe that'd be a bad idea, though, because it puts
an iotest-related thing quite far away from the iotests directory. I didn't
want anyone to have to hunt for this stuff. I try to explain my case for
this a bit better in the commit messages for v2.

I'm sympathetic to the dislike of having something "test-like, but isn't an
iotest" in the folder, though, and tried to address that in v2, but I'm not
confident it'll be to your satisfaction.


> >     Hanna
> >
> >
> >     PS: Also, summing up processes’ return codes makes me feel not good.
> >
> >
> > That's the part Vladimir didn't like. There was no real reason for it,
> > other than it was "easy".
>
> I very much don’t find it easy, because it’s semantically wrong and thus
> comparatively hard to understand.
>
> > I can make it a binary 0/1 return instead, if that'd grease the wheels.
>
> Well, while I consider it necessary, it doesn’t really make the patch
> more palatable to me.
>
>
OK, I am going to send a V2 that may-or-may-not precisely address your core
critique, but I think it's quite a bit tidier and goes quite a bit further
than what I did here in V1. I think I am still misunderstanding a core
complaint here, but I tried to address the things I thought I grokked:
Separate mypy and pylint tests, no funky return code manipulation, no
iotest prints inside of linters.py, etc. If it's still untenable for you,
I'll just have to go from there.

--js

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