* [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv
2021-09-23 0:16 [PATCH 0/6] iotests: update environment and linting configuration John Snow
@ 2021-09-23 0:16 ` John Snow
2021-09-23 10:33 ` Philippe Mathieu-Daudé
2021-09-23 15:19 ` Vladimir Sementsov-Ogievskiy
2021-09-23 0:16 ` [PATCH 2/6] iotests: add warning for rogue 'qemu' packages John Snow
` (5 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: John Snow @ 2021-09-23 0:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, John Snow,
qemu-block
We can drop the sys.path hacking in various places by doing
this. Additionally, by doing it in one place right up top, we can print
interesting warnings in case the environment does not look correct.
If we ever decide to change how the environment is crafted, all of the
"help me find my python packages" goop is all in one place, right in one
function.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/235 | 2 --
tests/qemu-iotests/297 | 6 ------
tests/qemu-iotests/300 | 7 +++----
tests/qemu-iotests/iotests.py | 2 --
tests/qemu-iotests/testenv.py | 14 +++++++++-----
tests/qemu-iotests/tests/mirror-top-perms | 7 +++----
6 files changed, 15 insertions(+), 23 deletions(-)
diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index 8aed45f9a76..4de920c3801 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -24,8 +24,6 @@ import os
import iotests
from iotests import qemu_img_create, qemu_io, file_path, log
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-
from qemu.machine import QEMUMachine
iotests.script_initialize(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b04cba53667..467b712280e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -68,12 +68,6 @@ def run_linters():
# Todo notes are fine, but fixme's or xxx's should probably just be
# fixed (in tests, at least)
env = os.environ.copy()
- qemu_module_path = os.path.join(os.path.dirname(__file__),
- '..', '..', 'python')
- try:
- env['PYTHONPATH'] += os.pathsep + qemu_module_path
- except KeyError:
- env['PYTHONPATH'] = qemu_module_path
subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
env=env, check=False)
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index fe94de84edd..10f9f2a8da6 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -24,12 +24,11 @@ import random
import re
from typing import Dict, List, Optional
-import iotests
-
-# Import qemu after iotests.py has amended sys.path
-# pylint: disable=wrong-import-order
from qemu.machine import machine
+import iotests
+
+
BlockBitmapMapping = List[Dict[str, object]]
mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ce06cf56304..b06ad76e0c5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -36,8 +36,6 @@
from contextlib import contextmanager
-# pylint: disable=import-error, wrong-import-position
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import qtest
from qemu.qmp import QMPMessage
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 70da0d60c80..88104dace90 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -108,12 +108,16 @@ def init_directories(self) -> None:
SAMPLE_IMG_DIR
OUTPUT_DIR
"""
+
+ # Path where qemu goodies live in this source tree.
+ qemu_srctree_path = Path(__file__, '../../../python').resolve()
+
self.pythonpath = os.getenv('PYTHONPATH')
- if self.pythonpath:
- self.pythonpath = self.source_iotests + os.pathsep + \
- self.pythonpath
- else:
- self.pythonpath = self.source_iotests
+ self.pythonpath = os.pathsep.join(filter(None, (
+ self.source_iotests,
+ str(qemu_srctree_path),
+ self.pythonpath,
+ )))
self.test_dir = os.getenv('TEST_DIR',
os.path.join(os.getcwd(), 'scratch'))
diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index 2fc8dd66e0a..73138a0ef91 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -20,13 +20,12 @@
#
import os
+
+import qemu
+
import iotests
from iotests import qemu_img
-# Import qemu after iotests.py has amended sys.path
-# pylint: disable=wrong-import-order
-import qemu
-
image_size = 1 * 1024 * 1024
source = os.path.join(iotests.test_dir, 'source.img')
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv
2021-09-23 0:16 ` [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv John Snow
@ 2021-09-23 10:33 ` Philippe Mathieu-Daudé
2021-09-23 15:19 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-23 10:33 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-block
On 9/23/21 02:16, John Snow wrote:
> We can drop the sys.path hacking in various places by doing
> this. Additionally, by doing it in one place right up top, we can print
> interesting warnings in case the environment does not look correct.
>
> If we ever decide to change how the environment is crafted, all of the
> "help me find my python packages" goop is all in one place, right in one
> function.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/235 | 2 --
> tests/qemu-iotests/297 | 6 ------
> tests/qemu-iotests/300 | 7 +++----
> tests/qemu-iotests/iotests.py | 2 --
> tests/qemu-iotests/testenv.py | 14 +++++++++-----
> tests/qemu-iotests/tests/mirror-top-perms | 7 +++----
> 6 files changed, 15 insertions(+), 23 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv
2021-09-23 0:16 ` [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv John Snow
2021-09-23 10:33 ` Philippe Mathieu-Daudé
@ 2021-09-23 15:19 ` Vladimir Sementsov-Ogievskiy
2021-09-23 15:46 ` John Snow
1 sibling, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-23 15:19 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block, Hanna Reitz
23.09.2021 03:16, John Snow wrote:
> We can drop the sys.path hacking in various places by doing
> this. Additionally, by doing it in one place right up top, we can print
> interesting warnings in case the environment does not look correct.
>
> If we ever decide to change how the environment is crafted, all of the
> "help me find my python packages" goop is all in one place, right in one
> function.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Hurrah!!
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/235 | 2 --
> tests/qemu-iotests/297 | 6 ------
> tests/qemu-iotests/300 | 7 +++----
> tests/qemu-iotests/iotests.py | 2 --
> tests/qemu-iotests/testenv.py | 14 +++++++++-----
> tests/qemu-iotests/tests/mirror-top-perms | 7 +++----
> 6 files changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> index 8aed45f9a76..4de920c3801 100755
> --- a/tests/qemu-iotests/235
> +++ b/tests/qemu-iotests/235
> @@ -24,8 +24,6 @@ import os
> import iotests
> from iotests import qemu_img_create, qemu_io, file_path, log
>
> -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> -
> from qemu.machine import QEMUMachine
>
> iotests.script_initialize(supported_fmts=['qcow2'])
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index b04cba53667..467b712280e 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -68,12 +68,6 @@ def run_linters():
> # Todo notes are fine, but fixme's or xxx's should probably just be
> # fixed (in tests, at least)
> env = os.environ.copy()
> - qemu_module_path = os.path.join(os.path.dirname(__file__),
> - '..', '..', 'python')
> - try:
> - env['PYTHONPATH'] += os.pathsep + qemu_module_path
> - except KeyError:
> - env['PYTHONPATH'] = qemu_module_path
> subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
> env=env, check=False)
>
> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> index fe94de84edd..10f9f2a8da6 100755
> --- a/tests/qemu-iotests/300
> +++ b/tests/qemu-iotests/300
> @@ -24,12 +24,11 @@ import random
> import re
> from typing import Dict, List, Optional
>
> -import iotests
> -
> -# Import qemu after iotests.py has amended sys.path
> -# pylint: disable=wrong-import-order
> from qemu.machine import machine
>
> +import iotests
> +
> +
> BlockBitmapMapping = List[Dict[str, object]]
>
> mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index ce06cf56304..b06ad76e0c5 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -36,8 +36,6 @@
>
> from contextlib import contextmanager
>
> -# pylint: disable=import-error, wrong-import-position
> -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> from qemu.machine import qtest
> from qemu.qmp import QMPMessage
>
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 70da0d60c80..88104dace90 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -108,12 +108,16 @@ def init_directories(self) -> None:
> SAMPLE_IMG_DIR
> OUTPUT_DIR
> """
> +
> + # Path where qemu goodies live in this source tree.
> + qemu_srctree_path = Path(__file__, '../../../python').resolve()
> +
> self.pythonpath = os.getenv('PYTHONPATH')
> - if self.pythonpath:
> - self.pythonpath = self.source_iotests + os.pathsep + \
> - self.pythonpath
> - else:
> - self.pythonpath = self.source_iotests
> + self.pythonpath = os.pathsep.join(filter(None, (
> + self.source_iotests,
> + str(qemu_srctree_path),
> + self.pythonpath,
> + )))
That was simple:)
Hmm, after you this you have
self.pythonpath = ...
self.pythonpath = something other
If have to resend, you may just use os.getenv('PYTHONPATH') inside filter(), it seems to be even nicer.
>
> self.test_dir = os.getenv('TEST_DIR',
> os.path.join(os.getcwd(), 'scratch'))
> diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
> index 2fc8dd66e0a..73138a0ef91 100755
> --- a/tests/qemu-iotests/tests/mirror-top-perms
> +++ b/tests/qemu-iotests/tests/mirror-top-perms
> @@ -20,13 +20,12 @@
> #
>
> import os
> +
> +import qemu
> +
> import iotests
> from iotests import qemu_img
>
> -# Import qemu after iotests.py has amended sys.path
> -# pylint: disable=wrong-import-order
> -import qemu
> -
>
> image_size = 1 * 1024 * 1024
> source = os.path.join(iotests.test_dir, 'source.img')
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv
2021-09-23 15:19 ` Vladimir Sementsov-Ogievskiy
@ 2021-09-23 15:46 ` John Snow
0 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2021-09-23 15:46 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, Hanna Reitz, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 5603 bytes --]
On Thu, Sep 23, 2021 at 11:20 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:
> 23.09.2021 03:16, John Snow wrote:
> > We can drop the sys.path hacking in various places by doing
> > this. Additionally, by doing it in one place right up top, we can print
> > interesting warnings in case the environment does not look correct.
> >
> > If we ever decide to change how the environment is crafted, all of the
> > "help me find my python packages" goop is all in one place, right in one
> > function.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Hurrah!!
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> > ---
> > tests/qemu-iotests/235 | 2 --
> > tests/qemu-iotests/297 | 6 ------
> > tests/qemu-iotests/300 | 7 +++----
> > tests/qemu-iotests/iotests.py | 2 --
> > tests/qemu-iotests/testenv.py | 14 +++++++++-----
> > tests/qemu-iotests/tests/mirror-top-perms | 7 +++----
> > 6 files changed, 15 insertions(+), 23 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> > index 8aed45f9a76..4de920c3801 100755
> > --- a/tests/qemu-iotests/235
> > +++ b/tests/qemu-iotests/235
> > @@ -24,8 +24,6 @@ import os
> > import iotests
> > from iotests import qemu_img_create, qemu_io, file_path, log
> >
> > -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> 'python'))
> > -
> > from qemu.machine import QEMUMachine
> >
> > iotests.script_initialize(supported_fmts=['qcow2'])
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index b04cba53667..467b712280e 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -68,12 +68,6 @@ def run_linters():
> > # Todo notes are fine, but fixme's or xxx's should probably just be
> > # fixed (in tests, at least)
> > env = os.environ.copy()
> > - qemu_module_path = os.path.join(os.path.dirname(__file__),
> > - '..', '..', 'python')
> > - try:
> > - env['PYTHONPATH'] += os.pathsep + qemu_module_path
> > - except KeyError:
> > - env['PYTHONPATH'] = qemu_module_path
> > subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX',
> *files),
> > env=env, check=False)
> >
> > diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> > index fe94de84edd..10f9f2a8da6 100755
> > --- a/tests/qemu-iotests/300
> > +++ b/tests/qemu-iotests/300
> > @@ -24,12 +24,11 @@ import random
> > import re
> > from typing import Dict, List, Optional
> >
> > -import iotests
> > -
> > -# Import qemu after iotests.py has amended sys.path
> > -# pylint: disable=wrong-import-order
> > from qemu.machine import machine
> >
> > +import iotests
> > +
> > +
> > BlockBitmapMapping = List[Dict[str, object]]
> >
> > mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
> > diff --git a/tests/qemu-iotests/iotests.py
> b/tests/qemu-iotests/iotests.py
> > index ce06cf56304..b06ad76e0c5 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -36,8 +36,6 @@
> >
> > from contextlib import contextmanager
> >
> > -# pylint: disable=import-error, wrong-import-position
> > -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> 'python'))
> > from qemu.machine import qtest
> > from qemu.qmp import QMPMessage
> >
> > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> > index 70da0d60c80..88104dace90 100644
> > --- a/tests/qemu-iotests/testenv.py
> > +++ b/tests/qemu-iotests/testenv.py
> > @@ -108,12 +108,16 @@ def init_directories(self) -> None:
> > SAMPLE_IMG_DIR
> > OUTPUT_DIR
> > """
> > +
> > + # Path where qemu goodies live in this source tree.
> > + qemu_srctree_path = Path(__file__, '../../../python').resolve()
> > +
> > self.pythonpath = os.getenv('PYTHONPATH')
> > - if self.pythonpath:
> > - self.pythonpath = self.source_iotests + os.pathsep + \
> > - self.pythonpath
> > - else:
> > - self.pythonpath = self.source_iotests
> > + self.pythonpath = os.pathsep.join(filter(None, (
> > + self.source_iotests,
> > + str(qemu_srctree_path),
> > + self.pythonpath,
> > + )))
>
> That was simple:)
>
>
Only because a very dedicated engineer from Virtuozzo spent so much time
writing a new iotest launcher ;)
> Hmm, after you this you have
>
> self.pythonpath = ...
> self.pythonpath = something other
>
>
> If have to resend, you may just use os.getenv('PYTHONPATH') inside
> filter(), it seems to be even nicer.
>
>
Ah, yeah. I'll just fold that in. Thanks!
> >
> > self.test_dir = os.getenv('TEST_DIR',
> > os.path.join(os.getcwd(), 'scratch'))
> > diff --git a/tests/qemu-iotests/tests/mirror-top-perms
> b/tests/qemu-iotests/tests/mirror-top-perms
> > index 2fc8dd66e0a..73138a0ef91 100755
> > --- a/tests/qemu-iotests/tests/mirror-top-perms
> > +++ b/tests/qemu-iotests/tests/mirror-top-perms
> > @@ -20,13 +20,12 @@
> > #
> >
> > import os
> > +
> > +import qemu
> > +
> > import iotests
> > from iotests import qemu_img
> >
> > -# Import qemu after iotests.py has amended sys.path
> > -# pylint: disable=wrong-import-order
> > -import qemu
> > -
> >
> > image_size = 1 * 1024 * 1024
> > source = os.path.join(iotests.test_dir, 'source.img')
> >
>
>
> --
> Best regards,
> Vladimir
>
>
[-- Attachment #2: Type: text/html, Size: 7783 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/6] iotests: add warning for rogue 'qemu' packages
2021-09-23 0:16 [PATCH 0/6] iotests: update environment and linting configuration John Snow
2021-09-23 0:16 ` [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv John Snow
@ 2021-09-23 0:16 ` John Snow
2021-09-23 10:57 ` Kevin Wolf
2021-09-23 11:09 ` Daniel P. Berrangé
2021-09-23 0:16 ` [PATCH 3/6] iotests/linters: check mypy files all at once John Snow
` (4 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: John Snow @ 2021-09-23 0:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, John Snow,
qemu-block
Add a warning for when 'iotests' runs against a qemu namespace that
isn't the one in the source tree. This might occur if you have
(accidentally) installed the Python namespace package to your local
packages.
(I'm not going to say that this is because I bit myself with this,
but. You can fill in the blanks.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/testenv.py | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 88104dace90..8a43b193af5 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -16,6 +16,8 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
+import importlib.util
+import logging
import os
import sys
import tempfile
@@ -25,7 +27,7 @@
import random
import subprocess
import glob
-from typing import List, Dict, Any, Optional, ContextManager
+from typing import List, Dict, Any, Optional, ContextManager, cast
DEF_GDB_OPTIONS = 'localhost:12345'
@@ -112,6 +114,22 @@ def init_directories(self) -> None:
# Path where qemu goodies live in this source tree.
qemu_srctree_path = Path(__file__, '../../../python').resolve()
+ # warn if we happen to be able to find 'qemu' packages from an
+ # unexpected location (i.e. the package is already installed in
+ # the user's environment)
+ qemu_spec = importlib.util.find_spec('qemu.qmp')
+ if qemu_spec:
+ spec_path = Path(cast(str, qemu_spec.origin))
+ try:
+ _ = spec_path.relative_to(qemu_srctree_path)
+ except ValueError:
+ self._logger.warning(
+ "WARNING: 'qemu' package will be imported from outside "
+ "the source tree!")
+ self._logger.warning(
+ "Importing from: '%s'",
+ spec_path.parents[1])
+
self.pythonpath = os.getenv('PYTHONPATH')
self.pythonpath = os.pathsep.join(filter(None, (
self.source_iotests,
@@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
self.build_root = os.path.join(self.build_iotests, '..', '..')
+ self._logger = logging.getLogger('qemu.iotests')
self.init_directories()
self.init_binaries()
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages
2021-09-23 0:16 ` [PATCH 2/6] iotests: add warning for rogue 'qemu' packages John Snow
@ 2021-09-23 10:57 ` Kevin Wolf
2021-09-23 11:17 ` Kevin Wolf
2021-09-23 11:09 ` Daniel P. Berrangé
1 sibling, 1 reply; 21+ messages in thread
From: Kevin Wolf @ 2021-09-23 10:57 UTC (permalink / raw)
To: John Snow
Cc: Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> Add a warning for when 'iotests' runs against a qemu namespace that
> isn't the one in the source tree. This might occur if you have
> (accidentally) installed the Python namespace package to your local
> packages.
>
> (I'm not going to say that this is because I bit myself with this,
> but. You can fill in the blanks.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/testenv.py | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 88104dace90..8a43b193af5 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -16,6 +16,8 @@
> # along with this program. If not, see <http://www.gnu.org/licenses/>.
> #
>
> +import importlib.util
> +import logging
> import os
> import sys
> import tempfile
> @@ -25,7 +27,7 @@
> import random
> import subprocess
> import glob
> -from typing import List, Dict, Any, Optional, ContextManager
> +from typing import List, Dict, Any, Optional, ContextManager, cast
>
> DEF_GDB_OPTIONS = 'localhost:12345'
>
> @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> # Path where qemu goodies live in this source tree.
> qemu_srctree_path = Path(__file__, '../../../python').resolve()
>
> + # warn if we happen to be able to find 'qemu' packages from an
> + # unexpected location (i.e. the package is already installed in
> + # the user's environment)
> + qemu_spec = importlib.util.find_spec('qemu.qmp')
> + if qemu_spec:
> + spec_path = Path(cast(str, qemu_spec.origin))
You're casting Optional[str] to str here. The source type is not obvious
from the local code, so unless you spend some time actively figuring it
out, this could be casting anything to str. But even knowing this, just
casting Optional away looks fishy anyway.
Looking up the ModuleSpec docs, it seems okay to assume that it's never
None in our case, but wouldn't it be much cleaner to just 'assert
qemu_spec.origin' first and then use it without the cast?
> + try:
> + _ = spec_path.relative_to(qemu_srctree_path)
> + except ValueError:
> + self._logger.warning(
> + "WARNING: 'qemu' package will be imported from outside "
> + "the source tree!")
> + self._logger.warning(
> + "Importing from: '%s'",
> + spec_path.parents[1])
> +
> self.pythonpath = os.getenv('PYTHONPATH')
> self.pythonpath = os.pathsep.join(filter(None, (
> self.source_iotests,
> @@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>
> self.build_root = os.path.join(self.build_iotests, '..', '..')
>
> + self._logger = logging.getLogger('qemu.iotests')
> self.init_directories()
> self.init_binaries()
Looks good otherwise.
Kevin
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages
2021-09-23 10:57 ` Kevin Wolf
@ 2021-09-23 11:17 ` Kevin Wolf
2021-09-23 14:59 ` John Snow
0 siblings, 1 reply; 21+ messages in thread
From: Kevin Wolf @ 2021-09-23 11:17 UTC (permalink / raw)
To: John Snow
Cc: Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Am 23.09.2021 um 12:57 hat Kevin Wolf geschrieben:
> Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> > Add a warning for when 'iotests' runs against a qemu namespace that
> > isn't the one in the source tree. This might occur if you have
> > (accidentally) installed the Python namespace package to your local
> > packages.
> >
> > (I'm not going to say that this is because I bit myself with this,
> > but. You can fill in the blanks.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> > tests/qemu-iotests/testenv.py | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> > index 88104dace90..8a43b193af5 100644
> > --- a/tests/qemu-iotests/testenv.py
> > +++ b/tests/qemu-iotests/testenv.py
> > @@ -16,6 +16,8 @@
> > # along with this program. If not, see <http://www.gnu.org/licenses/>.
> > #
> >
> > +import importlib.util
> > +import logging
> > import os
> > import sys
> > import tempfile
> > @@ -25,7 +27,7 @@
> > import random
> > import subprocess
> > import glob
> > -from typing import List, Dict, Any, Optional, ContextManager
> > +from typing import List, Dict, Any, Optional, ContextManager, cast
> >
> > DEF_GDB_OPTIONS = 'localhost:12345'
> >
> > @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> > # Path where qemu goodies live in this source tree.
> > qemu_srctree_path = Path(__file__, '../../../python').resolve()
> >
> > + # warn if we happen to be able to find 'qemu' packages from an
> > + # unexpected location (i.e. the package is already installed in
> > + # the user's environment)
> > + qemu_spec = importlib.util.find_spec('qemu.qmp')
> > + if qemu_spec:
> > + spec_path = Path(cast(str, qemu_spec.origin))
>
> You're casting Optional[str] to str here. The source type is not obvious
> from the local code, so unless you spend some time actively figuring it
> out, this could be casting anything to str. But even knowing this, just
> casting Optional away looks fishy anyway.
>
> Looking up the ModuleSpec docs, it seems okay to assume that it's never
> None in our case, but wouldn't it be much cleaner to just 'assert
> qemu_spec.origin' first and then use it without the cast?
>
> > + try:
> > + _ = spec_path.relative_to(qemu_srctree_path)
> > + except ValueError:
> > + self._logger.warning(
> > + "WARNING: 'qemu' package will be imported from outside "
> > + "the source tree!")
> > + self._logger.warning(
> > + "Importing from: '%s'",
> > + spec_path.parents[1])
> > +
> > self.pythonpath = os.getenv('PYTHONPATH')
> > self.pythonpath = os.pathsep.join(filter(None, (
> > self.source_iotests,
> > @@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
> >
> > self.build_root = os.path.join(self.build_iotests, '..', '..')
> >
> > + self._logger = logging.getLogger('qemu.iotests')
> > self.init_directories()
> > self.init_binaries()
>
> Looks good otherwise.
Well, actually there is a major problem: We'll pass the right PYTHONPATH
for the test scripts that we're calling, but this script itself doesn't
use it yet. So what I get is:
Traceback (most recent call last):
File "/home/kwolf/source/qemu/tests/qemu-iotests/build/check", line 120, in <module>
env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 253, in __init__
self.init_directories()
File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 120, in init_directories
qemu_spec = importlib.util.find_spec('qemu.qmp')
File "/usr/lib64/python3.9/importlib/util.py", line 94, in find_spec
parent = __import__(parent_name, fromlist=['__path__'])
ModuleNotFoundError: No module named 'qemu'
Kevin
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages
2021-09-23 11:17 ` Kevin Wolf
@ 2021-09-23 14:59 ` John Snow
0 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2021-09-23 14:59 UTC (permalink / raw)
To: Kevin Wolf
Cc: Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 4917 bytes --]
On Thu, Sep 23, 2021 at 7:17 AM Kevin Wolf <kwolf@redhat.com> wrote:
> Am 23.09.2021 um 12:57 hat Kevin Wolf geschrieben:
> > Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> > > Add a warning for when 'iotests' runs against a qemu namespace that
> > > isn't the one in the source tree. This might occur if you have
> > > (accidentally) installed the Python namespace package to your local
> > > packages.
> > >
> > > (I'm not going to say that this is because I bit myself with this,
> > > but. You can fill in the blanks.)
> > >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > > tests/qemu-iotests/testenv.py | 21 ++++++++++++++++++++-
> > > 1 file changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> > > index 88104dace90..8a43b193af5 100644
> > > --- a/tests/qemu-iotests/testenv.py
> > > +++ b/tests/qemu-iotests/testenv.py
> > > @@ -16,6 +16,8 @@
> > > # along with this program. If not, see <http://www.gnu.org/licenses/
> >.
> > > #
> > >
> > > +import importlib.util
> > > +import logging
> > > import os
> > > import sys
> > > import tempfile
> > > @@ -25,7 +27,7 @@
> > > import random
> > > import subprocess
> > > import glob
> > > -from typing import List, Dict, Any, Optional, ContextManager
> > > +from typing import List, Dict, Any, Optional, ContextManager, cast
> > >
> > > DEF_GDB_OPTIONS = 'localhost:12345'
> > >
> > > @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> > > # Path where qemu goodies live in this source tree.
> > > qemu_srctree_path = Path(__file__,
> '../../../python').resolve()
> > >
> > > + # warn if we happen to be able to find 'qemu' packages from an
> > > + # unexpected location (i.e. the package is already installed
> in
> > > + # the user's environment)
> > > + qemu_spec = importlib.util.find_spec('qemu.qmp')
> > > + if qemu_spec:
> > > + spec_path = Path(cast(str, qemu_spec.origin))
> >
> > You're casting Optional[str] to str here. The source type is not obvious
> > from the local code, so unless you spend some time actively figuring it
> > out, this could be casting anything to str. But even knowing this, just
> > casting Optional away looks fishy anyway.
> >
> > Looking up the ModuleSpec docs, it seems okay to assume that it's never
> > None in our case, but wouldn't it be much cleaner to just 'assert
> > qemu_spec.origin' first and then use it without the cast?
> >
>
OK. I suppose I was thinking: "It's always going to be a string, and if it
isn't, something else will explode below, surely, so the assertion is
redundant", but I don't want code that makes people wonder, so principle of
least surprise it is.
> > > + try:
> > > + _ = spec_path.relative_to(qemu_srctree_path)
> > > + except ValueError:
> > > + self._logger.warning(
> > > + "WARNING: 'qemu' package will be imported from
> outside "
> > > + "the source tree!")
> > > + self._logger.warning(
> > > + "Importing from: '%s'",
> > > + spec_path.parents[1])
> > > +
> > > self.pythonpath = os.getenv('PYTHONPATH')
> > > self.pythonpath = os.pathsep.join(filter(None, (
> > > self.source_iotests,
> > > @@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str,
> aiomode: str,
> > >
> > > self.build_root = os.path.join(self.build_iotests, '..', '..')
> > >
> > > + self._logger = logging.getLogger('qemu.iotests')
> > > self.init_directories()
> > > self.init_binaries()
> >
> > Looks good otherwise.
>
> Well, actually there is a major problem: We'll pass the right PYTHONPATH
> for the test scripts that we're calling, but this script itself doesn't
> use it yet. So what I get is:
>
> Traceback (most recent call last):
> File "/home/kwolf/source/qemu/tests/qemu-iotests/build/check", line 120,
> in <module>
> env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
> File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 253,
> in __init__
> self.init_directories()
> File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 120,
> in init_directories
> qemu_spec = importlib.util.find_spec('qemu.qmp')
> File "/usr/lib64/python3.9/importlib/util.py", line 94, in find_spec
> parent = __import__(parent_name, fromlist=['__path__'])
> ModuleNotFoundError: No module named 'qemu'
>
> Kevin
>
>
Tch. So, it turns out with namespace packages that once you remove the
subpackages (pip uninstall qemu), it leaves the empty namespace folder
behind. This is also the reason I directly use 'qemu.qmp' as a bellwether
here, to make sure we're prodding a package and not just an empty namespace
with nothing in it.
I'll fix these, thanks.
[-- Attachment #2: Type: text/html, Size: 6844 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages
2021-09-23 0:16 ` [PATCH 2/6] iotests: add warning for rogue 'qemu' packages John Snow
2021-09-23 10:57 ` Kevin Wolf
@ 2021-09-23 11:09 ` Daniel P. Berrangé
2021-09-23 15:42 ` John Snow
1 sibling, 1 reply; 21+ messages in thread
From: Daniel P. Berrangé @ 2021-09-23 11:09 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-devel,
qemu-block
On Wed, Sep 22, 2021 at 08:16:21PM -0400, John Snow wrote:
> Add a warning for when 'iotests' runs against a qemu namespace that
> isn't the one in the source tree. This might occur if you have
> (accidentally) installed the Python namespace package to your local
> packages.
IIUC, it is/was a valid use case to run the iotests on arbitrary
QEMU outside the source tree ie a distro packaged binary set.
Are we not allowing the tests/qemu-iotests/* content to be
run outside the context of the main source tree for this
purpose ?
eg consider if Fedora/RHEL builds put tests/qemu-iotests/* into
a 'qemu-iotests' RPM, which was installed and used with a distro
package python-qemu ?
> (I'm not going to say that this is because I bit myself with this,
> but. You can fill in the blanks.)
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/testenv.py | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 88104dace90..8a43b193af5 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -16,6 +16,8 @@
> # along with this program. If not, see <http://www.gnu.org/licenses/>.
> #
>
> +import importlib.util
> +import logging
> import os
> import sys
> import tempfile
> @@ -25,7 +27,7 @@
> import random
> import subprocess
> import glob
> -from typing import List, Dict, Any, Optional, ContextManager
> +from typing import List, Dict, Any, Optional, ContextManager, cast
>
> DEF_GDB_OPTIONS = 'localhost:12345'
>
> @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> # Path where qemu goodies live in this source tree.
> qemu_srctree_path = Path(__file__, '../../../python').resolve()
>
> + # warn if we happen to be able to find 'qemu' packages from an
> + # unexpected location (i.e. the package is already installed in
> + # the user's environment)
> + qemu_spec = importlib.util.find_spec('qemu.qmp')
> + if qemu_spec:
> + spec_path = Path(cast(str, qemu_spec.origin))
> + try:
> + _ = spec_path.relative_to(qemu_srctree_path)
> + except ValueError:
> + self._logger.warning(
> + "WARNING: 'qemu' package will be imported from outside "
> + "the source tree!")
> + self._logger.warning(
> + "Importing from: '%s'",
> + spec_path.parents[1])
It feels to me like the scenario we're blocking here is actally
the scenario we want to aim for.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages
2021-09-23 11:09 ` Daniel P. Berrangé
@ 2021-09-23 15:42 ` John Snow
0 siblings, 0 replies; 21+ messages in thread
From: John Snow @ 2021-09-23 15:42 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-devel,
qemu-block
[-- Attachment #1: Type: text/plain, Size: 5644 bytes --]
On Thu, Sep 23, 2021 at 7:09 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:
> On Wed, Sep 22, 2021 at 08:16:21PM -0400, John Snow wrote:
> > Add a warning for when 'iotests' runs against a qemu namespace that
> > isn't the one in the source tree. This might occur if you have
> > (accidentally) installed the Python namespace package to your local
> > packages.
>
> IIUC, it is/was a valid use case to run the iotests on arbitrary
> QEMU outside the source tree ie a distro packaged binary set.
>
> Are we not allowing the tests/qemu-iotests/* content to be
> run outside the context of the main source tree for this
> purpose ?
>
> eg consider if Fedora/RHEL builds put tests/qemu-iotests/* into
> a 'qemu-iotests' RPM, which was installed and used with a distro
> package python-qemu ?
>
>
(1) "isn't the one in the source tree" is imprecise language here. The real
key is that it must be the QEMU namespace located at "
testenv.py/../../../python/qemu". This usually means the source tree, but
it'd work in any identically structured filesystem hierarchy.
(2) The preceding patch might help illustrate this. At present the iotests
expect to be able to find the 'qemu' python packages by navigating
directories starting from their own location (and not CWD). What patch 1
does is to centralize the logic that has to go "searching" for the python
packages into the init_directories method in testenv.py so that each
individual test doesn't have to repeat it.
i.e. before patch 1, iotests.py does this:
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
'python'))
so we'll always crawl to ../../python from wherever iotests.py is.
After patch 1, we're going to crawl to the same location, but starting from
testenv.py instead. testenv.py and iotests.py are in the same directory, so
I expect whatever worked before (and in whatever environment) will continue
working after. I can't say with full certainty in what circumstances we
support running iotests, but I don't think I have introduced any new
limitation with this.
> > (I'm not going to say that this is because I bit myself with this,
> > but. You can fill in the blanks.)
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> > tests/qemu-iotests/testenv.py | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> > index 88104dace90..8a43b193af5 100644
> > --- a/tests/qemu-iotests/testenv.py
> > +++ b/tests/qemu-iotests/testenv.py
> > @@ -16,6 +16,8 @@
> > # along with this program. If not, see <http://www.gnu.org/licenses/>.
> > #
> >
> > +import importlib.util
> > +import logging
> > import os
> > import sys
> > import tempfile
> > @@ -25,7 +27,7 @@
> > import random
> > import subprocess
> > import glob
> > -from typing import List, Dict, Any, Optional, ContextManager
> > +from typing import List, Dict, Any, Optional, ContextManager, cast
> >
> > DEF_GDB_OPTIONS = 'localhost:12345'
> >
> > @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> > # Path where qemu goodies live in this source tree.
> > qemu_srctree_path = Path(__file__, '../../../python').resolve()
> >
> > + # warn if we happen to be able to find 'qemu' packages from an
> > + # unexpected location (i.e. the package is already installed in
> > + # the user's environment)
> > + qemu_spec = importlib.util.find_spec('qemu.qmp')
> > + if qemu_spec:
> > + spec_path = Path(cast(str, qemu_spec.origin))
> > + try:
> > + _ = spec_path.relative_to(qemu_srctree_path)
> > + except ValueError:
> > + self._logger.warning(
> > + "WARNING: 'qemu' package will be imported from
> outside "
> > + "the source tree!")
> > + self._logger.warning(
> > + "Importing from: '%s'",
> > + spec_path.parents[1])
>
> It feels to me like the scenario we're blocking here is actally
> the scenario we want to aim for.
>
>
It isn't blocking it, it's just a warning. At present, iotests expects that
it's using the version of the python packages that are in the source tree /
tarball / whatever. Since I converted those packages to be installable to
your environment, I introduced an ambiguity about which version iotests is
actually using: the version you installed, or the version in the source
tree? This is just merely a warning to let you know that iotests is
technically doing something new. ("Hey, you're running some other python
code than what iotests has done for the past ten years. Are you cool with
that?")
You're right, though: my actual end-goal (after a lot of pending cleanups I
have in-flight, I am getting to it ...!) is to eventually pivot iotests to
always using qemu as an installed package and wean it off of using
directory-crawling to find its dependencies. We are close to doing that.
When we do transition to that, the warning can be dropped as there will no
longer be an ambiguity over which version it is using. There are some
questions for me to ponder first over exactly how the environment setup
should be accomplished, though.
> Regards,
> Daniel
> --
> |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o-
> https://www.instagram.com/dberrange :|
>
>
[-- Attachment #2: Type: text/html, Size: 7829 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/6] iotests/linters: check mypy files all at once
2021-09-23 0:16 [PATCH 0/6] iotests: update environment and linting configuration John Snow
2021-09-23 0:16 ` [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv John Snow
2021-09-23 0:16 ` [PATCH 2/6] iotests: add warning for rogue 'qemu' packages John Snow
@ 2021-09-23 0:16 ` John Snow
2021-09-23 10:33 ` Philippe Mathieu-Daudé
2021-09-23 15:39 ` Vladimir Sementsov-Ogievskiy
2021-09-23 0:16 ` [PATCH 4/6] iotests/mirror-top-perms: Adjust imports John Snow
` (3 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: John Snow @ 2021-09-23 0:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, John Snow,
qemu-block
We can circumvent the '__main__' redefinition problem by passing
--scripts-are-modules. Take mypy out of the loop per-filename and check
everything in one go: it's quite a bit faster.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
tests/qemu-iotests/297 | 44 +++++++++++++++++++-----------------------
1 file changed, 20 insertions(+), 24 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 467b712280e..91ec34d9521 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -74,32 +74,28 @@ def run_linters():
print('=== mypy ===')
sys.stdout.flush()
- # We have to call mypy separately for each file. Otherwise, it
- # will interpret all given files as belonging together (i.e., they
- # may not both define the same classes, etc.; most notably, they
- # must not both define the __main__ module).
env['MYPYPATH'] = env['PYTHONPATH']
- for filename in files:
- p = subprocess.run(('mypy',
- '--warn-unused-configs',
- '--disallow-subclassing-any',
- '--disallow-any-generics',
- '--disallow-incomplete-defs',
- '--disallow-untyped-decorators',
- '--no-implicit-optional',
- '--warn-redundant-casts',
- '--warn-unused-ignores',
- '--no-implicit-reexport',
- '--namespace-packages',
- filename),
- env=env,
- check=False,
- stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT,
- universal_newlines=True)
+ p = subprocess.run(('mypy',
+ '--warn-unused-configs',
+ '--disallow-subclassing-any',
+ '--disallow-any-generics',
+ '--disallow-incomplete-defs',
+ '--disallow-untyped-decorators',
+ '--no-implicit-optional',
+ '--warn-redundant-casts',
+ '--warn-unused-ignores',
+ '--no-implicit-reexport',
+ '--namespace-packages',
+ '--scripts-are-modules',
+ *files),
+ env=env,
+ check=False,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ universal_newlines=True)
- if p.returncode != 0:
- print(p.stdout)
+ if p.returncode != 0:
+ print(p.stdout)
for linter in ('pylint-3', 'mypy'):
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/6] iotests/linters: check mypy files all at once
2021-09-23 0:16 ` [PATCH 3/6] iotests/linters: check mypy files all at once John Snow
@ 2021-09-23 10:33 ` Philippe Mathieu-Daudé
2021-09-23 15:39 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-23 10:33 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-block
On 9/23/21 02:16, 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.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> ---
> tests/qemu-iotests/297 | 44 +++++++++++++++++++-----------------------
> 1 file changed, 20 insertions(+), 24 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/6] iotests/linters: check mypy files all at once
2021-09-23 0:16 ` [PATCH 3/6] iotests/linters: check mypy files all at once John Snow
2021-09-23 10:33 ` Philippe Mathieu-Daudé
@ 2021-09-23 15:39 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-23 15:39 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block, Hanna Reitz
23.09.2021 03:16, 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.
>
> Signed-off-by: John Snow<jsnow@redhat.com>
> Reviewed-by: Hanna Reitz<hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/6] iotests/mirror-top-perms: Adjust imports
2021-09-23 0:16 [PATCH 0/6] iotests: update environment and linting configuration John Snow
` (2 preceding siblings ...)
2021-09-23 0:16 ` [PATCH 3/6] iotests/linters: check mypy files all at once John Snow
@ 2021-09-23 0:16 ` John Snow
2021-09-23 15:40 ` Vladimir Sementsov-Ogievskiy
2021-09-23 0:16 ` [PATCH 5/6] iotests/migrate-bitmaps-test: delint John Snow
` (2 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: John Snow @ 2021-09-23 0:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block, John Snow,
Hanna Reitz, Philippe Mathieu-Daudé
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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
tests/qemu-iotests/tests/mirror-top-perms | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index 73138a0ef91..3d475aa3a54 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,7 +21,8 @@
import os
-import qemu
+from qemu import qmp
+from qemu.machine import machine
import iotests
from iotests import qemu_img
@@ -46,7 +47,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
def tearDown(self):
try:
self.vm.shutdown()
- except qemu.machine.machine.AbnormalShutdown:
+ except machine.AbnormalShutdown:
pass
if self.vm_b is not None:
@@ -101,7 +102,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.launch()
print('ERROR: VM B launched successfully, this should not have '
'happened')
- except qemu.qmp.QMPConnectError:
+ except qmp.QMPConnectError:
assert 'Is another process using the image' in self.vm_b.get_log()
result = self.vm.qmp('block-job-cancel',
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/6] iotests/mirror-top-perms: Adjust imports
2021-09-23 0:16 ` [PATCH 4/6] iotests/mirror-top-perms: Adjust imports John Snow
@ 2021-09-23 15:40 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-23 15:40 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, qemu-block, Hanna Reitz, Philippe Mathieu-Daudé
23.09.2021 03:16, 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>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> Reviewed-by: Hanna Reitz<hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/6] iotests/migrate-bitmaps-test: delint
2021-09-23 0:16 [PATCH 0/6] iotests: update environment and linting configuration John Snow
` (3 preceding siblings ...)
2021-09-23 0:16 ` [PATCH 4/6] iotests/mirror-top-perms: Adjust imports John Snow
@ 2021-09-23 0:16 ` John Snow
2021-09-23 15:48 ` Vladimir Sementsov-Ogievskiy
2021-09-23 0:16 ` [PATCH 6/6] iotests: Update for pylint 2.11.1 John Snow
2021-09-23 12:57 ` [PATCH 0/6] iotests: update environment and linting configuration Kevin Wolf
6 siblings, 1 reply; 21+ messages in thread
From: John Snow @ 2021-09-23 0:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block, John Snow,
Hanna Reitz, Philippe Mathieu-Daudé
Mostly uninteresting stuff. Move the test injections under a function
named main() so that the variables used during that process aren't in
the global scope.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++++++++++--------
1 file changed, 28 insertions(+), 22 deletions(-)
diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test b/tests/qemu-iotests/tests/migrate-bitmaps-test
index dc431c35b35..c23df3d75c7 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-test
@@ -19,10 +19,11 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
-import os
import itertools
import operator
+import os
import re
+
import iotests
from iotests import qemu_img, qemu_img_create, Timeout
@@ -224,25 +225,6 @@ def inject_test_case(klass, suffix, method, *args, **kwargs):
setattr(klass, 'test_' + method + suffix, lambda self: mc(self))
-for cmb in list(itertools.product((True, False), repeat=5)):
- name = ('_' if cmb[0] else '_not_') + 'persistent_'
- name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
- name += '_online' if cmb[2] else '_offline'
- name += '_shared' if cmb[3] else '_nonshared'
- if cmb[4]:
- name += '__pre_shutdown'
-
- inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
- *list(cmb))
-
-for cmb in list(itertools.product((True, False), repeat=2)):
- name = ('_' if cmb[0] else '_not_') + 'persistent_'
- name += ('_' if cmb[1] else '_not_') + 'migbitmap'
-
- inject_test_case(TestDirtyBitmapMigration, name,
- 'do_test_migration_resume_source', *list(cmb))
-
-
class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
def setUp(self):
qemu_img_create('-f', iotests.imgfmt, base_a, size)
@@ -304,6 +286,30 @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
self.assert_qmp(result, 'return', {})
+def main() -> None:
+ for cmb in list(itertools.product((True, False), repeat=5)):
+ name = ('_' if cmb[0] else '_not_') + 'persistent_'
+ name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
+ name += '_online' if cmb[2] else '_offline'
+ name += '_shared' if cmb[3] else '_nonshared'
+ if cmb[4]:
+ name += '__pre_shutdown'
+
+ inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
+ *list(cmb))
+
+ for cmb in list(itertools.product((True, False), repeat=2)):
+ name = ('_' if cmb[0] else '_not_') + 'persistent_'
+ name += ('_' if cmb[1] else '_not_') + 'migbitmap'
+
+ inject_test_case(TestDirtyBitmapMigration, name,
+ 'do_test_migration_resume_source', *list(cmb))
+
+ iotests.main(
+ supported_fmts=['qcow2'],
+ supported_protocols=['file']
+ )
+
+
if __name__ == '__main__':
- iotests.main(supported_fmts=['qcow2'],
- supported_protocols=['file'])
+ main()
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 5/6] iotests/migrate-bitmaps-test: delint
2021-09-23 0:16 ` [PATCH 5/6] iotests/migrate-bitmaps-test: delint John Snow
@ 2021-09-23 15:48 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-23 15:48 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, qemu-block, Hanna Reitz, Philippe Mathieu-Daudé
23.09.2021 03:16, 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>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> Reviewed-by: Hanna Reitz<hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 6/6] iotests: Update for pylint 2.11.1
2021-09-23 0:16 [PATCH 0/6] iotests: update environment and linting configuration John Snow
` (4 preceding siblings ...)
2021-09-23 0:16 ` [PATCH 5/6] iotests/migrate-bitmaps-test: delint John Snow
@ 2021-09-23 0:16 ` John Snow
2021-09-23 15:51 ` Vladimir Sementsov-Ogievskiy
2021-09-23 12:57 ` [PATCH 0/6] iotests: update environment and linting configuration Kevin Wolf
6 siblings, 1 reply; 21+ messages in thread
From: John Snow @ 2021-09-23 0:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, John Snow,
qemu-block
1. Ignore the new f-strings warning, we're not interested in doing a
full conversion at this time.
2. Just mute the unbalanced-tuple-unpacking warning, it's not a real
error in this case and muting the dozens of callsites is just not
worth it.
3. Add encodings to read_text().
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/pylintrc | 6 +++++-
tests/qemu-iotests/testrunner.py | 7 ++++---
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index f2c0b522ac0..8cb4e1d6a6d 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -19,13 +19,17 @@ disable=invalid-name,
too-many-public-methods,
# pylint warns about Optional[] etc. as unsubscriptable in 3.9
unsubscriptable-object,
+ # pylint's static analysis causes false positivies for file_path();
+ # If we really care to make it statically knowable, we'll use mypy.
+ unbalanced-tuple-unpacking,
# Sometimes we need to disable a newly introduced pylint warning.
# Doing so should not produce a warning in older versions of pylint.
bad-option-value,
# These are temporary, and should be removed:
missing-docstring,
too-many-return-statements,
- too-many-statements
+ too-many-statements,
+ consider-using-f-string,
[FORMAT]
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 4a6ec421ed6..a56b6da3968 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -266,12 +266,13 @@ def do_run_test(self, test: str) -> TestResult:
diff=file_diff(str(f_reference), str(f_bad)))
if f_notrun.exists():
- return TestResult(status='not run',
- description=f_notrun.read_text().strip())
+ return TestResult(
+ status='not run',
+ description=f_notrun.read_text(encoding='utf-8').strip())
casenotrun = ''
if f_casenotrun.exists():
- casenotrun = f_casenotrun.read_text()
+ casenotrun = f_casenotrun.read_text(encoding='utf-8')
diff = file_diff(str(f_reference), str(f_bad))
if diff:
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 6/6] iotests: Update for pylint 2.11.1
2021-09-23 0:16 ` [PATCH 6/6] iotests: Update for pylint 2.11.1 John Snow
@ 2021-09-23 15:51 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-23 15:51 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block, Hanna Reitz
23.09.2021 03:16, John Snow wrote:
> 1. Ignore the new f-strings warning, we're not interested in doing a
> full conversion at this time.
>
> 2. Just mute the unbalanced-tuple-unpacking warning, it's not a real
> error in this case and muting the dozens of callsites is just not
> worth it.
>
> 3. Add encodings to read_text().
>
> Signed-off-by: John Snow<jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] iotests: update environment and linting configuration
2021-09-23 0:16 [PATCH 0/6] iotests: update environment and linting configuration John Snow
` (5 preceding siblings ...)
2021-09-23 0:16 ` [PATCH 6/6] iotests: Update for pylint 2.11.1 John Snow
@ 2021-09-23 12:57 ` Kevin Wolf
6 siblings, 0 replies; 21+ messages in thread
From: Kevin Wolf @ 2021-09-23 12:57 UTC (permalink / raw)
To: John Snow
Cc: Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt1
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/375630185
>
> This series partially supersedes:
> [PATCH v3 00/16] python/iotests: Run iotest linters during Python CI'
>
> Howdy, this is good stuff we want even if we aren't yet in agreement
> about the best way to run iotest 297 from CI.
>
> - Update linting config to tolerate pylint 2.11.1
> - Eliminate sys.path hacking in individual test files
> - make mypy execution in test 297 faster
>
> The rest of the actual "run at CI time" stuff can get handled separately
> and later pending some discussion on the other series.
Patch 2 seems to need some more work.
The rest is: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread