[PATCH v4 00/24] python: create installable package

[PATCH v4 00/24] python: create installable package

[John Snow]

This series factors the python/qemu directory as an installable
package. It does not yet actually change the mechanics of how any other
python source in the tree actually consumes it (yet), beyond the import
path. (some import statements change in a few places.)

git: https://gitlab.com/jsnow/qemu/-/commits/python-package-mk3
CI: https://gitlab.com/jsnow/qemu/-/pipelines/254940717
(New CI job: https://gitlab.com/jsnow/qemu/-/jobs/1024230604)

The primary motivation of this series is primarily to formalize our
dependencies on mypy, flake8, isort, and pylint alongside versions that
are known to work. It does this using the setup.cfg and setup.py
files. It also adds explicitly pinned versions (using Pipfile.lock) of
these dependencies that should behave in a repeatable and known way for
developers and CI environments both. Lastly, it enables those CI checks
such that we can enforce Python coding quality checks via the CI tests.

An auxiliary motivation is that this package is formatted in such a way
that it COULD be uploaded to https://pypi.org/project/qemu and installed
independently of qemu.git with `pip install qemu`, but that button
remains *unpushed* and this series *will not* cause any such
releases. We have time to debate finer points like API guarantees and
versioning even after this series is merged.

Some other things this enables that might be of interest:

With the python tooling as a proper package, you can install this
package in editable or production mode to a virtual environment, your
local user environment, or your system packages. The primary benefit of
this is to gain access to QMP tooling regardless of CWD, without needing
to battle sys.path (and confounding other python analysis tools).

For example: when developing, you may go to qemu/python/ and run `make
venv` followed by `pipenv shell` to activate a virtual environment that
contains the qemu python packages. These packages will always reflect
the current version of the source files in the tree. When you are
finished, you can simply exit the shell (^d) to remove these packages
from your python environment.

When not developing, you could install a version of this package to your
environment outright to gain access to the QMP and QEMUMachine classes
for lightweight scripting and testing by using pip: "pip install [--user] ."

TESTING THIS SERIES:

First of all, nothing should change. Without any intervention,
everything should behave exactly as it was before. The only new
information here comes from how to interact with and run the linters
that will be enforcing code quality standards in this subdirectory.

To test those, CD to qemu/python first, and then:

1. Try "make venv && pipenv shell" to get a venv with the package
installed to it in editable mode. Ctrl+d exits this venv shell. While in
this shell, any python script that uses "from qemu.[qmp|machine] import
..." should work correctly regardless of where the script is, or what
your CWD is.

You will need Python 3.6 installed on your system to do this step. For
Fedora: "dnf install python3.6" will do the trick.

2. Try "make check" while still in the shell to run the Python linters
using the venv built in the previous step. This will pull some packages
from PyPI and run pytest, which will in turn execute mypy, flake8, isort
and pylint with the correct arguments.

3. Having exited the shell from above, try "make venv-check". This will
create and update the venv if needed, then run 'make check' within the
context of that shell. It should pass as long as the above did.

4. Still outside of the venv, you may try running "make check". This
will not install anything, but unless you have the right Python
dependencies installed, these tests may fail for you. You might try
using "pip install --user .[devel]" to install the development packages
needed to run the tests successfully to your local user's python
environment. Once done, you will probably want to "pip uninstall qemu"
to remove that package to avoid it interfering with other things.

5. "make distclean" will delete the venv and any temporary files that
may have been created by packaging, installing, testing, etc.

Changelog:

- Moved qemu/machine/accel.py to qemu/utils/accel.py
- Integrated CI patches into this series
- Changed version of package to 0.6.0.0a1
- Misc different changes for import statements in e.g. iotests/VM tests
- Modified iotests invocation of mypy ever so slightly

Reviewer notes:

- The VERSION hack may be imperfect, but at 0.x and without uploading it
  to PyPI, we have *all* the time in the world to fine-tune it later.
- The CI integration may not be perfect, but it is better than *nothing*,
  so I think it's worth doing even in an imperfect state.

John Snow (24):
  python/console_socket: avoid one-letter variable
  iotests/297: add --namespace-packages to mypy arguments
  python: create qemu packages
  python: create utils sub-package
  python: add qemu package installer
  python: add VERSION file
  python: add directory structure README.rst files
  python: Add pipenv support
  python: add pylint import exceptions
  python: move pylintrc into setup.cfg
  python: add pylint to pipenv
  python: move flake8 config to setup.cfg
  python: Add flake8 to pipenv
  python: move mypy.ini into setup.cfg
  python: add mypy to pipenv
  python: move .isort.cfg into setup.cfg
  python/qemu: add isort to pipenv
  python/qemu: add qemu package itself to pipenv
  python: add devel package requirements to setuptools
  python: add pytest and tests
  python: add excluded dirs to flake8 config
  python: add Makefile for some common tasks
  python: add .gitignore
  gitlab: add python linters to CI

 python/PACKAGE.rst                          |  32 +++
 python/README.rst                           |  41 +++
 python/qemu/README.rst                      |   8 +
 python/qemu/machine/README.rst              |   9 +
 python/qemu/qmp/README.rst                  |   9 +
 python/qemu/utils/README.rst                |   9 +
 .gitlab-ci.yml                              |  10 +
 python/.gitignore                           |   9 +
 python/Makefile                             |  35 +++
 python/Pipfile                              |  13 +
 python/Pipfile.lock                         | 285 ++++++++++++++++++++
 python/VERSION                              |   1 +
 python/mypy.ini                             |   4 -
 python/qemu/.flake8                         |   2 -
 python/qemu/.isort.cfg                      |   7 -
 python/qemu/__init__.py                     |  11 -
 python/qemu/machine/__init__.py             |  36 +++
 python/qemu/{ => machine}/console_socket.py |  10 +-
 python/qemu/{ => machine}/machine.py        |  16 +-
 python/qemu/{ => machine}/qtest.py          |   3 +-
 python/qemu/pylintrc                        |  58 ----
 python/qemu/{qmp.py => qmp/__init__.py}     |  12 +-
 python/qemu/utils/__init__.py               |  23 ++
 python/qemu/{ => utils}/accel.py            |   0
 python/setup.cfg                            |  82 ++++++
 python/setup.py                             |  23 ++
 python/tests/test_lint.py                   |  28 ++
 tests/acceptance/boot_linux.py              |   3 +-
 tests/acceptance/virtio-gpu.py              |   2 +-
 tests/acceptance/virtiofs_submounts.py      |   2 +-
 tests/docker/dockerfiles/fedora.docker      |   2 +
 tests/qemu-iotests/297                      |   1 +
 tests/qemu-iotests/300                      |   4 +-
 tests/qemu-iotests/iotests.py               |   2 +-
 tests/vm/aarch64vm.py                       |   2 +-
 tests/vm/basevm.py                          |   2 +-
 36 files changed, 693 insertions(+), 103 deletions(-)
 create mode 100644 python/PACKAGE.rst
 create mode 100644 python/README.rst
 create mode 100644 python/qemu/README.rst
 create mode 100644 python/qemu/machine/README.rst
 create mode 100644 python/qemu/qmp/README.rst
 create mode 100644 python/qemu/utils/README.rst
 create mode 100644 python/.gitignore
 create mode 100644 python/Makefile
 create mode 100644 python/Pipfile
 create mode 100644 python/Pipfile.lock
 create mode 100644 python/VERSION
 delete mode 100644 python/mypy.ini
 delete mode 100644 python/qemu/.flake8
 delete mode 100644 python/qemu/.isort.cfg
 delete mode 100644 python/qemu/__init__.py
 create mode 100644 python/qemu/machine/__init__.py
 rename python/qemu/{ => machine}/console_socket.py (95%)
 rename python/qemu/{ => machine}/machine.py (98%)
 rename python/qemu/{ => machine}/qtest.py (98%)
 delete mode 100644 python/qemu/pylintrc
 rename python/qemu/{qmp.py => qmp/__init__.py} (96%)
 create mode 100644 python/qemu/utils/__init__.py
 rename python/qemu/{ => utils}/accel.py (100%)
 create mode 100644 python/setup.cfg
 create mode 100755 python/setup.py
 create mode 100644 python/tests/test_lint.py

-- 
2.29.2

[PATCH v4 01/24] python/console_socket: avoid one-letter variable

[John Snow]

Fixes pylint warnings.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/console_socket.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index ac21130e446..87237bebef7 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -46,11 +46,11 @@ def __init__(self, address: str, file: Optional[str] = None,
             self._drain_thread = self._thread_start()
 
     def __repr__(self) -> str:
-        s = super().__repr__()
-        s = s.rstrip(">")
-        s = "%s,  logfile=%s, drain_thread=%s>" % (s, self._logfile,
-                                                   self._drain_thread)
-        return s
+        tmp = super().__repr__()
+        tmp = tmp.rstrip(">")
+        tmp = "%s,  logfile=%s, drain_thread=%s>" % (tmp, self._logfile,
+                                                     self._drain_thread)
+        return tmp
 
     def _drain_fn(self) -> None:
         """Drains the socket and runs while the socket is open."""
-- 
2.29.2

[PATCH v4 02/24] iotests/297: add --namespace-packages to mypy arguments

[John Snow]

mypy is kind of weird about how it handles imports. For legacy reasons,
it won't load PEP 420 namespaces, because of logic implemented prior to
that becoming a standard.

So, if you plan on using any, you have to pass
--namespace-packages. Alright, fine.

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index a37910b42d9..433b7323368 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -95,6 +95,7 @@ def run_linters():
                             '--warn-redundant-casts',
                             '--warn-unused-ignores',
                             '--no-implicit-reexport',
+                            '--namespace-packages',
                             filename),
                            env=env,
                            check=False,
-- 
2.29.2

[PATCH v4 03/24] python: create qemu packages

[John Snow]

move python/qemu/*.py to python/qemu/[machine, qmp]/*.py and update import
directives across the tree.

This is done to create a PEP420 namespace package, in which we may
create subpackages. To do this, the namespace directory ("qemu") should
not have any modules in it. Those files will go into new 'machine' and 'qmp'
subpackages instead.

Implement machine/__init__.py making the top-level classes and functions
from its various modules available directly inside the package. Change
qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
library classes are available directly from "qemu.qmp" instead of
"qemu.qmp.qmp".

Signed-off-by: John Snow <jsnow@redhat.com>


---

Note for reviewers: in the next patch, I add a utils sub-package and
move qemu/machine/accel.py to qemu/utils/accel.py. If we like it that
way, we can squash it in here if we want, or just leave it as its own
follow-up patch, I am just leaving it as something that will be easy to
un-do or change for now.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/{qemu => }/.isort.cfg                |  0
 python/qemu/__init__.py                     | 11 ------
 python/qemu/{ => machine}/.flake8           |  0
 python/qemu/machine/__init__.py             | 41 +++++++++++++++++++++
 python/qemu/{ => machine}/accel.py          |  0
 python/qemu/{ => machine}/console_socket.py |  0
 python/qemu/{ => machine}/machine.py        | 16 +++++---
 python/qemu/{ => machine}/pylintrc          |  0
 python/qemu/{ => machine}/qtest.py          |  3 +-
 python/qemu/{qmp.py => qmp/__init__.py}     | 12 +++++-
 tests/acceptance/boot_linux.py              |  3 +-
 tests/qemu-iotests/300                      |  4 +-
 tests/qemu-iotests/iotests.py               |  2 +-
 tests/vm/basevm.py                          |  3 +-
 14 files changed, 70 insertions(+), 25 deletions(-)
 rename python/{qemu => }/.isort.cfg (100%)
 delete mode 100644 python/qemu/__init__.py
 rename python/qemu/{ => machine}/.flake8 (100%)
 create mode 100644 python/qemu/machine/__init__.py
 rename python/qemu/{ => machine}/accel.py (100%)
 rename python/qemu/{ => machine}/console_socket.py (100%)
 rename python/qemu/{ => machine}/machine.py (98%)
 rename python/qemu/{ => machine}/pylintrc (100%)
 rename python/qemu/{ => machine}/qtest.py (99%)
 rename python/qemu/{qmp.py => qmp/__init__.py} (96%)

diff --git a/python/qemu/.isort.cfg b/python/.isort.cfg
similarity index 100%
rename from python/qemu/.isort.cfg
rename to python/.isort.cfg
diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
deleted file mode 100644
index 4ca06c34a41..00000000000
--- a/python/qemu/__init__.py
+++ /dev/null
@@ -1,11 +0,0 @@
-# QEMU library
-#
-# Copyright (C) 2015-2016 Red Hat Inc.
-# Copyright (C) 2012 IBM Corp.
-#
-# Authors:
-#  Fam Zheng <famz@redhat.com>
-#
-# This work is licensed under the terms of the GNU GPL, version 2.  See
-# the COPYING file in the top-level directory.
-#
diff --git a/python/qemu/.flake8 b/python/qemu/machine/.flake8
similarity index 100%
rename from python/qemu/.flake8
rename to python/qemu/machine/.flake8
diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
new file mode 100644
index 00000000000..27b0b19abd3
--- /dev/null
+++ b/python/qemu/machine/__init__.py
@@ -0,0 +1,41 @@
+"""
+QEMU development and testing library.
+
+This library provides a few high-level classes for driving QEMU from a
+test suite, not intended for production use.
+
+- QEMUMachine: Configure and Boot a QEMU VM
+ - QEMUQtestMachine: VM class, with a qtest socket.
+
+- QEMUQtestProtocol: Connect to, send/receive qtest messages.
+
+- list_accel: List available accelerators
+- kvm_available: Probe for KVM support
+- tcg_available: Probe for TCG support
+"""
+
+# Copyright (C) 2020 John Snow for Red Hat Inc.
+# Copyright (C) 2015-2016 Red Hat Inc.
+# Copyright (C) 2012 IBM Corp.
+#
+# Authors:
+#  John Snow <jsnow@redhat.com>
+#  Fam Zheng <fam@euphon.net>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.  See
+# the COPYING file in the top-level directory.
+#
+
+from .accel import kvm_available, list_accel, tcg_available
+from .machine import QEMUMachine
+from .qtest import QEMUQtestMachine, QEMUQtestProtocol
+
+
+__all__ = (
+    'list_accel',
+    'kvm_available',
+    'tcg_available',
+    'QEMUMachine',
+    'QEMUQtestProtocol',
+    'QEMUQtestMachine',
+)
diff --git a/python/qemu/accel.py b/python/qemu/machine/accel.py
similarity index 100%
rename from python/qemu/accel.py
rename to python/qemu/machine/accel.py
diff --git a/python/qemu/console_socket.py b/python/qemu/machine/console_socket.py
similarity index 100%
rename from python/qemu/console_socket.py
rename to python/qemu/machine/console_socket.py
diff --git a/python/qemu/machine.py b/python/qemu/machine/machine.py
similarity index 98%
rename from python/qemu/machine.py
rename to python/qemu/machine/machine.py
index 7a40f4604be..dd6ce289350 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine/machine.py
@@ -38,8 +38,14 @@
     Type,
 )
 
-from . import console_socket, qmp
-from .qmp import QMPMessage, QMPReturnValue, SocketAddrT
+from qemu.qmp import (
+    QEMUMonitorProtocol,
+    QMPMessage,
+    QMPReturnValue,
+    SocketAddrT,
+)
+
+from . import console_socket
 
 
 LOG = logging.getLogger(__name__)
@@ -139,7 +145,7 @@ def __init__(self,
         self._events: List[QMPMessage] = []
         self._iolog: Optional[str] = None
         self._qmp_set = True   # Enable QMP monitor by default.
-        self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
+        self._qmp_connection: Optional[QEMUMonitorProtocol] = None
         self._qemu_full_args: Tuple[str, ...] = ()
         self._temp_dir: Optional[str] = None
         self._launched = False
@@ -315,7 +321,7 @@ def _pre_launch(self) -> None:
             if self._remove_monitor_sockfile:
                 assert isinstance(self._monitor_address, str)
                 self._remove_files.append(self._monitor_address)
-            self._qmp_connection = qmp.QEMUMonitorProtocol(
+            self._qmp_connection = QEMUMonitorProtocol(
                 self._monitor_address,
                 server=True,
                 nickname=self._name
@@ -535,7 +541,7 @@ def set_qmp_monitor(self, enabled: bool = True) -> None:
         self._qmp_set = enabled
 
     @property
-    def _qmp(self) -> qmp.QEMUMonitorProtocol:
+    def _qmp(self) -> QEMUMonitorProtocol:
         if self._qmp_connection is None:
             raise QEMUMachineError("Attempt to access QMP with no connection")
         return self._qmp_connection
diff --git a/python/qemu/pylintrc b/python/qemu/machine/pylintrc
similarity index 100%
rename from python/qemu/pylintrc
rename to python/qemu/machine/pylintrc
diff --git a/python/qemu/qtest.py b/python/qemu/machine/qtest.py
similarity index 99%
rename from python/qemu/qtest.py
rename to python/qemu/machine/qtest.py
index 39a0cf62fe9..53926e434a7 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,8 +26,9 @@
     TextIO,
 )
 
+from qemu.qmp import SocketAddrT
+
 from .machine import QEMUMachine
-from .qmp import SocketAddrT
 
 
 class QEMUQtestProtocol:
diff --git a/python/qemu/qmp.py b/python/qemu/qmp/__init__.py
similarity index 96%
rename from python/qemu/qmp.py
rename to python/qemu/qmp/__init__.py
index 2cd4d43036c..9606248a3d2 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp/__init__.py
@@ -1,4 +1,14 @@
-""" QEMU Monitor Protocol Python class """
+"""
+QEMU Monitor Protocol (QMP) development library & tooling.
+
+This package provides a fairly low-level class for communicating to QMP
+protocol servers, as implemented by QEMU, the QEMU Guest Agent, and the
+QEMU Storage Daemon. This library is not intended for production use.
+
+`QEMUMonitorProtocol` is the primary class of interest, and all errors
+raised derive from `QMPError`.
+"""
+
 # Copyright (C) 2009, 2010 Red Hat Inc.
 #
 # Authors:
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index bcd923bb4a0..212365fd185 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -12,8 +12,7 @@
 
 from avocado_qemu import Test, BUILD_DIR
 
-from qemu.accel import kvm_available
-from qemu.accel import tcg_available
+from qemu.machine import kvm_available, tcg_available
 
 from avocado.utils import cloudinit
 from avocado.utils import network
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 43264d883d7..5c96782ffb9 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -28,7 +28,7 @@ import iotests
 
 # Import qemu after iotests.py has amended sys.path
 # pylint: disable=wrong-import-order
-import qemu
+from qemu.machine import machine
 
 BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
 
@@ -466,7 +466,7 @@ class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
         # the failed migration
         try:
             self.vm_b.shutdown()
-        except qemu.machine.AbnormalShutdown:
+        except machine.AbnormalShutdown:
             pass
 
     def test_aliased_bitmap_name_too_long(self) -> None:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 00be68eca3e..4d27de579eb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,7 +37,7 @@
 
 # pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu import qtest
+from qemu.machine import qtest
 from qemu.qmp import QMPMessage
 
 # Use this logger for logging messages directly from the iotests module
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 00f1d5ca8da..12d08cf2b1b 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -19,8 +19,7 @@
 import time
 import datetime
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.accel import kvm_available
-from qemu.machine import QEMUMachine
+from qemu.machine import kvm_available, QEMUMachine
 import subprocess
 import hashlib
 import argparse
-- 
2.29.2

[PATCH v4 04/24] python: create utils sub-package

[John Snow]

Create a space for miscellaneous things that don't belong strictly in
"qemu.machine" nor "qemu.qmp" packages.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/__init__.py         |  8 --------
 python/qemu/utils/__init__.py           | 23 +++++++++++++++++++++++
 python/qemu/{machine => utils}/accel.py |  0
 tests/acceptance/boot_linux.py          |  2 +-
 tests/acceptance/virtio-gpu.py          |  2 +-
 tests/acceptance/virtiofs_submounts.py  |  2 +-
 tests/vm/aarch64vm.py                   |  2 +-
 tests/vm/basevm.py                      |  3 ++-
 8 files changed, 29 insertions(+), 13 deletions(-)
 create mode 100644 python/qemu/utils/__init__.py
 rename python/qemu/{machine => utils}/accel.py (100%)

diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
index 27b0b19abd3..891a8f784b5 100644
--- a/python/qemu/machine/__init__.py
+++ b/python/qemu/machine/__init__.py
@@ -8,10 +8,6 @@
  - QEMUQtestMachine: VM class, with a qtest socket.
 
 - QEMUQtestProtocol: Connect to, send/receive qtest messages.
-
-- list_accel: List available accelerators
-- kvm_available: Probe for KVM support
-- tcg_available: Probe for TCG support
 """
 
 # Copyright (C) 2020 John Snow for Red Hat Inc.
@@ -26,15 +22,11 @@
 # the COPYING file in the top-level directory.
 #
 
-from .accel import kvm_available, list_accel, tcg_available
 from .machine import QEMUMachine
 from .qtest import QEMUQtestMachine, QEMUQtestProtocol
 
 
 __all__ = (
-    'list_accel',
-    'kvm_available',
-    'tcg_available',
     'QEMUMachine',
     'QEMUQtestProtocol',
     'QEMUQtestMachine',
diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
new file mode 100644
index 00000000000..edf807a93e5
--- /dev/null
+++ b/python/qemu/utils/__init__.py
@@ -0,0 +1,23 @@
+"""
+QEMU development and testing utilities
+
+This library provides a small handful of utilities for performing various tasks
+not directly related to the launching of a VM.
+
+The only module included at present is accel; its public functions are
+repeated here for your convenience:
+
+- list_accel: List available accelerators
+- kvm_available: Probe for KVM support
+- tcg_available: Prove for TCG support
+"""
+
+# pylint: disable=import-error
+from .accel import kvm_available, list_accel, tcg_available
+
+
+__all__ = (
+    'list_accel',
+    'kvm_available',
+    'tcg_available',
+)
diff --git a/python/qemu/machine/accel.py b/python/qemu/utils/accel.py
similarity index 100%
rename from python/qemu/machine/accel.py
rename to python/qemu/utils/accel.py
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 212365fd185..824cf03d5f4 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -12,7 +12,7 @@
 
 from avocado_qemu import Test, BUILD_DIR
 
-from qemu.machine import kvm_available, tcg_available
+from qemu.utils import kvm_available, tcg_available
 
 from avocado.utils import cloudinit
 from avocado.utils import network
diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
index 211f02932f2..73671476aa0 100644
--- a/tests/acceptance/virtio-gpu.py
+++ b/tests/acceptance/virtio-gpu.py
@@ -10,7 +10,7 @@
 from avocado_qemu import exec_command_and_wait_for_pattern
 from avocado_qemu import is_readable_executable_file
 
-from qemu.accel import kvm_available
+from qemu.utils import kvm_available
 
 import os
 import socket
diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 949ca87a837..6b684253a60 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -9,7 +9,7 @@
 from avocado_qemu import wait_for_console_pattern
 from avocado.utils import ssh
 
-from qemu.accel import kvm_available
+from qemu.utils import kvm_available
 
 from boot_linux import BootLinux
 
diff --git a/tests/vm/aarch64vm.py b/tests/vm/aarch64vm.py
index d70ab843b6b..b00cce07eb8 100644
--- a/tests/vm/aarch64vm.py
+++ b/tests/vm/aarch64vm.py
@@ -14,7 +14,7 @@
 import sys
 import subprocess
 import basevm
-from qemu.accel import kvm_available
+from qemu.utils import kvm_available
 
 # This is the config needed for current version of QEMU.
 # This works for both kvm and tcg.
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 12d08cf2b1b..a3867fdf88e 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -19,7 +19,8 @@
 import time
 import datetime
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.machine import kvm_available, QEMUMachine
+from qemu.machine import QEMUMachine
+from qemu.utils import kvm_available
 import subprocess
 import hashlib
 import argparse
-- 
2.29.2

[PATCH v4 05/24] python: add qemu package installer

[John Snow]

Add setup.cfg and setup.py, necessary for installing a package via
pip. Add a rst document explaining the basics of what this package is
for and who to contact for more information. This document will be used
as the landing page for the package on PyPI.

I am not yet using a pyproject.toml style package manifest, because
"editable" installs are not defined by PEP-517 and pip did not have
support for this for some time; I consider the feature necessary for
development.

Use a light-weight setup.py instead.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/PACKAGE.rst | 32 ++++++++++++++++++++++++++++++++
 python/setup.cfg   | 19 +++++++++++++++++++
 python/setup.py    | 23 +++++++++++++++++++++++
 3 files changed, 74 insertions(+)
 create mode 100644 python/PACKAGE.rst
 create mode 100644 python/setup.cfg
 create mode 100755 python/setup.py

diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
new file mode 100644
index 00000000000..0e714c87eb3
--- /dev/null
+++ b/python/PACKAGE.rst
@@ -0,0 +1,32 @@
+QEMU Python Tooling
+===================
+
+This package provides QEMU tooling used by the QEMU project to build,
+configure, and test QEMU. It is not a fully-fledged SDK and it is subject
+to change at any time.
+
+Usage
+-----
+
+The ``qemu.qmp`` subpackage provides a library for communicating with
+QMP servers. The ``qemu.machine`` subpackage offers rudimentary
+facilities for launching and managing QEMU processes. Refer to each
+package's documentation
+(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
+for more information.
+
+Contributing
+------------
+
+This package is maintained by John Snow <jsnow@redhat.com> as part of
+the QEMU source tree. Contributions are welcome and follow the `QEMU
+patch submission process
+<https://wiki.qemu.org/Contribute/SubmitAPatch>`_, which involves
+sending patches to the QEMU development mailing list.
+
+John maintains a `GitLab staging branch
+<https://gitlab.com/jsnow/qemu/-/tree/python>`_, and there is an
+official `GitLab mirror <https://gitlab.com/qemu-project/qemu>`_.
+
+Please report bugs by sending a mail to qemu-devel@nongnu.org and CC
+jsnow@redhat.com.
diff --git a/python/setup.cfg b/python/setup.cfg
new file mode 100644
index 00000000000..dd71640fc2f
--- /dev/null
+++ b/python/setup.cfg
@@ -0,0 +1,19 @@
+[metadata]
+name = qemu
+maintainer = QEMU Developer Team
+maintainer_email = qemu-devel@nongnu.org
+url = https://www.qemu.org/
+download_url = https://www.qemu.org/download/
+description = QEMU Python Build, Debug and SDK tooling.
+long_description = file:PACKAGE.rst
+long_description_content_type = text/x-rst
+classifiers =
+    Development Status :: 3 - Alpha
+    License :: OSI Approved :: GNU General Public License v2 (GPLv2)
+    Natural Language :: English
+    Operating System :: OS Independent
+    Programming Language :: Python :: 3 :: Only
+
+[options]
+python_requires = >= 3.6
+packages = find_namespace:
diff --git a/python/setup.py b/python/setup.py
new file mode 100755
index 00000000000..e93d0075d16
--- /dev/null
+++ b/python/setup.py
@@ -0,0 +1,23 @@
+#!/usr/bin/env python3
+"""
+QEMU tooling installer script
+Copyright (c) 2020 John Snow for Red Hat, Inc.
+"""
+
+import setuptools
+import pkg_resources
+
+
+def main():
+    """
+    QEMU tooling installer
+    """
+
+    # https://medium.com/@daveshawley/safely-using-setup-cfg-for-metadata-1babbe54c108
+    pkg_resources.require('setuptools>=39.2')
+
+    setuptools.setup()
+
+
+if __name__ == '__main__':
+    main()
-- 
2.29.2

[PATCH v4 06/24] python: add VERSION file

[John Snow]

Python infrastructure as it exists today is not capable reliably of
single-sourcing a package version from a parent directory. The authors
of pip are working to correct this, but as of today this is not possible.

The problem is that when using pip to build and install a python
package, it copies files over to a temporary directory and performs its
build there. This loses access to any information in the parent
directory, including git itself.

Further, Python versions have a standard (PEP 440) that may or may not
follow QEMU's versioning. In general, it does; but naturally QEMU does
not follow PEP 440. To avoid any automatically-generated conflict, a
manual version file is preferred.


I am proposing:

- Python tooling follows the QEMU version, indirectly, but with a major
  version of 0 to indicate that the API is not expected to be
  stable. This would mean version 0.5.2.0, 0.5.1.1, 0.5.3.0, etc.

- In the event that a Python package needs to be updated independently
  of the QEMU version, a pre-release alpha version should be preferred,
  but *only* after inclusion to the qemu development or stable branches.

  e.g. 0.5.2.0a1, 0.5.2.0a2, and so on should be preferred prior to
  5.2.0's release.

- The Python core tooling makes absolutely no version compatibility
  checks or constraints. It *may* work with releases of QEMU from the
  past or future, but it is not required to.

  i.e., "qemu.machine" will, for now, remain in lock-step with QEMU.

- We reserve the right to split the qemu package into independently
  versioned subpackages at a later date. This might allow for us to
  begin versioning QMP independently from QEMU at a later date, if
  we so choose.


Implement this versioning scheme by adding a VERSION file and setting it
to 0.6.0.0a1.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/VERSION   | 1 +
 python/setup.cfg | 1 +
 2 files changed, 2 insertions(+)
 create mode 100644 python/VERSION

diff --git a/python/VERSION b/python/VERSION
new file mode 100644
index 00000000000..ffc91e96185
--- /dev/null
+++ b/python/VERSION
@@ -0,0 +1 @@
+0.6.0.0a1
diff --git a/python/setup.cfg b/python/setup.cfg
index dd71640fc2f..e7f8ab23815 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -1,5 +1,6 @@
 [metadata]
 name = qemu
+version = file:VERSION
 maintainer = QEMU Developer Team
 maintainer_email = qemu-devel@nongnu.org
 url = https://www.qemu.org/
-- 
2.29.2

[PATCH v4 07/24] python: add directory structure README.rst files

[John Snow]

Add short readmes to python/, python/qemu/, python/qemu/machine,
python/qemu/qmp, and python/qemu/utils that explain the directory
hierarchy. These readmes are visible when browsing the source on
e.g. gitlab/github and are designed to help new developers/users quickly
make sense of the source tree.

They are not designed for inclusion in a published manual.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/README.rst              | 41 ++++++++++++++++++++++++++++++++++
 python/qemu/README.rst         |  8 +++++++
 python/qemu/machine/README.rst |  9 ++++++++
 python/qemu/qmp/README.rst     |  9 ++++++++
 python/qemu/utils/README.rst   |  9 ++++++++
 5 files changed, 76 insertions(+)
 create mode 100644 python/README.rst
 create mode 100644 python/qemu/README.rst
 create mode 100644 python/qemu/machine/README.rst
 create mode 100644 python/qemu/qmp/README.rst
 create mode 100644 python/qemu/utils/README.rst

diff --git a/python/README.rst b/python/README.rst
new file mode 100644
index 00000000000..6a14b99e104
--- /dev/null
+++ b/python/README.rst
@@ -0,0 +1,41 @@
+QEMU Python Tooling
+===================
+
+This directory houses Python tooling used by the QEMU project to build,
+configure, and test QEMU. It is organized by namespace (``qemu``), and
+then by package (``qemu/machine``, ``qemu/qmp``).
+
+``setup.py`` is used by ``pip`` to install this tooling to the current
+environment. ``setup.cfg`` provides the packaging configuration used by
+setup.py in a setuptools specific format. You will generally invoke it
+by doing one of the following:
+
+1. ``pip3 install .`` will install these packages to your current
+   environment. If you are inside a virtual environment, they will
+   install there. If you are not, it will attempt to install to the
+   global environment, which is not recommended.
+
+2. ``pip3 install --user .`` will install these packages to your user's
+   local python packages. If you are inside of a virtual environment,
+   this will fail.
+
+If you amend the ``-e`` argument, pip will install in "editable" mode;
+which installs a version of the package that installs a forwarder
+pointing to these files, such that the package always reflects the
+latest version in your git tree.
+
+See `Installing packages using pip and virtual environments
+<https://packaging.python.org/guides/installing-using-pip-and-virtual-environments/>`_
+for more information.
+
+
+Files in this directory
+-----------------------
+
+- ``qemu/`` Python package source directory.
+- ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
+- ``README.rst`` you are here!
+- ``VERSION`` contains the PEP-440 compliant version used to describe
+  this package; it is referenced by ``setup.cfg``.
+- ``setup.cfg`` houses setuptools package configuration.
+- ``setup.py`` is the setuptools installer used by pip; See above.
diff --git a/python/qemu/README.rst b/python/qemu/README.rst
new file mode 100644
index 00000000000..d04943f526c
--- /dev/null
+++ b/python/qemu/README.rst
@@ -0,0 +1,8 @@
+QEMU Python Namespace
+=====================
+
+This directory serves as the root of a `Python PEP 420 implicit
+namespace package <https://www.python.org/dev/peps/pep-0420/>`_.
+
+Each directory below is assumed to be an installable Python package that
+is available under the ``qemu.<package>`` namespace.
diff --git a/python/qemu/machine/README.rst b/python/qemu/machine/README.rst
new file mode 100644
index 00000000000..ac2b4fffb42
--- /dev/null
+++ b/python/qemu/machine/README.rst
@@ -0,0 +1,9 @@
+qemu.machine package
+====================
+
+This package provides core utilities used for testing and debugging
+QEMU. It is used by the iotests, vm tests, acceptance tests, and several
+other utilities in the ./scripts directory. It is not a fully-fledged
+SDK and it is subject to change at any time.
+
+See the documentation in ``__init__.py`` for more information.
diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
new file mode 100644
index 00000000000..c21951491cf
--- /dev/null
+++ b/python/qemu/qmp/README.rst
@@ -0,0 +1,9 @@
+qemu.qmp package
+================
+
+This package provides a library used for connecting to and communicating
+with QMP servers. It is used extensively by iotests, vm tests,
+acceptance tests, and other utilities in the ./scripts directory. It is
+not a fully-fledged SDK and is subject to change at any time.
+
+See the documentation in ``__init__.py`` for more information.
diff --git a/python/qemu/utils/README.rst b/python/qemu/utils/README.rst
new file mode 100644
index 00000000000..4b33c1f27e1
--- /dev/null
+++ b/python/qemu/utils/README.rst
@@ -0,0 +1,9 @@
+qemu.utils package
+==================
+
+This package provides misc utilities used for testing and debugging
+QEMU. It is used most directly by the qemu.machine package, but has some
+uses by the vm and acceptance tests for determining accelerator support.
+
+See the documentation in ``__init__.py`` and ``accel.py`` for more
+information.
-- 
2.29.2

[PATCH v4 08/24] python: Add pipenv support

[John Snow]

pipenv is a tool used for managing virtual environments with pinned,
explicit dependencies. It is used for precisely recreating python
virtual environments.

pipenv uses two files to do this:

(1) Pipfile, which is similar in purpose and scope to what setup.py
lists. It specifies the requisite minimum to get a functional
environment for using this package.

(2) Pipfile.lock, which is similar in purpose to `pip freeze >
requirements.txt`. It specifies a canonical virtual environment used for
deployment or testing. This ensures that all users have repeatable
results.

The primary benefit of using this tool is to ensure repeatable CI
results with a known set of packages. Although I endeavor to support as
many versions as I can, the fluid nature of the Python toolchain often
means tailoring code for fairly specific versions.

Note that pipenv is *not* required to install or use this module; this is
purely for the sake of repeatable testing by CI or developers.

Here, a "blank" pipfile is added with no dependencies, but specifies
Python 3.6 for the virtual environment.

Pipfile will specify our version minimums, while Pipfile.lock specifies
an exact loudout of packages that were known to operate correctly. This
latter file provides the real value for easy setup of container images
and CI environments.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 python/Pipfile

diff --git a/python/Pipfile b/python/Pipfile
new file mode 100644
index 00000000000..9534830b5eb
--- /dev/null
+++ b/python/Pipfile
@@ -0,0 +1,11 @@
+[[source]]
+name = "pypi"
+url = "https://pypi.org/simple"
+verify_ssl = true
+
+[dev-packages]
+
+[packages]
+
+[requires]
+python_version = "3.6"
-- 
2.29.2

[PATCH v4 09/24] python: add pylint import exceptions

[John Snow]

Pylint 2.5.x and 2.6.x have regressions that make import checking
inconsistent, see:

https://github.com/PyCQA/pylint/issues/3609
https://github.com/PyCQA/pylint/issues/3624
https://github.com/PyCQA/pylint/issues/3651

Pinning to 2.4.4 is worse, because it mandates versions of shared
dependencies that are too old for features we want in isort and mypy.
Oh well.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/__init__.py | 3 +++
 python/qemu/machine/machine.py  | 2 +-
 python/qemu/machine/qtest.py    | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
index 891a8f784b5..6881f424c63 100644
--- a/python/qemu/machine/__init__.py
+++ b/python/qemu/machine/__init__.py
@@ -22,6 +22,9 @@
 # the COPYING file in the top-level directory.
 #
 
+# pylint: disable=import-error
+# see: https://github.com/PyCQA/pylint/issues/3624
+# see: https://github.com/PyCQA/pylint/issues/3651
 from .machine import QEMUMachine
 from .qtest import QEMUQtestMachine, QEMUQtestProtocol
 
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index dd6ce289350..4c1fde6101a 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -38,7 +38,7 @@
     Type,
 )
 
-from qemu.qmp import (
+from qemu.qmp import (  # pylint: disable=import-error
     QEMUMonitorProtocol,
     QMPMessage,
     QMPReturnValue,
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index 53926e434a7..c3adf4e3012 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
     TextIO,
 )
 
-from qemu.qmp import SocketAddrT
+from qemu.qmp import SocketAddrT  # pylint: disable=import-error
 
 from .machine import QEMUMachine
 
-- 
2.29.2

[PATCH v4 10/24] python: move pylintrc into setup.cfg

[John Snow]

Delete the empty settings now that it's sharing a home with settings for
other tools.

pylint can now be run from this folder as "pylint qemu".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/pylintrc | 58 ------------------------------------
 python/setup.cfg             | 29 ++++++++++++++++++
 2 files changed, 29 insertions(+), 58 deletions(-)
 delete mode 100644 python/qemu/machine/pylintrc

diff --git a/python/qemu/machine/pylintrc b/python/qemu/machine/pylintrc
deleted file mode 100644
index 3f69205000d..00000000000
--- a/python/qemu/machine/pylintrc
+++ /dev/null
@@ -1,58 +0,0 @@
-[MASTER]
-
-[MESSAGES CONTROL]
-
-# Disable the message, report, category or checker with the given id(s). You
-# can either give multiple identifiers separated by comma (,) or put this
-# option multiple times (only on the command line, not in the configuration
-# file where it should appear only once). You can also use "--disable=all" to
-# disable everything first and then reenable specific checks. For example, if
-# you want to run only the similarities checker, you can use "--disable=all
-# --enable=similarities". If you want to run only the classes checker, but have
-# no Warning level messages displayed, use "--disable=all --enable=classes
-# --disable=W".
-disable=too-many-arguments,
-        too-many-instance-attributes,
-        too-many-public-methods,
-
-[REPORTS]
-
-[REFACTORING]
-
-[MISCELLANEOUS]
-
-[LOGGING]
-
-[BASIC]
-
-# Good variable names which should always be accepted, separated by a comma.
-good-names=i,
-           j,
-           k,
-           ex,
-           Run,
-           _,
-           fd,
-           c,
-[VARIABLES]
-
-[STRING]
-
-[SPELLING]
-
-[FORMAT]
-
-[SIMILARITIES]
-
-# Ignore imports when computing similarities.
-ignore-imports=yes
-
-[TYPECHECK]
-
-[CLASSES]
-
-[IMPORTS]
-
-[DESIGN]
-
-[EXCEPTIONS]
diff --git a/python/setup.cfg b/python/setup.cfg
index e7f8ab23815..20b24372a4a 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -18,3 +18,32 @@ classifiers =
 [options]
 python_requires = >= 3.6
 packages = find_namespace:
+
+[pylint.messages control]
+# Disable the message, report, category or checker with the given id(s). You
+# can either give multiple identifiers separated by comma (,) or put this
+# option multiple times (only on the command line, not in the configuration
+# file where it should appear only once). You can also use "--disable=all" to
+# disable everything first and then reenable specific checks. For example, if
+# you want to run only the similarities checker, you can use "--disable=all
+# --enable=similarities". If you want to run only the classes checker, but have
+# no Warning level messages displayed, use "--disable=all --enable=classes
+# --disable=W".
+disable=too-many-arguments,
+        too-many-instance-attributes,
+        too-many-public-methods,
+
+[pylint.basic]
+# Good variable names which should always be accepted, separated by a comma.
+good-names=i,
+           j,
+           k,
+           ex,
+           Run,
+           _,
+           fd,
+           c,
+
+[pylint.similarities]
+# Ignore imports when computing similarities.
+ignore-imports=yes
-- 
2.29.2

[PATCH v4 11/24] python: add pylint to pipenv

[John Snow]

We are specifying >= pylint 2.6.x for two reasons:

1. For setup.cfg support, added in pylint 2.5.x
2. To clarify that we are using a version that has incompatibly dropped
bad-whitespace checks.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile      |   1 +
 python/Pipfile.lock | 137 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 138 insertions(+)
 create mode 100644 python/Pipfile.lock

diff --git a/python/Pipfile b/python/Pipfile
index 9534830b5eb..1e58986c895 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,6 +4,7 @@ url = "https://pypi.org/simple"
 verify_ssl = true
 
 [dev-packages]
+pylint = ">=2.6.0"
 
 [packages]
 
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
new file mode 100644
index 00000000000..4506335b7d9
--- /dev/null
+++ b/python/Pipfile.lock
@@ -0,0 +1,137 @@
+{
+    "_meta": {
+        "hash": {
+            "sha256": "b7ac1f2ad73bc166244c0378298afba64951a16bb749b81a9668dc41f40f941c"
+        },
+        "pipfile-spec": 6,
+        "requires": {
+            "python_version": "3.6"
+        },
+        "sources": [
+            {
+                "name": "pypi",
+                "url": "https://pypi.org/simple",
+                "verify_ssl": true
+            }
+        ]
+    },
+    "default": {},
+    "develop": {
+        "astroid": {
+            "hashes": [
+                "sha256:2f4078c2a41bf377eea06d71c9d2ba4eb8f6b1af2135bec27bbbb7d8f12bb703",
+                "sha256:bc58d83eb610252fd8de6363e39d4f1d0619c894b0ed24603b881c02e64c7386"
+            ],
+            "markers": "python_version >= '3.5'",
+            "version": "==2.4.2"
+        },
+        "isort": {
+            "hashes": [
+                "sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
+                "sha256:fff4f0c04e1825522ce6949973e83110a6e907750cd92d128b0d14aaaadbffdc"
+            ],
+            "markers": "python_version >= '3.6' and python_version < '4.0'",
+            "version": "==5.7.0"
+        },
+        "lazy-object-proxy": {
+            "hashes": [
+                "sha256:0c4b206227a8097f05c4dbdd323c50edf81f15db3b8dc064d08c62d37e1a504d",
+                "sha256:194d092e6f246b906e8f70884e620e459fc54db3259e60cf69a4d66c3fda3449",
+                "sha256:1be7e4c9f96948003609aa6c974ae59830a6baecc5376c25c92d7d697e684c08",
+                "sha256:4677f594e474c91da97f489fea5b7daa17b5517190899cf213697e48d3902f5a",
+                "sha256:48dab84ebd4831077b150572aec802f303117c8cc5c871e182447281ebf3ac50",
+                "sha256:5541cada25cd173702dbd99f8e22434105456314462326f06dba3e180f203dfd",
+                "sha256:59f79fef100b09564bc2df42ea2d8d21a64fdcda64979c0fa3db7bdaabaf6239",
+                "sha256:8d859b89baf8ef7f8bc6b00aa20316483d67f0b1cbf422f5b4dc56701c8f2ffb",
+                "sha256:9254f4358b9b541e3441b007a0ea0764b9d056afdeafc1a5569eee1cc6c1b9ea",
+                "sha256:9651375199045a358eb6741df3e02a651e0330be090b3bc79f6d0de31a80ec3e",
+                "sha256:97bb5884f6f1cdce0099f86b907aa41c970c3c672ac8b9c8352789e103cf3156",
+                "sha256:9b15f3f4c0f35727d3a0fba4b770b3c4ebbb1fa907dbcc046a1d2799f3edd142",
+                "sha256:a2238e9d1bb71a56cd710611a1614d1194dc10a175c1e08d75e1a7bcc250d442",
+                "sha256:a6ae12d08c0bf9909ce12385803a543bfe99b95fe01e752536a60af2b7797c62",
+                "sha256:ca0a928a3ddbc5725be2dd1cf895ec0a254798915fb3a36af0964a0a4149e3db",
+                "sha256:cb2c7c57005a6804ab66f106ceb8482da55f5314b7fcb06551db1edae4ad1531",
+                "sha256:d74bb8693bf9cf75ac3b47a54d716bbb1a92648d5f781fc799347cfc95952383",
+                "sha256:d945239a5639b3ff35b70a88c5f2f491913eb94871780ebfabb2568bd58afc5a",
+                "sha256:eba7011090323c1dadf18b3b689845fd96a61ba0a1dfbd7f24b921398affc357",
+                "sha256:efa1909120ce98bbb3777e8b6f92237f5d5c8ea6758efea36a473e1d38f7d3e4",
+                "sha256:f3900e8a5de27447acbf900b4750b0ddfd7ec1ea7fbaf11dfa911141bc522af0"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==1.4.3"
+        },
+        "mccabe": {
+            "hashes": [
+                "sha256:ab8a6258860da4b6677da4bd2fe5dc2c659cff31b3ee4f7f5d64e79735b80d42",
+                "sha256:dd8d182285a0fe56bace7f45b5e7d1a6ebcbf524e8f3bd87eb0f125271b8831f"
+            ],
+            "version": "==0.6.1"
+        },
+        "pylint": {
+            "hashes": [
+                "sha256:bb4a908c9dadbc3aac18860550e870f58e1a02c9f2c204fdf5693d73be061210",
+                "sha256:bfe68f020f8a0fece830a22dd4d5dddb4ecc6137db04face4c3420a46a52239f"
+            ],
+            "index": "pypi",
+            "version": "==2.6.0"
+        },
+        "six": {
+            "hashes": [
+                "sha256:30639c035cdb23534cd4aa2dd52c3bf48f06e5f4a941509c8bafd8ce11080259",
+                "sha256:8b74bedcbbbaca38ff6d7491d76f2b06b3592611af620f8426e82dddb04a5ced"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'",
+            "version": "==1.15.0"
+        },
+        "toml": {
+            "hashes": [
+                "sha256:806143ae5bfb6a3c6e736a764057db0e6a0e05e338b5630894a5f779cabb4f9b",
+                "sha256:b3bda1d108d5dd99f4a20d24d9c348e91c4db7ab1b749200bded2f839ccbe68f"
+            ],
+            "markers": "python_version >= '2.6' and python_version not in '3.0, 3.1, 3.2'",
+            "version": "==0.10.2"
+        },
+        "typed-ast": {
+            "hashes": [
+                "sha256:07d49388d5bf7e863f7fa2f124b1b1d89d8aa0e2f7812faff0a5658c01c59aa1",
+                "sha256:14bf1522cdee369e8f5581238edac09150c765ec1cb33615855889cf33dcb92d",
+                "sha256:240296b27397e4e37874abb1df2a608a92df85cf3e2a04d0d4d61055c8305ba6",
+                "sha256:36d829b31ab67d6fcb30e185ec996e1f72b892255a745d3a82138c97d21ed1cd",
+                "sha256:37f48d46d733d57cc70fd5f30572d11ab8ed92da6e6b28e024e4a3edfb456e37",
+                "sha256:4c790331247081ea7c632a76d5b2a265e6d325ecd3179d06e9cf8d46d90dd151",
+                "sha256:5dcfc2e264bd8a1db8b11a892bd1647154ce03eeba94b461effe68790d8b8e07",
+                "sha256:7147e2a76c75f0f64c4319886e7639e490fee87c9d25cb1d4faef1d8cf83a440",
+                "sha256:7703620125e4fb79b64aa52427ec192822e9f45d37d4b6625ab37ef403e1df70",
+                "sha256:8368f83e93c7156ccd40e49a783a6a6850ca25b556c0fa0240ed0f659d2fe496",
+                "sha256:84aa6223d71012c68d577c83f4e7db50d11d6b1399a9c779046d75e24bed74ea",
+                "sha256:85f95aa97a35bdb2f2f7d10ec5bbdac0aeb9dafdaf88e17492da0504de2e6400",
+                "sha256:8db0e856712f79c45956da0c9a40ca4246abc3485ae0d7ecc86a20f5e4c09abc",
+                "sha256:9044ef2df88d7f33692ae3f18d3be63dec69c4fb1b5a4a9ac950f9b4ba571606",
+                "sha256:963c80b583b0661918718b095e02303d8078950b26cc00b5e5ea9ababe0de1fc",
+                "sha256:987f15737aba2ab5f3928c617ccf1ce412e2e321c77ab16ca5a293e7bbffd581",
+                "sha256:9ec45db0c766f196ae629e509f059ff05fc3148f9ffd28f3cfe75d4afb485412",
+                "sha256:9fc0b3cb5d1720e7141d103cf4819aea239f7d136acf9ee4a69b047b7986175a",
+                "sha256:a2c927c49f2029291fbabd673d51a2180038f8cd5a5b2f290f78c4516be48be2",
+                "sha256:a38878a223bdd37c9709d07cd357bb79f4c760b29210e14ad0fb395294583787",
+                "sha256:b4fcdcfa302538f70929eb7b392f536a237cbe2ed9cba88e3bf5027b39f5f77f",
+                "sha256:c0c74e5579af4b977c8b932f40a5464764b2f86681327410aa028a22d2f54937",
+                "sha256:c1c876fd795b36126f773db9cbb393f19808edd2637e00fd6caba0e25f2c7b64",
+                "sha256:c9aadc4924d4b5799112837b226160428524a9a45f830e0d0f184b19e4090487",
+                "sha256:cc7b98bf58167b7f2db91a4327da24fb93368838eb84a44c472283778fc2446b",
+                "sha256:cf54cfa843f297991b7388c281cb3855d911137223c6b6d2dd82a47ae5125a41",
+                "sha256:d003156bb6a59cda9050e983441b7fa2487f7800d76bdc065566b7d728b4581a",
+                "sha256:d175297e9533d8d37437abc14e8a83cbc68af93cc9c1c59c2c292ec59a0697a3",
+                "sha256:d746a437cdbca200622385305aedd9aef68e8a645e385cc483bdc5e488f07166",
+                "sha256:e683e409e5c45d5c9082dc1daf13f6374300806240719f95dc783d1fc942af10"
+            ],
+            "markers": "implementation_name == 'cpython' and python_version < '3.8'",
+            "version": "==1.4.2"
+        },
+        "wrapt": {
+            "hashes": [
+                "sha256:b62ffa81fb85f4332a4f609cab4ac40709470da05643a082ec1eb88e6d9b97d7"
+            ],
+            "version": "==1.12.1"
+        }
+    }
+}
-- 
2.29.2

[PATCH v4 12/24] python: move flake8 config to setup.cfg

[John Snow]

Update the comment concerning the flake8 exception to match commit
42c0dd12, whose commit message stated:

A note on the flake8 exception: flake8 will warn on *any* bare except,
but pylint's is context-aware and will suppress the warning if you
re-raise the exception.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/.flake8 | 2 --
 python/setup.cfg            | 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)
 delete mode 100644 python/qemu/machine/.flake8

diff --git a/python/qemu/machine/.flake8 b/python/qemu/machine/.flake8
deleted file mode 100644
index 45d8146f3f5..00000000000
--- a/python/qemu/machine/.flake8
+++ /dev/null
@@ -1,2 +0,0 @@
-[flake8]
-extend-ignore = E722  # Pylint handles this, but smarter.
\ No newline at end of file
diff --git a/python/setup.cfg b/python/setup.cfg
index 20b24372a4a..9ecb2902006 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -19,6 +19,9 @@ classifiers =
 python_requires = >= 3.6
 packages = find_namespace:
 
+[flake8]
+extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
+
 [pylint.messages control]
 # Disable the message, report, category or checker with the given id(s). You
 # can either give multiple identifiers separated by comma (,) or put this
-- 
2.29.2

[PATCH v4 13/24] python: Add flake8 to pipenv

[John Snow]

flake8 3.5.x does not support the --extend-ignore syntax used in the
.flake8 file to gracefully extend default ignores, so 3.6.x is our
minimum requirement. There is no known upper bound.

flake8 can be run from the python/ directory with no arguments.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile      |  1 +
 python/Pipfile.lock | 51 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/python/Pipfile b/python/Pipfile
index 1e58986c895..d1f7045f680 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,6 +4,7 @@ url = "https://pypi.org/simple"
 verify_ssl = true
 
 [dev-packages]
+flake8 = ">=3.6.0"
 pylint = ">=2.6.0"
 
 [packages]
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 4506335b7d9..869b0bdf67f 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
     "_meta": {
         "hash": {
-            "sha256": "b7ac1f2ad73bc166244c0378298afba64951a16bb749b81a9668dc41f40f941c"
+            "sha256": "9f6d4857a6c72ad40fc1ec1e58cdb76f187a2986ac4156f0027e5eb798ec69a9"
         },
         "pipfile-spec": 6,
         "requires": {
@@ -25,6 +25,22 @@
             "markers": "python_version >= '3.5'",
             "version": "==2.4.2"
         },
+        "flake8": {
+            "hashes": [
+                "sha256:749dbbd6bfd0cf1318af27bf97a14e28e5ff548ef8e5b1566ccfb25a11e7c839",
+                "sha256:aadae8761ec651813c24be05c6f7b4680857ef6afaae4651a4eccaef97ce6c3b"
+            ],
+            "index": "pypi",
+            "version": "==3.8.4"
+        },
+        "importlib-metadata": {
+            "hashes": [
+                "sha256:ace61d5fc652dc280e7b6b4ff732a9c2d40db2c0f92bc6cb74e07b73d53a1771",
+                "sha256:fa5daa4477a7414ae34e95942e4dd07f62adf589143c875c133c1e53c4eff38d"
+            ],
+            "markers": "python_version < '3.8'",
+            "version": "==3.4.0"
+        },
         "isort": {
             "hashes": [
                 "sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
@@ -67,6 +83,22 @@
             ],
             "version": "==0.6.1"
         },
+        "pycodestyle": {
+            "hashes": [
+                "sha256:2295e7b2f6b5bd100585ebcb1f616591b652db8a741695b3d8f5d28bdc934367",
+                "sha256:c58a7d2815e0e8d7972bf1803331fb0152f867bd89adf8a01dfd55085434192e"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==2.6.0"
+        },
+        "pyflakes": {
+            "hashes": [
+                "sha256:0d94e0e05a19e57a99444b6ddcf9a6eb2e5c68d3ca1e98e90707af8152c90a92",
+                "sha256:35b2d75ee967ea93b55750aa9edbbf72813e06a66ba54438df2cfac9e3c27fc8"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==2.2.0"
+        },
         "pylint": {
             "hashes": [
                 "sha256:bb4a908c9dadbc3aac18860550e870f58e1a02c9f2c204fdf5693d73be061210",
@@ -127,11 +159,28 @@
             "markers": "implementation_name == 'cpython' and python_version < '3.8'",
             "version": "==1.4.2"
         },
+        "typing-extensions": {
+            "hashes": [
+                "sha256:7cb407020f00f7bfc3cb3e7881628838e69d8f3fcab2f64742a5e76b2f841918",
+                "sha256:99d4073b617d30288f569d3f13d2bd7548c3a7e4c8de87db09a9d29bb3a4a60c",
+                "sha256:dafc7639cde7f1b6e1acc0f457842a83e722ccca8eef5270af2d74792619a89f"
+            ],
+            "markers": "python_version < '3.8'",
+            "version": "==3.7.4.3"
+        },
         "wrapt": {
             "hashes": [
                 "sha256:b62ffa81fb85f4332a4f609cab4ac40709470da05643a082ec1eb88e6d9b97d7"
             ],
             "version": "==1.12.1"
+        },
+        "zipp": {
+            "hashes": [
+                "sha256:102c24ef8f171fd729d46599845e95c7ab894a4cf45f5de11a44cc7444fb1108",
+                "sha256:ed5eee1974372595f9e416cc7bbeeb12335201d8081ca8a0743c954d4446e5cb"
+            ],
+            "markers": "python_version >= '3.6'",
+            "version": "==3.4.0"
         }
     }
 }
-- 
2.29.2

[PATCH v4 14/24] python: move mypy.ini into setup.cfg

[John Snow]

mypy supports reading its configuration values from a central project
configuration file; do so.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/mypy.ini  | 4 ----
 python/setup.cfg | 5 +++++
 2 files changed, 5 insertions(+), 4 deletions(-)
 delete mode 100644 python/mypy.ini

diff --git a/python/mypy.ini b/python/mypy.ini
deleted file mode 100644
index 1a581c5f1ea..00000000000
--- a/python/mypy.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[mypy]
-strict = True
-python_version = 3.6
-warn_unused_configs = True
diff --git a/python/setup.cfg b/python/setup.cfg
index 9ecb2902006..0bd55c5e373 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -22,6 +22,11 @@ packages = find_namespace:
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
 
+[mypy]
+strict = True
+python_version = 3.6
+warn_unused_configs = True
+
 [pylint.messages control]
 # Disable the message, report, category or checker with the given id(s). You
 # can either give multiple identifiers separated by comma (,) or put this
-- 
2.29.2

[PATCH v4 15/24] python: add mypy to pipenv

[John Snow]

0.730 appears to be about the oldest version that works with the
features we want, including nice human readable output (to make sure
iotest 297 passes), and type-parameterized Popen generics.

0.770, however, supports adding 'strict' to the config file, so require
at least 0.770.

Now that we are checking a namespace package, we need to tell mypy to
allow PEP420 namespaces, so modify the mypy config as part of the move.

mypy can now be run from the python root by typing 'mypy qemu'.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile      |  1 +
 python/Pipfile.lock | 37 ++++++++++++++++++++++++++++++++++++-
 python/setup.cfg    |  1 +
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/python/Pipfile b/python/Pipfile
index d1f7045f680..51c537b0d10 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -5,6 +5,7 @@ verify_ssl = true
 
 [dev-packages]
 flake8 = ">=3.6.0"
+mypy = ">=0.770"
 pylint = ">=2.6.0"
 
 [packages]
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 869b0bdf67f..5f8aab117c3 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
     "_meta": {
         "hash": {
-            "sha256": "9f6d4857a6c72ad40fc1ec1e58cdb76f187a2986ac4156f0027e5eb798ec69a9"
+            "sha256": "65f010a8f2e55e9870d2b7e0d8af129516097d23abf2504f396552748b067ade"
         },
         "pipfile-spec": 6,
         "requires": {
@@ -83,6 +83,41 @@
             ],
             "version": "==0.6.1"
         },
+        "mypy": {
+            "hashes": [
+                "sha256:0d2fc8beb99cd88f2d7e20d69131353053fbecea17904ee6f0348759302c52fa",
+                "sha256:2b216eacca0ec0ee124af9429bfd858d5619a0725ee5f88057e6e076f9eb1a7b",
+                "sha256:319ee5c248a7c3f94477f92a729b7ab06bf8a6d04447ef3aa8c9ba2aa47c6dcf",
+                "sha256:3e0c159a7853e3521e3f582adb1f3eac66d0b0639d434278e2867af3a8c62653",
+                "sha256:5615785d3e2f4f03ab7697983d82c4b98af5c321614f51b8f1034eb9ebe48363",
+                "sha256:5ff616787122774f510caeb7b980542a7cc2222be3f00837a304ea85cd56e488",
+                "sha256:6f8425fecd2ba6007e526209bb985ce7f49ed0d2ac1cc1a44f243380a06a84fb",
+                "sha256:74f5aa50d0866bc6fb8e213441c41e466c86678c800700b87b012ed11c0a13e0",
+                "sha256:90b6f46dc2181d74f80617deca611925d7e63007cf416397358aa42efb593e07",
+                "sha256:947126195bfe4709c360e89b40114c6746ae248f04d379dca6f6ab677aa07641",
+                "sha256:a301da58d566aca05f8f449403c710c50a9860782148332322decf73a603280b",
+                "sha256:aa9d4901f3ee1a986a3a79fe079ffbf7f999478c281376f48faa31daaa814e86",
+                "sha256:b9150db14a48a8fa114189bfe49baccdff89da8c6639c2717750c7ae62316738",
+                "sha256:b95068a3ce3b50332c40e31a955653be245666a4bc7819d3c8898aa9fb9ea496",
+                "sha256:ca7ad5aed210841f1e77f5f2f7d725b62c78fa77519312042c719ed2ab937876",
+                "sha256:d16c54b0dffb861dc6318a8730952265876d90c5101085a4bc56913e8521ba19",
+                "sha256:e0202e37756ed09daf4b0ba64ad2c245d357659e014c3f51d8cd0681ba66940a",
+                "sha256:e1c84c65ff6d69fb42958ece5b1255394714e0aac4df5ffe151bc4fe19c7600a",
+                "sha256:e32b7b282c4ed4e378bba8b8dfa08e1cfa6f6574067ef22f86bee5b1039de0c9",
+                "sha256:e3b8432f8df19e3c11235c4563a7250666dc9aa7cdda58d21b4177b20256ca9f",
+                "sha256:e497a544391f733eca922fdcb326d19e894789cd4ff61d48b4b195776476c5cf",
+                "sha256:f5fdf935a46aa20aa937f2478480ebf4be9186e98e49cc3843af9a5795a49a25"
+            ],
+            "index": "pypi",
+            "version": "==0.800"
+        },
+        "mypy-extensions": {
+            "hashes": [
+                "sha256:090fedd75945a69ae91ce1303b5824f428daf5a028d2f6ab8a299250a846f15d",
+                "sha256:2d82818f5bb3e369420cb3c4060a7970edba416647068eb4c5343488a6c604a8"
+            ],
+            "version": "==0.4.3"
+        },
         "pycodestyle": {
             "hashes": [
                 "sha256:2295e7b2f6b5bd100585ebcb1f616591b652db8a741695b3d8f5d28bdc934367",
diff --git a/python/setup.cfg b/python/setup.cfg
index 0bd55c5e373..f919e95f95b 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -26,6 +26,7 @@ extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
 strict = True
 python_version = 3.6
 warn_unused_configs = True
+namespace_packages = True
 
 [pylint.messages control]
 # Disable the message, report, category or checker with the given id(s). You
-- 
2.29.2

[PATCH v4 16/24] python: move .isort.cfg into setup.cfg

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/.isort.cfg | 7 -------
 python/setup.cfg  | 8 ++++++++
 2 files changed, 8 insertions(+), 7 deletions(-)
 delete mode 100644 python/.isort.cfg

diff --git a/python/.isort.cfg b/python/.isort.cfg
deleted file mode 100644
index 6d0fd6cc0d3..00000000000
--- a/python/.isort.cfg
+++ /dev/null
@@ -1,7 +0,0 @@
-[settings]
-force_grid_wrap=4
-force_sort_within_sections=True
-include_trailing_comma=True
-line_length=72
-lines_after_imports=2
-multi_line_output=3
\ No newline at end of file
diff --git a/python/setup.cfg b/python/setup.cfg
index f919e95f95b..bc90d52b9ca 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -56,3 +56,11 @@ good-names=i,
 [pylint.similarities]
 # Ignore imports when computing similarities.
 ignore-imports=yes
+
+[isort]
+force_grid_wrap=4
+force_sort_within_sections=True
+include_trailing_comma=True
+line_length=72
+lines_after_imports=2
+multi_line_output=3
-- 
2.29.2

[PATCH v4 17/24] python/qemu: add isort to pipenv

[John Snow]

isort 5.0.0 through 5.0.4 has a bug that causes it to misinterpret
certain "from ..." clauses that are not related to imports.

Require 5.0.5 or greater.

isort can be run with 'isort -c qemu' from the python root.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/Pipfile      | 1 +
 python/Pipfile.lock | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/python/Pipfile b/python/Pipfile
index 51c537b0d10..75b96f29d87 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -5,6 +5,7 @@ verify_ssl = true
 
 [dev-packages]
 flake8 = ">=3.6.0"
+isort = ">=5.0.5"
 mypy = ">=0.770"
 pylint = ">=2.6.0"
 
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 5f8aab117c3..201e735c27a 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
     "_meta": {
         "hash": {
-            "sha256": "65f010a8f2e55e9870d2b7e0d8af129516097d23abf2504f396552748b067ade"
+            "sha256": "b89c7a1b8a414f2a4cd708964123fb427d55419ee0b39e088bf2e7d4fbc11979"
         },
         "pipfile-spec": 6,
         "requires": {
@@ -46,7 +46,7 @@
                 "sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
                 "sha256:fff4f0c04e1825522ce6949973e83110a6e907750cd92d128b0d14aaaadbffdc"
             ],
-            "markers": "python_version >= '3.6' and python_version < '4.0'",
+            "index": "pypi",
             "version": "==5.7.0"
         },
         "lazy-object-proxy": {
-- 
2.29.2

[PATCH v4 18/24] python/qemu: add qemu package itself to pipenv

[John Snow]

This adds the python qemu packages themselves to the pipenv manifest.
'pipenv sync' will create a virtual environment sufficient to use the SDK.
'pipenv sync --dev' will create a virtual environment sufficient to use
and test the SDK (with pylint, mypy, isort, flake8, etc.)

The qemu packages are installed in 'editable' mode; all changes made to
the python package inside the git tree will be reflected in the
installed package without reinstallation. This includes changes made
via git pull and so on.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile      | 1 +
 python/Pipfile.lock | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/python/Pipfile b/python/Pipfile
index 75b96f29d87..214fb175e7a 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -10,6 +10,7 @@ mypy = ">=0.770"
 pylint = ">=2.6.0"
 
 [packages]
+qemu = {editable = true,path = "."}
 
 [requires]
 python_version = "3.6"
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 201e735c27a..4b4402f49e5 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
     "_meta": {
         "hash": {
-            "sha256": "b89c7a1b8a414f2a4cd708964123fb427d55419ee0b39e088bf2e7d4fbc11979"
+            "sha256": "e38d142c3fadc2f2ed849e86f7ebd14e25974dc12228751490aef5a9ee074f2f"
         },
         "pipfile-spec": 6,
         "requires": {
@@ -15,7 +15,12 @@
             }
         ]
     },
-    "default": {},
+    "default": {
+        "qemu": {
+            "editable": true,
+            "path": "."
+        }
+    },
     "develop": {
         "astroid": {
             "hashes": [
-- 
2.29.2

[PATCH v4 19/24] python: add devel package requirements to setuptools

[John Snow]

setuptools doesn't have a formal understanding of development requires,
but it has an optional feataures section. Fine; add a "devel" feature
and add the requirements to it.

To avoid duplication, we can modify pipenv to install qemu[devel]
instead. This enables us to run invocations like "pip install -e
.[devel]" and test the package on bleeding-edge packages beyond those
specified in Pipfile.lock.

Importantly, this also allows us to install the qemu development
packages in a non-networked mode: `pip3 install --no-index -e .[devel]`
will now fail if the proper development dependencies are not already
met. This can be useful for automated build scripts where fetching
network packages may be undesirable.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile      |  5 +----
 python/Pipfile.lock | 14 +++++++++-----
 python/setup.cfg    |  9 +++++++++
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/python/Pipfile b/python/Pipfile
index 214fb175e7a..e7acb8cefa4 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,10 +4,7 @@ url = "https://pypi.org/simple"
 verify_ssl = true
 
 [dev-packages]
-flake8 = ">=3.6.0"
-isort = ">=5.0.5"
-mypy = ">=0.770"
-pylint = ">=2.6.0"
+qemu = {editable = true, extras = ["devel"], path = "."}
 
 [packages]
 qemu = {editable = true,path = "."}
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 4b4402f49e5..85b3124a491 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
     "_meta": {
         "hash": {
-            "sha256": "e38d142c3fadc2f2ed849e86f7ebd14e25974dc12228751490aef5a9ee074f2f"
+            "sha256": "eff562a688ebc6f3ffe67494dbb804b883e2159ad81c4d55d96da9f7aec13e91"
         },
         "pipfile-spec": 6,
         "requires": {
@@ -35,7 +35,7 @@
                 "sha256:749dbbd6bfd0cf1318af27bf97a14e28e5ff548ef8e5b1566ccfb25a11e7c839",
                 "sha256:aadae8761ec651813c24be05c6f7b4680857ef6afaae4651a4eccaef97ce6c3b"
             ],
-            "index": "pypi",
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
             "version": "==3.8.4"
         },
         "importlib-metadata": {
@@ -51,7 +51,7 @@
                 "sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
                 "sha256:fff4f0c04e1825522ce6949973e83110a6e907750cd92d128b0d14aaaadbffdc"
             ],
-            "index": "pypi",
+            "markers": "python_version >= '3.6' and python_version < '4.0'",
             "version": "==5.7.0"
         },
         "lazy-object-proxy": {
@@ -113,7 +113,7 @@
                 "sha256:e497a544391f733eca922fdcb326d19e894789cd4ff61d48b4b195776476c5cf",
                 "sha256:f5fdf935a46aa20aa937f2478480ebf4be9186e98e49cc3843af9a5795a49a25"
             ],
-            "index": "pypi",
+            "markers": "python_version >= '3.5'",
             "version": "==0.800"
         },
         "mypy-extensions": {
@@ -144,9 +144,13 @@
                 "sha256:bb4a908c9dadbc3aac18860550e870f58e1a02c9f2c204fdf5693d73be061210",
                 "sha256:bfe68f020f8a0fece830a22dd4d5dddb4ecc6137db04face4c3420a46a52239f"
             ],
-            "index": "pypi",
+            "markers": "python_version >= '3.5'",
             "version": "==2.6.0"
         },
+        "qemu": {
+            "editable": true,
+            "path": "."
+        },
         "six": {
             "hashes": [
                 "sha256:30639c035cdb23534cd4aa2dd52c3bf48f06e5f4a941509c8bafd8ce11080259",
diff --git a/python/setup.cfg b/python/setup.cfg
index bc90d52b9ca..11c361501e8 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -19,6 +19,15 @@ classifiers =
 python_requires = >= 3.6
 packages = find_namespace:
 
+[options.extras_require]
+# Run `pipenv lock` when changing these requirements.
+devel =
+    flake8 >= 3.6.0
+    isort >= 5.0.5
+    mypy >= 0.770
+    pylint >= 2.6.0
+
+
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
 
-- 
2.29.2

[PATCH v4 20/24] python: add pytest and tests

[John Snow]

Try using pytest to manage our various tests; even though right now
they're only invoking binaries and not really running any python-native
code.

Create tests/, and add test_lint.py which calls out to mypy, flake8,
pylint and isort to enforce the standards in this directory.

Add pytest to the setup.cfg development dependencies; add a pytest
configuration section as well; run it in verbose mode.

Finally, add pytest to the Pipfile environment and lock the new
dependencies.

Provided you have the right development dependencies (mypy, flake8,
isort, pylint, and now pytest) You should be able to run "pytest" from
the python folder to run all of these linters with the correct
arguments.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile.lock       | 57 ++++++++++++++++++++++++++++++++++++++-
 python/setup.cfg          |  5 ++++
 python/tests/test_lint.py | 28 +++++++++++++++++++
 3 files changed, 89 insertions(+), 1 deletion(-)
 create mode 100644 python/tests/test_lint.py

diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 85b3124a491..0fa1a14a9b2 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -30,6 +30,14 @@
             "markers": "python_version >= '3.5'",
             "version": "==2.4.2"
         },
+        "attrs": {
+            "hashes": [
+                "sha256:31b2eced602aa8423c2aea9c76a724617ed67cf9513173fd3a4f03e3a929c7e6",
+                "sha256:832aa3cde19744e49938b91fea06d69ecb9e649c93ba974535d08ad92164f700"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==20.3.0"
+        },
         "flake8": {
             "hashes": [
                 "sha256:749dbbd6bfd0cf1318af27bf97a14e28e5ff548ef8e5b1566ccfb25a11e7c839",
@@ -43,9 +51,16 @@
                 "sha256:ace61d5fc652dc280e7b6b4ff732a9c2d40db2c0f92bc6cb74e07b73d53a1771",
                 "sha256:fa5daa4477a7414ae34e95942e4dd07f62adf589143c875c133c1e53c4eff38d"
             ],
-            "markers": "python_version < '3.8'",
+            "markers": "python_version < '3.8' and python_version < '3.8'",
             "version": "==3.4.0"
         },
+        "iniconfig": {
+            "hashes": [
+                "sha256:011e24c64b7f47f6ebd835bb12a743f2fbe9a26d4cecaa7f53bc4f35ee9da8b3",
+                "sha256:bc3af051d7d14b2ee5ef9969666def0cd1a000e121eaea580d4a313df4b37f32"
+            ],
+            "version": "==1.1.1"
+        },
         "isort": {
             "hashes": [
                 "sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
@@ -123,6 +138,30 @@
             ],
             "version": "==0.4.3"
         },
+        "packaging": {
+            "hashes": [
+                "sha256:5b327ac1320dc863dca72f4514ecc086f31186744b84a230374cc1fd776feae5",
+                "sha256:67714da7f7bc052e064859c05c595155bd1ee9f69f76557e21f051443c20947a"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==20.9"
+        },
+        "pluggy": {
+            "hashes": [
+                "sha256:15b2acde666561e1298d71b523007ed7364de07029219b604cf808bfa1c765b0",
+                "sha256:966c145cd83c96502c3c3868f50408687b38434af77734af1e9ca461a4081d2d"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==0.13.1"
+        },
+        "py": {
+            "hashes": [
+                "sha256:21b81bda15b66ef5e1a777a21c4dcd9c20ad3efd0b3f817e7a809035269e1bd3",
+                "sha256:3b80836aa6d1feeaa108e046da6423ab8f6ceda6468545ae8d02d9d58d18818a"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==1.10.0"
+        },
         "pycodestyle": {
             "hashes": [
                 "sha256:2295e7b2f6b5bd100585ebcb1f616591b652db8a741695b3d8f5d28bdc934367",
@@ -147,6 +186,22 @@
             "markers": "python_version >= '3.5'",
             "version": "==2.6.0"
         },
+        "pyparsing": {
+            "hashes": [
+                "sha256:c203ec8783bf771a155b207279b9bccb8dea02d8f0c9e5f8ead507bc3246ecc1",
+                "sha256:ef9d7589ef3c200abe66653d3f1ab1033c3c419ae9b9bdb1240a85b024efc88b"
+            ],
+            "markers": "python_version >= '2.6' and python_version not in '3.0, 3.1, 3.2'",
+            "version": "==2.4.7"
+        },
+        "pytest": {
+            "hashes": [
+                "sha256:9d1edf9e7d0b84d72ea3dbcdfd22b35fb543a5e8f2a60092dd578936bf63d7f9",
+                "sha256:b574b57423e818210672e07ca1fa90aaf194a4f63f3ab909a2c67ebb22913839"
+            ],
+            "markers": "python_version >= '3.6'",
+            "version": "==6.2.2"
+        },
         "qemu": {
             "editable": true,
             "path": "."
diff --git a/python/setup.cfg b/python/setup.cfg
index 11c361501e8..2c5943277d7 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -26,6 +26,7 @@ devel =
     isort >= 5.0.5
     mypy >= 0.770
     pylint >= 2.6.0
+    pytest >= 3.0.0
 
 
 [flake8]
@@ -73,3 +74,7 @@ include_trailing_comma=True
 line_length=72
 lines_after_imports=2
 multi_line_output=3
+
+[tool:pytest]
+addopts = -v
+console_output_style = count
diff --git a/python/tests/test_lint.py b/python/tests/test_lint.py
new file mode 100644
index 00000000000..38fefa01c66
--- /dev/null
+++ b/python/tests/test_lint.py
@@ -0,0 +1,28 @@
+import subprocess
+
+
+def _external_test(command_line: str) -> None:
+    args = command_line.split(' ')
+    try:
+        subprocess.run(args, check=True)
+    except subprocess.CalledProcessError as err:
+        # Re-contextualize to avoid pytest showing error context inside
+        # the subprocess module itself
+        raise Exception("'{:s}' returned {:d}".format(
+            ' '.join(args), err.returncode)) from None
+
+
+def test_isort() -> None:
+    _external_test('isort -c qemu')
+
+
+def test_flake8() -> None:
+    _external_test('flake8')
+
+
+def test_pylint() -> None:
+    _external_test('pylint qemu')
+
+
+def test_mypy() -> None:
+    _external_test('mypy -p qemu')
-- 
2.29.2

[PATCH v4 21/24] python: add excluded dirs to flake8 config

[John Snow]

Following patches make obvious that we ought to ignore certain
directories to avoid wildly erroneous flake8 output.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/setup.cfg | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/python/setup.cfg b/python/setup.cfg
index 2c5943277d7..2c12d9ab89b 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -31,6 +31,8 @@ devel =
 
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
+exclude = __pycache__,
+          .venv,
 
 [mypy]
 strict = True
-- 
2.29.2

[PATCH v4 22/24] python: add Makefile for some common tasks

[John Snow]

Add "make venv" to create the pipenv-managed virtual environment that
contains our explicitly pinned dependencies.

Add "make check" to run the python linters [in the host execution
environment].

Add "make venv-check" which combines the above two: create/update the
venv, then run the linters in that explicitly managed environment.

make clean: delete miscellaneous build output possibly created by
pipenv, pip, or other python packaging utilities

make distclean: delete the above, and the .venv, too.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Makefile | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 python/Makefile

diff --git a/python/Makefile b/python/Makefile
new file mode 100644
index 00000000000..51915eaec3b
--- /dev/null
+++ b/python/Makefile
@@ -0,0 +1,35 @@
+.PHONY: help venv venv-check check clean distclean
+
+help:
+	@echo "python packaging help:"
+	@echo ""
+	@echo "make venv:       Create pipenv's virtual environment."
+	@echo "    NOTE: Requires Python 3.6 and pipenv."
+	@echo "          Will download packages from PyPI."
+	@echo "    HINT: On Fedora: 'sudo dnf install python36 pipenv'"
+	@echo ""
+	@echo "make venv-check: run linters using pipenv's virtual environment."
+	@echo ""
+	@echo "make check:      run linters using the current environment."
+	@echo "    Hint: Install deps with: 'pip install \".[devel]\"'"
+	@echo ""
+	@echo "make clean:      remove build output."
+	@echo ""
+	@echo "make distclean:  remove venv files and everything from 'clean'."
+
+
+venv: .venv
+.venv: Pipfile.lock
+	@PIPENV_VENV_IN_PROJECT=1 pipenv sync --dev --keep-outdated
+
+venv-check: venv
+	@pipenv run make check
+
+check:
+	@pytest
+
+clean:
+	rm -rf build/ dist/
+
+distclean: clean
+	rm -rf qemu.egg.info/ .venv
-- 
2.29.2

[PATCH v4 23/24] python: add .gitignore

[John Snow]

Ignore *Python* build and package output (build, dist, qemu.egg-info);
these files are not created as part of a QEMU build.

Ignore miscellaneous cached python confetti (__pycache__, *.pyc,
.mypy_cache).

Ignore .idea (pycharm) and .venv (pipenv et al).

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/.gitignore | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644 python/.gitignore

diff --git a/python/.gitignore b/python/.gitignore
new file mode 100644
index 00000000000..78c522768bc
--- /dev/null
+++ b/python/.gitignore
@@ -0,0 +1,9 @@
+*.pyc
+.idea/
+.mypy_cache/
+.pytest_cache/
+.venv/
+__pycache__/
+build/
+dist/
+qemu.egg-info/
-- 
2.29.2

[PATCH v4 24/24] gitlab: add python linters to CI

[John Snow]

Add python3.6 to the fedora container image: we need it to run the
linters against that explicit version to make sure we don't break our
minimum version promise.

Add pipenv so that we can fetch precise versions of pip packages we need
to guarantee test reproducability.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 .gitlab-ci.yml                         | 10 ++++++++++
 tests/docker/dockerfiles/fedora.docker |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 28a83afb914..d1ae3972956 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -633,6 +633,16 @@ check-patch:
     GIT_DEPTH: 1000
   allow_failure: true
 
+
+check-python:
+  stage: build
+  image: $CI_REGISTRY_IMAGE/qemu/fedora:latest
+  script:
+    - cd python
+    - make venv-check
+  variables:
+    GIT_DEPTH: 1000
+
 check-dco:
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/centos8:latest
diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker
index 0d7602abbeb..1262b8c5e03 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -84,6 +84,7 @@ ENV PACKAGES \
     numactl-devel \
     perl \
     perl-Test-Harness \
+    pipenv \
     pixman-devel \
     python3 \
     python3-PyYAML \
@@ -93,6 +94,7 @@ ENV PACKAGES \
     python3-pip \
     python3-sphinx \
     python3-virtualenv \
+    python3.6 \
     rdma-core-devel \
     SDL2-devel \
     snappy-devel \
-- 
2.29.2

Re: [PATCH v4 00/24] python: create installable package

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:32PM -0500, John Snow wrote:
> This series factors the python/qemu directory as an installable
> package. It does not yet actually change the mechanics of how any other
> python source in the tree actually consumes it (yet), beyond the import
> path. (some import statements change in a few places.)
> 
> git: https://gitlab.com/jsnow/qemu/-/commits/python-package-mk3
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/254940717
> (New CI job: https://gitlab.com/jsnow/qemu/-/jobs/1024230604)
> 
> The primary motivation of this series is primarily to formalize our
> dependencies on mypy, flake8, isort, and pylint alongside versions that
> are known to work. It does this using the setup.cfg and setup.py
> files. It also adds explicitly pinned versions (using Pipfile.lock) of
> these dependencies that should behave in a repeatable and known way for
> developers and CI environments both. Lastly, it enables those CI checks
> such that we can enforce Python coding quality checks via the CI tests.
> 
> An auxiliary motivation is that this package is formatted in such a way
> that it COULD be uploaded to https://pypi.org/project/qemu and installed
> independently of qemu.git with `pip install qemu`, but that button
> remains *unpushed* and this series *will not* cause any such
> releases. We have time to debate finer points like API guarantees and
> versioning even after this series is merged.
> 
> Some other things this enables that might be of interest:
> 
> With the python tooling as a proper package, you can install this
> package in editable or production mode to a virtual environment, your
> local user environment, or your system packages. The primary benefit of
> this is to gain access to QMP tooling regardless of CWD, without needing
> to battle sys.path (and confounding other python analysis tools).
> 
> For example: when developing, you may go to qemu/python/ and run `make
> venv` followed by `pipenv shell` to activate a virtual environment that
> contains the qemu python packages. These packages will always reflect
> the current version of the source files in the tree. When you are
> finished, you can simply exit the shell (^d) to remove these packages
> from your python environment.
> 
> When not developing, you could install a version of this package to your
> environment outright to gain access to the QMP and QEMUMachine classes
> for lightweight scripting and testing by using pip: "pip install [--user] ."
> 
> TESTING THIS SERIES:
> 
> First of all, nothing should change. Without any intervention,
> everything should behave exactly as it was before. The only new
> information here comes from how to interact with and run the linters
> that will be enforcing code quality standards in this subdirectory.
> 
> To test those, CD to qemu/python first, and then:
> 
> 1. Try "make venv && pipenv shell" to get a venv with the package
> installed to it in editable mode. Ctrl+d exits this venv shell. While in
> this shell, any python script that uses "from qemu.[qmp|machine] import
> ..." should work correctly regardless of where the script is, or what
> your CWD is.
>

Ack here, works as expected.

> You will need Python 3.6 installed on your system to do this step. For
> Fedora: "dnf install python3.6" will do the trick.
>

You may have explained this before, so forgive me asking again.  Why
is this dependent on a given Python version, and not a *minimum*
Python version? Are the checkers themselves susceptible to different
behavior depending on the Python version used?  If so, any hint on the
strategy for developing with say, Python 3.8, and then having issues
caught only on Python 3.6?

> 2. Try "make check" while still in the shell to run the Python linters
> using the venv built in the previous step. This will pull some packages
> from PyPI and run pytest, which will in turn execute mypy, flake8, isort
> and pylint with the correct arguments.
>

Works as expected.  I'll provide more feedback at the specific patches.

> 3. Having exited the shell from above, try "make venv-check". This will
> create and update the venv if needed, then run 'make check' within the
> context of that shell. It should pass as long as the above did.
>

If this makes into a documentation (or on a v5), you may just want to
tell users to run "deactivate" instead of exiting the shell completely.

> 4. Still outside of the venv, you may try running "make check". This
> will not install anything, but unless you have the right Python
> dependencies installed, these tests may fail for you. You might try
> using "pip install --user .[devel]" to install the development packages
> needed to run the tests successfully to your local user's python
> environment. Once done, you will probably want to "pip uninstall qemu"
> to remove that package to avoid it interfering with other things.
>

This is good info for completeness, but I wonder if "make check"
should exist at all.  If it's a requirement for "make check-venv", the
question becomes if it should be advertised.  Hint: I don't think it
should, it just adds some a bit of confusion IMO.

> 5. "make distclean" will delete the venv and any temporary files that
> may have been created by packaging, installing, testing, etc.
>

Works as expected.  Now, unto the individual patches.

Cheers,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 01/24] python/console_socket: avoid one-letter variable

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:33PM -0500, John Snow wrote:
> Fixes pylint warnings.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/console_socket.py | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 02/24] iotests/297: add --namespace-packages to mypy arguments

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:34PM -0500, John Snow wrote:
> mypy is kind of weird about how it handles imports. For legacy reasons,
> it won't load PEP 420 namespaces, because of logic implemented prior to
> that becoming a standard.
> 
> So, if you plan on using any, you have to pass
> --namespace-packages. Alright, fine.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/297 | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 03/24] python: create qemu packages

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:35PM -0500, John Snow wrote:
> move python/qemu/*.py to python/qemu/[machine, qmp]/*.py and update import
> directives across the tree.
> 
> This is done to create a PEP420 namespace package, in which we may
> create subpackages. To do this, the namespace directory ("qemu") should
> not have any modules in it. Those files will go into new 'machine' and 'qmp'
> subpackages instead.
> 
> Implement machine/__init__.py making the top-level classes and functions
> from its various modules available directly inside the package. Change
> qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
> library classes are available directly from "qemu.qmp" instead of
> "qemu.qmp.qmp".
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> 
> ---
> 
> Note for reviewers: in the next patch, I add a utils sub-package and
> move qemu/machine/accel.py to qemu/utils/accel.py. If we like it that
> way, we can squash it in here if we want, or just leave it as its own
> follow-up patch, I am just leaving it as something that will be easy to
> un-do or change for now.

IMO this is fine as it is.

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/{qemu => }/.isort.cfg                |  0
>  python/qemu/__init__.py                     | 11 ------
>  python/qemu/{ => machine}/.flake8           |  0
>  python/qemu/machine/__init__.py             | 41 +++++++++++++++++++++
>  python/qemu/{ => machine}/accel.py          |  0
>  python/qemu/{ => machine}/console_socket.py |  0
>  python/qemu/{ => machine}/machine.py        | 16 +++++---
>  python/qemu/{ => machine}/pylintrc          |  0
>  python/qemu/{ => machine}/qtest.py          |  3 +-
>  python/qemu/{qmp.py => qmp/__init__.py}     | 12 +++++-
>  tests/acceptance/boot_linux.py              |  3 +-
>  tests/qemu-iotests/300                      |  4 +-
>  tests/qemu-iotests/iotests.py               |  2 +-
>  tests/vm/basevm.py                          |  3 +-
>  14 files changed, 70 insertions(+), 25 deletions(-)
>  rename python/{qemu => }/.isort.cfg (100%)
>  delete mode 100644 python/qemu/__init__.py
>  rename python/qemu/{ => machine}/.flake8 (100%)
>  create mode 100644 python/qemu/machine/__init__.py
>  rename python/qemu/{ => machine}/accel.py (100%)
>  rename python/qemu/{ => machine}/console_socket.py (100%)
>  rename python/qemu/{ => machine}/machine.py (98%)
>  rename python/qemu/{ => machine}/pylintrc (100%)
>  rename python/qemu/{ => machine}/qtest.py (99%)
>  rename python/qemu/{qmp.py => qmp/__init__.py} (96%)
> 
> diff --git a/python/qemu/.isort.cfg b/python/.isort.cfg
> similarity index 100%
> rename from python/qemu/.isort.cfg
> rename to python/.isort.cfg
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> deleted file mode 100644
> index 4ca06c34a41..00000000000
> --- a/python/qemu/__init__.py
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -# QEMU library
> -#
> -# Copyright (C) 2015-2016 Red Hat Inc.
> -# Copyright (C) 2012 IBM Corp.
> -#
> -# Authors:
> -#  Fam Zheng <famz@redhat.com>
> -#
> -# This work is licensed under the terms of the GNU GPL, version 2.  See
> -# the COPYING file in the top-level directory.
> -#
> diff --git a/python/qemu/.flake8 b/python/qemu/machine/.flake8
> similarity index 100%
> rename from python/qemu/.flake8
> rename to python/qemu/machine/.flake8
> diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
> new file mode 100644
> index 00000000000..27b0b19abd3
> --- /dev/null
> +++ b/python/qemu/machine/__init__.py
> @@ -0,0 +1,41 @@
> +"""
> +QEMU development and testing library.
> +
> +This library provides a few high-level classes for driving QEMU from a
> +test suite, not intended for production use.
> +
> +- QEMUMachine: Configure and Boot a QEMU VM
> + - QEMUQtestMachine: VM class, with a qtest socket.
> +
> +- QEMUQtestProtocol: Connect to, send/receive qtest messages.
> +
> +- list_accel: List available accelerators
> +- kvm_available: Probe for KVM support
> +- tcg_available: Probe for TCG support
> +"""
> +
> +# Copyright (C) 2020 John Snow for Red Hat Inc.
> +# Copyright (C) 2015-2016 Red Hat Inc.
> +# Copyright (C) 2012 IBM Corp.
> +#
> +# Authors:
> +#  John Snow <jsnow@redhat.com>
> +#  Fam Zheng <fam@euphon.net>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.  See
> +# the COPYING file in the top-level directory.
> +#
> +
> +from .accel import kvm_available, list_accel, tcg_available
> +from .machine import QEMUMachine
> +from .qtest import QEMUQtestMachine, QEMUQtestProtocol
> +
> +
> +__all__ = (
> +    'list_accel',
> +    'kvm_available',
> +    'tcg_available',
> +    'QEMUMachine',
> +    'QEMUQtestProtocol',
> +    'QEMUQtestMachine',
> +)
> diff --git a/python/qemu/accel.py b/python/qemu/machine/accel.py
> similarity index 100%
> rename from python/qemu/accel.py
> rename to python/qemu/machine/accel.py
> diff --git a/python/qemu/console_socket.py b/python/qemu/machine/console_socket.py
> similarity index 100%
> rename from python/qemu/console_socket.py
> rename to python/qemu/machine/console_socket.py
> diff --git a/python/qemu/machine.py b/python/qemu/machine/machine.py
> similarity index 98%
> rename from python/qemu/machine.py
> rename to python/qemu/machine/machine.py
> index 7a40f4604be..dd6ce289350 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -38,8 +38,14 @@
>      Type,
>  )
>  
> -from . import console_socket, qmp
> -from .qmp import QMPMessage, QMPReturnValue, SocketAddrT
> +from qemu.qmp import (
> +    QEMUMonitorProtocol,
> +    QMPMessage,
> +    QMPReturnValue,
> +    SocketAddrT,
> +)
> +
> +from . import console_socket
>  
>

You're moving the files and "isorting" those imports.  Intentional?
IMO a clear isort followed by a move is preferred.

>  LOG = logging.getLogger(__name__)
> @@ -139,7 +145,7 @@ def __init__(self,
>          self._events: List[QMPMessage] = []
>          self._iolog: Optional[str] = None
>          self._qmp_set = True   # Enable QMP monitor by default.
> -        self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
> +        self._qmp_connection: Optional[QEMUMonitorProtocol] = None
>          self._qemu_full_args: Tuple[str, ...] = ()
>          self._temp_dir: Optional[str] = None
>          self._launched = False
> @@ -315,7 +321,7 @@ def _pre_launch(self) -> None:
>              if self._remove_monitor_sockfile:
>                  assert isinstance(self._monitor_address, str)
>                  self._remove_files.append(self._monitor_address)
> -            self._qmp_connection = qmp.QEMUMonitorProtocol(
> +            self._qmp_connection = QEMUMonitorProtocol(
>                  self._monitor_address,
>                  server=True,
>                  nickname=self._name
> @@ -535,7 +541,7 @@ def set_qmp_monitor(self, enabled: bool = True) -> None:
>          self._qmp_set = enabled
>  
>      @property
> -    def _qmp(self) -> qmp.QEMUMonitorProtocol:
> +    def _qmp(self) -> QEMUMonitorProtocol:
>          if self._qmp_connection is None:
>              raise QEMUMachineError("Attempt to access QMP with no connection")
>          return self._qmp_connection
> diff --git a/python/qemu/pylintrc b/python/qemu/machine/pylintrc
> similarity index 100%
> rename from python/qemu/pylintrc
> rename to python/qemu/machine/pylintrc
> diff --git a/python/qemu/qtest.py b/python/qemu/machine/qtest.py
> similarity index 99%
> rename from python/qemu/qtest.py
> rename to python/qemu/machine/qtest.py
> index 39a0cf62fe9..53926e434a7 100644
> --- a/python/qemu/qtest.py
> +++ b/python/qemu/machine/qtest.py
> @@ -26,8 +26,9 @@
>      TextIO,
>  )
>  
> +from qemu.qmp import SocketAddrT
> +
>  from .machine import QEMUMachine
> -from .qmp import SocketAddrT
>  
>  
>  class QEMUQtestProtocol:
> diff --git a/python/qemu/qmp.py b/python/qemu/qmp/__init__.py
> similarity index 96%
> rename from python/qemu/qmp.py
> rename to python/qemu/qmp/__init__.py
> index 2cd4d43036c..9606248a3d2 100644
> --- a/python/qemu/qmp.py
> +++ b/python/qemu/qmp/__init__.py
> @@ -1,4 +1,14 @@
> -""" QEMU Monitor Protocol Python class """
> +"""
> +QEMU Monitor Protocol (QMP) development library & tooling.
> +
> +This package provides a fairly low-level class for communicating to QMP
> +protocol servers, as implemented by QEMU, the QEMU Guest Agent, and the
> +QEMU Storage Daemon. This library is not intended for production use.
> +
> +`QEMUMonitorProtocol` is the primary class of interest, and all errors
> +raised derive from `QMPError`.
> +"""
> +
>  # Copyright (C) 2009, 2010 Red Hat Inc.
>  #
>  # Authors:
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index bcd923bb4a0..212365fd185 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -12,8 +12,7 @@
>  
>  from avocado_qemu import Test, BUILD_DIR
>  
> -from qemu.accel import kvm_available
> -from qemu.accel import tcg_available
> +from qemu.machine import kvm_available, tcg_available
>

There are a few extra files where a similar change is needed:

  $ git grep -E '(kvm|tcg)_available'
  tests/acceptance/virtio-gpu.py:from qemu.accel import kvm_available
  tests/acceptance/virtiofs_submounts.py:from qemu.accel import kvm_available
  tests/vm/aarch64vm.py:from qemu.accel import kvm_available

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 03/24] python: create qemu packages

[John Snow]

On 2/12/21 12:17 AM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:35PM -0500, John Snow wrote:
>> move python/qemu/*.py to python/qemu/[machine, qmp]/*.py and update import
>> directives across the tree.
>>
>> This is done to create a PEP420 namespace package, in which we may
>> create subpackages. To do this, the namespace directory ("qemu") should
>> not have any modules in it. Those files will go into new 'machine' and 'qmp'
>> subpackages instead.
>>
>> Implement machine/__init__.py making the top-level classes and functions
>> from its various modules available directly inside the package. Change
>> qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
>> library classes are available directly from "qemu.qmp" instead of
>> "qemu.qmp.qmp".
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>>
>> ---
>>
>> Note for reviewers: in the next patch, I add a utils sub-package and
>> move qemu/machine/accel.py to qemu/utils/accel.py. If we like it that
>> way, we can squash it in here if we want, or just leave it as its own
>> follow-up patch, I am just leaving it as something that will be easy to
>> un-do or change for now.
> 
> IMO this is fine as it is.
> 

Sure, it just needs to be updated to stand on its own :)

>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   python/{qemu => }/.isort.cfg                |  0
>>   python/qemu/__init__.py                     | 11 ------
>>   python/qemu/{ => machine}/.flake8           |  0
>>   python/qemu/machine/__init__.py             | 41 +++++++++++++++++++++
>>   python/qemu/{ => machine}/accel.py          |  0
>>   python/qemu/{ => machine}/console_socket.py |  0
>>   python/qemu/{ => machine}/machine.py        | 16 +++++---
>>   python/qemu/{ => machine}/pylintrc          |  0
>>   python/qemu/{ => machine}/qtest.py          |  3 +-
>>   python/qemu/{qmp.py => qmp/__init__.py}     | 12 +++++-
>>   tests/acceptance/boot_linux.py              |  3 +-
>>   tests/qemu-iotests/300                      |  4 +-
>>   tests/qemu-iotests/iotests.py               |  2 +-
>>   tests/vm/basevm.py                          |  3 +-
>>   14 files changed, 70 insertions(+), 25 deletions(-)
>>   rename python/{qemu => }/.isort.cfg (100%)
>>   delete mode 100644 python/qemu/__init__.py
>>   rename python/qemu/{ => machine}/.flake8 (100%)
>>   create mode 100644 python/qemu/machine/__init__.py
>>   rename python/qemu/{ => machine}/accel.py (100%)
>>   rename python/qemu/{ => machine}/console_socket.py (100%)
>>   rename python/qemu/{ => machine}/machine.py (98%)
>>   rename python/qemu/{ => machine}/pylintrc (100%)
>>   rename python/qemu/{ => machine}/qtest.py (99%)
>>   rename python/qemu/{qmp.py => qmp/__init__.py} (96%)
>>
>> diff --git a/python/qemu/.isort.cfg b/python/.isort.cfg
>> similarity index 100%
>> rename from python/qemu/.isort.cfg
>> rename to python/.isort.cfg
>> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
>> deleted file mode 100644
>> index 4ca06c34a41..00000000000
>> --- a/python/qemu/__init__.py
>> +++ /dev/null
>> @@ -1,11 +0,0 @@
>> -# QEMU library
>> -#
>> -# Copyright (C) 2015-2016 Red Hat Inc.
>> -# Copyright (C) 2012 IBM Corp.
>> -#
>> -# Authors:
>> -#  Fam Zheng <famz@redhat.com>
>> -#
>> -# This work is licensed under the terms of the GNU GPL, version 2.  See
>> -# the COPYING file in the top-level directory.
>> -#
>> diff --git a/python/qemu/.flake8 b/python/qemu/machine/.flake8
>> similarity index 100%
>> rename from python/qemu/.flake8
>> rename to python/qemu/machine/.flake8
>> diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
>> new file mode 100644
>> index 00000000000..27b0b19abd3
>> --- /dev/null
>> +++ b/python/qemu/machine/__init__.py
>> @@ -0,0 +1,41 @@
>> +"""
>> +QEMU development and testing library.
>> +
>> +This library provides a few high-level classes for driving QEMU from a
>> +test suite, not intended for production use.
>> +
>> +- QEMUMachine: Configure and Boot a QEMU VM
>> + - QEMUQtestMachine: VM class, with a qtest socket.
>> +
>> +- QEMUQtestProtocol: Connect to, send/receive qtest messages.
>> +
>> +- list_accel: List available accelerators
>> +- kvm_available: Probe for KVM support
>> +- tcg_available: Probe for TCG support
>> +"""
>> +
>> +# Copyright (C) 2020 John Snow for Red Hat Inc.
>> +# Copyright (C) 2015-2016 Red Hat Inc.
>> +# Copyright (C) 2012 IBM Corp.
>> +#
>> +# Authors:
>> +#  John Snow <jsnow@redhat.com>
>> +#  Fam Zheng <fam@euphon.net>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2.  See
>> +# the COPYING file in the top-level directory.
>> +#
>> +
>> +from .accel import kvm_available, list_accel, tcg_available
>> +from .machine import QEMUMachine
>> +from .qtest import QEMUQtestMachine, QEMUQtestProtocol
>> +
>> +
>> +__all__ = (
>> +    'list_accel',
>> +    'kvm_available',
>> +    'tcg_available',
>> +    'QEMUMachine',
>> +    'QEMUQtestProtocol',
>> +    'QEMUQtestMachine',
>> +)
>> diff --git a/python/qemu/accel.py b/python/qemu/machine/accel.py
>> similarity index 100%
>> rename from python/qemu/accel.py
>> rename to python/qemu/machine/accel.py
>> diff --git a/python/qemu/console_socket.py b/python/qemu/machine/console_socket.py
>> similarity index 100%
>> rename from python/qemu/console_socket.py
>> rename to python/qemu/machine/console_socket.py
>> diff --git a/python/qemu/machine.py b/python/qemu/machine/machine.py
>> similarity index 98%
>> rename from python/qemu/machine.py
>> rename to python/qemu/machine/machine.py
>> index 7a40f4604be..dd6ce289350 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine/machine.py
>> @@ -38,8 +38,14 @@
>>       Type,
>>   )
>>   
>> -from . import console_socket, qmp
>> -from .qmp import QMPMessage, QMPReturnValue, SocketAddrT
>> +from qemu.qmp import (
>> +    QEMUMonitorProtocol,
>> +    QMPMessage,
>> +    QMPReturnValue,
>> +    SocketAddrT,
>> +)
>> +
>> +from . import console_socket
>>   
>>
> 
> You're moving the files and "isorting" those imports.  Intentional?
> IMO a clear isort followed by a move is preferred.
> 

Unconscious habit; they needed to be rewritten because of the move. I 
didn't see a strong benefit to changing them just prior to needing to 
rewrite them anyway. Similarly, I didn't see a benefit to moving and 
rewriting them and then isorting them in the next patch.

>>   LOG = logging.getLogger(__name__)
>> @@ -139,7 +145,7 @@ def __init__(self,
>>           self._events: List[QMPMessage] = []
>>           self._iolog: Optional[str] = None
>>           self._qmp_set = True   # Enable QMP monitor by default.
>> -        self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
>> +        self._qmp_connection: Optional[QEMUMonitorProtocol] = None
>>           self._qemu_full_args: Tuple[str, ...] = ()
>>           self._temp_dir: Optional[str] = None
>>           self._launched = False
>> @@ -315,7 +321,7 @@ def _pre_launch(self) -> None:
>>               if self._remove_monitor_sockfile:
>>                   assert isinstance(self._monitor_address, str)
>>                   self._remove_files.append(self._monitor_address)
>> -            self._qmp_connection = qmp.QEMUMonitorProtocol(
>> +            self._qmp_connection = QEMUMonitorProtocol(
>>                   self._monitor_address,
>>                   server=True,
>>                   nickname=self._name
>> @@ -535,7 +541,7 @@ def set_qmp_monitor(self, enabled: bool = True) -> None:
>>           self._qmp_set = enabled
>>   
>>       @property
>> -    def _qmp(self) -> qmp.QEMUMonitorProtocol:
>> +    def _qmp(self) -> QEMUMonitorProtocol:
>>           if self._qmp_connection is None:
>>               raise QEMUMachineError("Attempt to access QMP with no connection")
>>           return self._qmp_connection
>> diff --git a/python/qemu/pylintrc b/python/qemu/machine/pylintrc
>> similarity index 100%
>> rename from python/qemu/pylintrc
>> rename to python/qemu/machine/pylintrc
>> diff --git a/python/qemu/qtest.py b/python/qemu/machine/qtest.py
>> similarity index 99%
>> rename from python/qemu/qtest.py
>> rename to python/qemu/machine/qtest.py
>> index 39a0cf62fe9..53926e434a7 100644
>> --- a/python/qemu/qtest.py
>> +++ b/python/qemu/machine/qtest.py
>> @@ -26,8 +26,9 @@
>>       TextIO,
>>   )
>>   
>> +from qemu.qmp import SocketAddrT
>> +
>>   from .machine import QEMUMachine
>> -from .qmp import SocketAddrT
>>   
>>   
>>   class QEMUQtestProtocol:
>> diff --git a/python/qemu/qmp.py b/python/qemu/qmp/__init__.py
>> similarity index 96%
>> rename from python/qemu/qmp.py
>> rename to python/qemu/qmp/__init__.py
>> index 2cd4d43036c..9606248a3d2 100644
>> --- a/python/qemu/qmp.py
>> +++ b/python/qemu/qmp/__init__.py
>> @@ -1,4 +1,14 @@
>> -""" QEMU Monitor Protocol Python class """
>> +"""
>> +QEMU Monitor Protocol (QMP) development library & tooling.
>> +
>> +This package provides a fairly low-level class for communicating to QMP
>> +protocol servers, as implemented by QEMU, the QEMU Guest Agent, and the
>> +QEMU Storage Daemon. This library is not intended for production use.
>> +
>> +`QEMUMonitorProtocol` is the primary class of interest, and all errors
>> +raised derive from `QMPError`.
>> +"""
>> +
>>   # Copyright (C) 2009, 2010 Red Hat Inc.
>>   #
>>   # Authors:
>> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
>> index bcd923bb4a0..212365fd185 100644
>> --- a/tests/acceptance/boot_linux.py
>> +++ b/tests/acceptance/boot_linux.py
>> @@ -12,8 +12,7 @@
>>   
>>   from avocado_qemu import Test, BUILD_DIR
>>   
>> -from qemu.accel import kvm_available
>> -from qemu.accel import tcg_available
>> +from qemu.machine import kvm_available, tcg_available
>>
> 
> There are a few extra files where a similar change is needed:
> 
>    $ git grep -E '(kvm|tcg)_available'
>    tests/acceptance/virtio-gpu.py:from qemu.accel import kvm_available
>    tests/acceptance/virtiofs_submounts.py:from qemu.accel import kvm_available
>    tests/vm/aarch64vm.py:from qemu.accel import kvm_available

Oh, right. These are ones I fixed in the subsequent patch, but break for 
this intermediate commit. Easy enough to fix.

Our test suites are too cumbersome and too slow now to test on 
intermediate commits anymore, I've long since abandoned the idea.

I wonder if that's a scalability problem for our review process.

Thanks,
--js

Re: [PATCH v4 00/24] python: create installable package

[John Snow]

On 2/11/21 9:52 PM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:32PM -0500, John Snow wrote:
>> This series factors the python/qemu directory as an installable
>> package. It does not yet actually change the mechanics of how any other
>> python source in the tree actually consumes it (yet), beyond the import
>> path. (some import statements change in a few places.)
>>
>> git: https://gitlab.com/jsnow/qemu/-/commits/python-package-mk3
>> CI: https://gitlab.com/jsnow/qemu/-/pipelines/254940717
>> (New CI job: https://gitlab.com/jsnow/qemu/-/jobs/1024230604)
>>
>> The primary motivation of this series is primarily to formalize our
>> dependencies on mypy, flake8, isort, and pylint alongside versions that
>> are known to work. It does this using the setup.cfg and setup.py
>> files. It also adds explicitly pinned versions (using Pipfile.lock) of
>> these dependencies that should behave in a repeatable and known way for
>> developers and CI environments both. Lastly, it enables those CI checks
>> such that we can enforce Python coding quality checks via the CI tests.
>>
>> An auxiliary motivation is that this package is formatted in such a way
>> that it COULD be uploaded to https://pypi.org/project/qemu and installed
>> independently of qemu.git with `pip install qemu`, but that button
>> remains *unpushed* and this series *will not* cause any such
>> releases. We have time to debate finer points like API guarantees and
>> versioning even after this series is merged.
>>
>> Some other things this enables that might be of interest:
>>
>> With the python tooling as a proper package, you can install this
>> package in editable or production mode to a virtual environment, your
>> local user environment, or your system packages. The primary benefit of
>> this is to gain access to QMP tooling regardless of CWD, without needing
>> to battle sys.path (and confounding other python analysis tools).
>>
>> For example: when developing, you may go to qemu/python/ and run `make
>> venv` followed by `pipenv shell` to activate a virtual environment that
>> contains the qemu python packages. These packages will always reflect
>> the current version of the source files in the tree. When you are
>> finished, you can simply exit the shell (^d) to remove these packages
>> from your python environment.
>>
>> When not developing, you could install a version of this package to your
>> environment outright to gain access to the QMP and QEMUMachine classes
>> for lightweight scripting and testing by using pip: "pip install [--user] ."
>>
>> TESTING THIS SERIES:
>>
>> First of all, nothing should change. Without any intervention,
>> everything should behave exactly as it was before. The only new
>> information here comes from how to interact with and run the linters
>> that will be enforcing code quality standards in this subdirectory.
>>
>> To test those, CD to qemu/python first, and then:
>>
>> 1. Try "make venv && pipenv shell" to get a venv with the package
>> installed to it in editable mode. Ctrl+d exits this venv shell. While in
>> this shell, any python script that uses "from qemu.[qmp|machine] import
>> ..." should work correctly regardless of where the script is, or what
>> your CWD is.
>>
> 
> Ack here, works as expected.
> 

That's a relief!

>> You will need Python 3.6 installed on your system to do this step. For
>> Fedora: "dnf install python3.6" will do the trick.
>>
> 
> You may have explained this before, so forgive me asking again.  Why
> is this dependent on a given Python version, and not a *minimum*
> Python version? Are the checkers themselves susceptible to different
> behavior depending on the Python version used?  If so, any hint on the
> strategy for developing with say, Python 3.8, and then having issues
> caught only on Python 3.6?
> 

It's a good question, and I have struggled with communicating the 
language. So here are a few points:

(1) Users will not need Python 3.6 on their local systems to be able to 
run the linters; they will be able to run "make check" to run it under 
*any* environment -- granted they have the necessary packages. (mypy, 
pylint, pytest, flake8, and isort.) See note #2 below:

(2) `pip install [--user] .[devel]` will install the needed dependencies 
to the local environment/venv regardless of python version. These 
dependencies are not pinned, but do express a minimum viable version (in 
setup.cfg) for each tool that I tested rigorously.

(3) The CI system will target Python 3.6 specifically, because that is 
our minimum version. This will work to prevent future features from 
bleeding into the codebase, which was a notable problem when we were 
targeting simultaneous 2.7 and 3.x deployments. If we were going to only 
run against one version, 3.6 is the defensibly correct version to run 
against. If we want to run against more, I'd say let's not let 
perfection be the enemy of good enough.

(4) I considered it important to be able to run, as much as is possible, 
the *EXACT SAME* environment for tests as the CI runs. For this purpose, 
"make venv-check" uses pipenv to install a pinned set of dependencies 
(that might be lower than what you'd get otherwise), and uses explicitly 
Python 3.6. This is to make it repeatable and simple to run a test 
that's as close to what the CI runner will do as possible. This takes a 
lot of the thinking out of it.


So, to answer you more directly:

- 3.6 is a *minimum* for "make check". (setup.cfg)
- 3.6 is a *dependency* for "make venv-check". (Pipfile, Pipfile.lock)

And, as far as known version problems:

- pylint 2.6.0 is not compatible with Python 3.9. They are working on 
it. 2.6.1-dev works alright, but isn't released yet. The linters may be 
unavailable to folks with 3.9-only environment.

Workarounds:

- Make your own venv using 3.7 or 3.8
- Use the make venv-check entry point to use 3.6.
- Give up and push it to gitlab and see if the CI test passes.


And, finally, here's my maintainer testing strategy/promises:

- I endeavor to make sure this package is tested and working on any 
non-EOL Python version (3.6 - 3.9 at time of writing)
- I endeavor to ensure that it is easy to test and lint these files on 
your local development system with minimum hassle
- Given the volatility of compatibility between different versions of 
linters and python, however, the current *canonical check* is Python 
3.6, using the explicitly pinned versions in the Pipfile.lock. There may 
be times (like right now) where the local linting test may not work with 
your version of Python.

>> 2. Try "make check" while still in the shell to run the Python linters
>> using the venv built in the previous step. This will pull some packages
>> from PyPI and run pytest, which will in turn execute mypy, flake8, isort
>> and pylint with the correct arguments.
>>
> 
> Works as expected.  I'll provide more feedback at the specific patches.
> 
>> 3. Having exited the shell from above, try "make venv-check". This will
>> create and update the venv if needed, then run 'make check' within the
>> context of that shell. It should pass as long as the above did.
>>
> 
> If this makes into a documentation (or on a v5), you may just want to
> tell users to run "deactivate" instead of exiting the shell completely.
> 

It depends on how you entered the shell. Literally "pipenv shell" does 
create a new shell process that you should exit from. Since I suggested 
using the pipenv invocation, I match that suggestion by telling the user 
to exit (instead of deactivate).

You know too much about python for your own good!

>> 4. Still outside of the venv, you may try running "make check". This
>> will not install anything, but unless you have the right Python
>> dependencies installed, these tests may fail for you. You might try
>> using "pip install --user .[devel]" to install the development packages
>> needed to run the tests successfully to your local user's python
>> environment. Once done, you will probably want to "pip uninstall qemu"
>> to remove that package to avoid it interfering with other things.
>>
> 
> This is good info for completeness, but I wonder if "make check"
> should exist at all.  If it's a requirement for "make check-venv", the
> question becomes if it should be advertised.  Hint: I don't think it
> should, it just adds some a bit of confusion IMO.
> 

I think it's cleanly separated... If you understand much about how 
python virtual environments work.

- You can run the tests on any environment you want! or,
- You can run those tests on a very, very specific environment that is 
controlled with a militaristic, iron fist.

current belief: People who are not developing python can just wait for 
the little orb to turn green in Gitlab-CI and not worry about running 
any particular test at all, actually. This current patch-set does not 
integrate these tests into "make check" at all, on purpose.

People who ARE developing this package (infrequently) will need to know 
which they want to run. My suggestion is to use "make venv-check" as the 
best predictor of the CI check, though it can be slow and clunky.

If you develop on this package a *lot*, and you regularly use a venv to 
develop, "make check" starts to make a lot more sense.

"make" with no arguments produces the help message;

```
python packaging help:

make venv:       Create pipenv's virtual environment.
     NOTE: Requires Python 3.6 and pipenv.
           Will download packages from PyPI.
     HINT: On Fedora: 'sudo dnf install python36 pipenv'

make venv-check: run linters using pipenv's virtual environment.

make check:      run linters using the current environment.
     Hint: Install deps with: 'pip install ".[devel]"'
```

... Which, I suppose if you don't use python much, it might not make 
sense to you which environment you want to run the tests under. I can 
probably add a hint:

"HINT: Run this test if you're unsure which to run."

I could probably also amend the hint for the "make check" option, to say 
something like: "This is an advanced option that uses your current 
environment's python3 interpreter and packages with zero setup."

>> 5. "make distclean" will delete the venv and any temporary files that
>> may have been created by packaging, installing, testing, etc.
>>
> 
> Works as expected.  Now, unto the individual patches.
> 
> Cheers,
> - Cleber.
> 

Thanks!
--js

Re: [PATCH v4 04/24] python: create utils sub-package

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:36PM -0500, John Snow wrote:
> Create a space for miscellaneous things that don't belong strictly in
> "qemu.machine" nor "qemu.qmp" packages.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/__init__.py         |  8 --------
>  python/qemu/utils/__init__.py           | 23 +++++++++++++++++++++++
>  python/qemu/{machine => utils}/accel.py |  0
>  tests/acceptance/boot_linux.py          |  2 +-
>  tests/acceptance/virtio-gpu.py          |  2 +-
>  tests/acceptance/virtiofs_submounts.py  |  2 +-
>  tests/vm/aarch64vm.py                   |  2 +-
>  tests/vm/basevm.py                      |  3 ++-
>  8 files changed, 29 insertions(+), 13 deletions(-)
>  create mode 100644 python/qemu/utils/__init__.py
>  rename python/qemu/{machine => utils}/accel.py (100%)
> 
> diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
> index 27b0b19abd3..891a8f784b5 100644
> --- a/python/qemu/machine/__init__.py
> +++ b/python/qemu/machine/__init__.py
> @@ -8,10 +8,6 @@
>   - QEMUQtestMachine: VM class, with a qtest socket.
>  
>  - QEMUQtestProtocol: Connect to, send/receive qtest messages.
> -
> -- list_accel: List available accelerators
> -- kvm_available: Probe for KVM support
> -- tcg_available: Probe for TCG support
>  """
>  
>  # Copyright (C) 2020 John Snow for Red Hat Inc.
> @@ -26,15 +22,11 @@
>  # the COPYING file in the top-level directory.
>  #
>  
> -from .accel import kvm_available, list_accel, tcg_available
>  from .machine import QEMUMachine
>  from .qtest import QEMUQtestMachine, QEMUQtestProtocol
>  
>  
>  __all__ = (
> -    'list_accel',
> -    'kvm_available',
> -    'tcg_available',
>      'QEMUMachine',
>      'QEMUQtestProtocol',
>      'QEMUQtestMachine',
> diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
> new file mode 100644
> index 00000000000..edf807a93e5
> --- /dev/null
> +++ b/python/qemu/utils/__init__.py
> @@ -0,0 +1,23 @@
> +"""
> +QEMU development and testing utilities
> +
> +This library provides a small handful of utilities for performing various tasks
> +not directly related to the launching of a VM.
> +
> +The only module included at present is accel; its public functions are
> +repeated here for your convenience:
> +
> +- list_accel: List available accelerators
> +- kvm_available: Probe for KVM support
> +- tcg_available: Prove for TCG support
> +"""
> +
> +# pylint: disable=import-error
> +from .accel import kvm_available, list_accel, tcg_available
> +
> +
> +__all__ = (
> +    'list_accel',
> +    'kvm_available',
> +    'tcg_available',
> +)
> diff --git a/python/qemu/machine/accel.py b/python/qemu/utils/accel.py
> similarity index 100%
> rename from python/qemu/machine/accel.py
> rename to python/qemu/utils/accel.py
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index 212365fd185..824cf03d5f4 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -12,7 +12,7 @@
>  
>  from avocado_qemu import Test, BUILD_DIR
>  
> -from qemu.machine import kvm_available, tcg_available
> +from qemu.utils import kvm_available, tcg_available
>  

With the latest changes merged earlier Today, this won't be necessary
anymore on boot_linux.py, but the equivalent change will be necessary
on tests/acceptance/avocado_qemu/__init__.py.

With the change mentioned above (which you would catch on a rebase):

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 05/24] python: add qemu package installer

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:37PM -0500, John Snow wrote:
> Add setup.cfg and setup.py, necessary for installing a package via
> pip. Add a rst document explaining the basics of what this package is

Nitpick 1: setup.cfg and setup.py are indeed used by pip, your
statement is correct.  But, it hides the fact that these can be used
without pip.  On a source tree based install, you may want to simply
use "python setup.py develop" to achieve what "pip install -e" would
do (without pip ever entering the picture).

Nitpick 2: while most people will understand what you mean by "rst
document", I believe that "Add a README file in reStructuredText
format" would be more obvious.

> for and who to contact for more information. This document will be used
> as the landing page for the package on PyPI.
> 
> I am not yet using a pyproject.toml style package manifest, because
> "editable" installs are not defined by PEP-517 and pip did not have
> support for this for some time; I consider the feature necessary for
> development.
>

I'm glad you kept it like this... I bet there's going to be another
PEP out, replacing the status quo, by the time I finish this review.

> Use a light-weight setup.py instead.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/PACKAGE.rst | 32 ++++++++++++++++++++++++++++++++
>  python/setup.cfg   | 19 +++++++++++++++++++
>  python/setup.py    | 23 +++++++++++++++++++++++
>  3 files changed, 74 insertions(+)
>  create mode 100644 python/PACKAGE.rst
>  create mode 100644 python/setup.cfg
>  create mode 100755 python/setup.py
> 
> diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
> new file mode 100644
> index 00000000000..0e714c87eb3
> --- /dev/null
> +++ b/python/PACKAGE.rst
> @@ -0,0 +1,32 @@
> +QEMU Python Tooling
> +===================
> +
> +This package provides QEMU tooling used by the QEMU project to build,
> +configure, and test QEMU. It is not a fully-fledged SDK and it is subject
> +to change at any time.
> +
> +Usage
> +-----
> +
> +The ``qemu.qmp`` subpackage provides a library for communicating with
> +QMP servers. The ``qemu.machine`` subpackage offers rudimentary
> +facilities for launching and managing QEMU processes. Refer to each
> +package's documentation
> +(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
> +for more information.
> +

This gives the impression that those are the only subpackages (and
they were).  Better to reword it taking into account the qemu.utils
subpackage and possibly others (leave it open so that it doesn't rot
so quickly).

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 06/24] python: add VERSION file

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:38PM -0500, John Snow wrote:
> Python infrastructure as it exists today is not capable reliably of
> single-sourcing a package version from a parent directory. The authors
> of pip are working to correct this, but as of today this is not possible.
> 
> The problem is that when using pip to build and install a python
> package, it copies files over to a temporary directory and performs its
> build there. This loses access to any information in the parent
> directory, including git itself.
> 
> Further, Python versions have a standard (PEP 440) that may or may not
> follow QEMU's versioning. In general, it does; but naturally QEMU does
> not follow PEP 440. To avoid any automatically-generated conflict, a
> manual version file is preferred.
> 
> 
> I am proposing:
> 
> - Python tooling follows the QEMU version, indirectly, but with a major
>   version of 0 to indicate that the API is not expected to be
>   stable. This would mean version 0.5.2.0, 0.5.1.1, 0.5.3.0, etc.
> 
> - In the event that a Python package needs to be updated independently
>   of the QEMU version, a pre-release alpha version should be preferred,
>   but *only* after inclusion to the qemu development or stable branches.
> 
>   e.g. 0.5.2.0a1, 0.5.2.0a2, and so on should be preferred prior to
>   5.2.0's release.
> 
> - The Python core tooling makes absolutely no version compatibility
>   checks or constraints. It *may* work with releases of QEMU from the
>   past or future, but it is not required to.
> 
>   i.e., "qemu.machine" will, for now, remain in lock-step with QEMU.
> 
> - We reserve the right to split the qemu package into independently
>   versioned subpackages at a later date. This might allow for us to
>   begin versioning QMP independently from QEMU at a later date, if
>   we so choose.
> 
> 
> Implement this versioning scheme by adding a VERSION file and setting it
> to 0.6.0.0a1.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/VERSION   | 1 +
>  python/setup.cfg | 1 +
>  2 files changed, 2 insertions(+)
>  create mode 100644 python/VERSION
> 

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 07/24] python: add directory structure README.rst files

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:39PM -0500, John Snow wrote:
> Add short readmes to python/, python/qemu/, python/qemu/machine,
> python/qemu/qmp, and python/qemu/utils that explain the directory
> hierarchy. These readmes are visible when browsing the source on
> e.g. gitlab/github and are designed to help new developers/users quickly
> make sense of the source tree.
> 
> They are not designed for inclusion in a published manual.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/README.rst              | 41 ++++++++++++++++++++++++++++++++++
>  python/qemu/README.rst         |  8 +++++++
>  python/qemu/machine/README.rst |  9 ++++++++
>  python/qemu/qmp/README.rst     |  9 ++++++++
>  python/qemu/utils/README.rst   |  9 ++++++++
>  5 files changed, 76 insertions(+)
>  create mode 100644 python/README.rst
>  create mode 100644 python/qemu/README.rst
>  create mode 100644 python/qemu/machine/README.rst
>  create mode 100644 python/qemu/qmp/README.rst
>  create mode 100644 python/qemu/utils/README.rst
>

It's not often I complain about too much documentation, but I honestly
think this will not scale.  I understand that the intention is to help
users browsing through the directory structure it has a huge potential
for bit rot.

The READMEs at the first two levels seem OK, but the ones at the
subpackages level will be a maintainance nightmare.  I would *very
much* move those (subpackage ones) documentation into the Python file
themselves.

> diff --git a/python/README.rst b/python/README.rst
> new file mode 100644
> index 00000000000..6a14b99e104
> --- /dev/null
> +++ b/python/README.rst
> @@ -0,0 +1,41 @@
> +QEMU Python Tooling
> +===================
> +
> +This directory houses Python tooling used by the QEMU project to build,
> +configure, and test QEMU. It is organized by namespace (``qemu``), and
> +then by package (``qemu/machine``, ``qemu/qmp``).
> +
> +``setup.py`` is used by ``pip`` to install this tooling to the current
> +environment. ``setup.cfg`` provides the packaging configuration used by
> +setup.py in a setuptools specific format. You will generally invoke it
> +by doing one of the following:
> +
> +1. ``pip3 install .`` will install these packages to your current
> +   environment. If you are inside a virtual environment, they will
> +   install there. If you are not, it will attempt to install to the
> +   global environment, which is not recommended.
> +
> +2. ``pip3 install --user .`` will install these packages to your user's
> +   local python packages. If you are inside of a virtual environment,
> +   this will fail.
> +
> +If you amend the ``-e`` argument, pip will install in "editable" mode;
> +which installs a version of the package that installs a forwarder
> +pointing to these files, such that the package always reflects the
> +latest version in your git tree.
> +
> +See `Installing packages using pip and virtual environments
> +<https://packaging.python.org/guides/installing-using-pip-and-virtual-environments/>`_
> +for more information.
> +
> +
> +Files in this directory
> +-----------------------
> +
> +- ``qemu/`` Python package source directory.
> +- ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
> +- ``README.rst`` you are here!
> +- ``VERSION`` contains the PEP-440 compliant version used to describe
> +  this package; it is referenced by ``setup.cfg``.
> +- ``setup.cfg`` houses setuptools package configuration.
> +- ``setup.py`` is the setuptools installer used by pip; See above.
> diff --git a/python/qemu/README.rst b/python/qemu/README.rst
> new file mode 100644
> index 00000000000..d04943f526c
> --- /dev/null
> +++ b/python/qemu/README.rst
> @@ -0,0 +1,8 @@
> +QEMU Python Namespace
> +=====================
> +
> +This directory serves as the root of a `Python PEP 420 implicit
> +namespace package <https://www.python.org/dev/peps/pep-0420/>`_.
> +
> +Each directory below is assumed to be an installable Python package that
> +is available under the ``qemu.<package>`` namespace.
> diff --git a/python/qemu/machine/README.rst b/python/qemu/machine/README.rst
> new file mode 100644
> index 00000000000..ac2b4fffb42
> --- /dev/null
> +++ b/python/qemu/machine/README.rst
> @@ -0,0 +1,9 @@
> +qemu.machine package
> +====================
> +
> +This package provides core utilities used for testing and debugging
> +QEMU. It is used by the iotests, vm tests, acceptance tests, and several
> +other utilities in the ./scripts directory. It is not a fully-fledged
> +SDK and it is subject to change at any time.
> +
> +See the documentation in ``__init__.py`` for more information.
> diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
> new file mode 100644
> index 00000000000..c21951491cf
> --- /dev/null
> +++ b/python/qemu/qmp/README.rst
> @@ -0,0 +1,9 @@
> +qemu.qmp package
> +================
> +
> +This package provides a library used for connecting to and communicating
> +with QMP servers. It is used extensively by iotests, vm tests,
> +acceptance tests, and other utilities in the ./scripts directory. It is
> +not a fully-fledged SDK and is subject to change at any time.
> +
> +See the documentation in ``__init__.py`` for more information.
> diff --git a/python/qemu/utils/README.rst b/python/qemu/utils/README.rst
> new file mode 100644
> index 00000000000..4b33c1f27e1
> --- /dev/null
> +++ b/python/qemu/utils/README.rst
> @@ -0,0 +1,9 @@
> +qemu.utils package
> +==================
> +
> +This package provides misc utilities used for testing and debugging
> +QEMU. It is used most directly by the qemu.machine package, but has some
> +uses by the vm and acceptance tests for determining accelerator support.
> +
> +See the documentation in ``__init__.py`` and ``accel.py`` for more
> +information.

And example of the bit rot and the huge maintainance cost is when a
new file is added here, let's say, "qemu/utils/network.py".  I think
your good intentions would quickly backfire.

Regards,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 08/24] python: Add pipenv support

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
> pipenv is a tool used for managing virtual environments with pinned,
> explicit dependencies. It is used for precisely recreating python
> virtual environments.
> 
> pipenv uses two files to do this:
> 
> (1) Pipfile, which is similar in purpose and scope to what setup.py
> lists. It specifies the requisite minimum to get a functional
> environment for using this package.
> 
> (2) Pipfile.lock, which is similar in purpose to `pip freeze >
> requirements.txt`. It specifies a canonical virtual environment used for
> deployment or testing. This ensures that all users have repeatable
> results.
> 
> The primary benefit of using this tool is to ensure repeatable CI
> results with a known set of packages. Although I endeavor to support as
> many versions as I can, the fluid nature of the Python toolchain often
> means tailoring code for fairly specific versions.
> 
> Note that pipenv is *not* required to install or use this module; this is
> purely for the sake of repeatable testing by CI or developers.
> 
> Here, a "blank" pipfile is added with no dependencies, but specifies
> Python 3.6 for the virtual environment.
> 
> Pipfile will specify our version minimums, while Pipfile.lock specifies
> an exact loudout of packages that were known to operate correctly. This

Layout? Loadout?

> latter file provides the real value for easy setup of container images
> and CI environments.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/Pipfile | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>  create mode 100644 python/Pipfile
>

Other than that,

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 08/24] python: Add pipenv support

[Cleber Rosa]

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

On Tue, Feb 16, 2021 at 09:59:47PM -0500, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
> > pipenv is a tool used for managing virtual environments with pinned,
> > explicit dependencies. It is used for precisely recreating python
> > virtual environments.
> > 
> > pipenv uses two files to do this:
> > 
> > (1) Pipfile, which is similar in purpose and scope to what setup.py
> > lists. It specifies the requisite minimum to get a functional
> > environment for using this package.
> > 
> > (2) Pipfile.lock, which is similar in purpose to `pip freeze >
> > requirements.txt`. It specifies a canonical virtual environment used for
> > deployment or testing. This ensures that all users have repeatable
> > results.
> > 
> > The primary benefit of using this tool is to ensure repeatable CI
> > results with a known set of packages. Although I endeavor to support as
> > many versions as I can, the fluid nature of the Python toolchain often
> > means tailoring code for fairly specific versions.
> > 
> > Note that pipenv is *not* required to install or use this module; this is
> > purely for the sake of repeatable testing by CI or developers.
> > 
> > Here, a "blank" pipfile is added with no dependencies, but specifies
> > Python 3.6 for the virtual environment.
> > 
> > Pipfile will specify our version minimums, while Pipfile.lock specifies
> > an exact loudout of packages that were known to operate correctly. This
> 
> Layout? Loadout?
> 
> > latter file provides the real value for easy setup of container images
> > and CI environments.
> > 
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  python/Pipfile | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >  create mode 100644 python/Pipfile
> >
> 
> Other than that,
> 
> Reviewed-by: Cleber Rosa <crosa@redhat.com>

Actually, just one suggestion: bump the position of this patch twice.
It makes it easier to understand its purpose if it is placed right
before the "python: add pylint to pipenv" patch.

Cheers,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 09/24] python: add pylint import exceptions

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:41PM -0500, John Snow wrote:
> Pylint 2.5.x and 2.6.x have regressions that make import checking
> inconsistent, see:
> 
> https://github.com/PyCQA/pylint/issues/3609
> https://github.com/PyCQA/pylint/issues/3624
> https://github.com/PyCQA/pylint/issues/3651
> 
> Pinning to 2.4.4 is worse, because it mandates versions of shared
> dependencies that are too old for features we want in isort and mypy.
> Oh well.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/__init__.py | 3 +++
>  python/qemu/machine/machine.py  | 2 +-
>  python/qemu/machine/qtest.py    | 2 +-
>  3 files changed, 5 insertions(+), 2 deletions(-)
>

I can see your struggle on those issues, so I choose not to punish
myself attempting to replicate them.  I'll forever trust you in these
matters.

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 10/24] python: move pylintrc into setup.cfg

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:42PM -0500, John Snow wrote:
> Delete the empty settings now that it's sharing a home with settings for
> other tools.
> 
> pylint can now be run from this folder as "pylint qemu".
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/pylintrc | 58 ------------------------------------
>  python/setup.cfg             | 29 ++++++++++++++++++
>  2 files changed, 29 insertions(+), 58 deletions(-)
>  delete mode 100644 python/qemu/machine/pylintrc
>

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 00/24] python: create installable package

[Cleber Rosa]

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

On Mon, Feb 15, 2021 at 04:32:29PM -0500, John Snow wrote:
> On 2/11/21 9:52 PM, Cleber Rosa wrote:
> > On Thu, Feb 11, 2021 at 01:58:32PM -0500, John Snow wrote:
> > > This series factors the python/qemu directory as an installable
> > > package. It does not yet actually change the mechanics of how any other
> > > python source in the tree actually consumes it (yet), beyond the import
> > > path. (some import statements change in a few places.)
> > > 
> > > git: https://gitlab.com/jsnow/qemu/-/commits/python-package-mk3
> > > CI: https://gitlab.com/jsnow/qemu/-/pipelines/254940717
> > > (New CI job: https://gitlab.com/jsnow/qemu/-/jobs/1024230604)
> > > 
> > > The primary motivation of this series is primarily to formalize our
> > > dependencies on mypy, flake8, isort, and pylint alongside versions that
> > > are known to work. It does this using the setup.cfg and setup.py
> > > files. It also adds explicitly pinned versions (using Pipfile.lock) of
> > > these dependencies that should behave in a repeatable and known way for
> > > developers and CI environments both. Lastly, it enables those CI checks
> > > such that we can enforce Python coding quality checks via the CI tests.
> > > 
> > > An auxiliary motivation is that this package is formatted in such a way
> > > that it COULD be uploaded to https://pypi.org/project/qemu and installed
> > > independently of qemu.git with `pip install qemu`, but that button
> > > remains *unpushed* and this series *will not* cause any such
> > > releases. We have time to debate finer points like API guarantees and
> > > versioning even after this series is merged.
> > > 
> > > Some other things this enables that might be of interest:
> > > 
> > > With the python tooling as a proper package, you can install this
> > > package in editable or production mode to a virtual environment, your
> > > local user environment, or your system packages. The primary benefit of
> > > this is to gain access to QMP tooling regardless of CWD, without needing
> > > to battle sys.path (and confounding other python analysis tools).
> > > 
> > > For example: when developing, you may go to qemu/python/ and run `make
> > > venv` followed by `pipenv shell` to activate a virtual environment that
> > > contains the qemu python packages. These packages will always reflect
> > > the current version of the source files in the tree. When you are
> > > finished, you can simply exit the shell (^d) to remove these packages
> > > from your python environment.
> > > 
> > > When not developing, you could install a version of this package to your
> > > environment outright to gain access to the QMP and QEMUMachine classes
> > > for lightweight scripting and testing by using pip: "pip install [--user] ."
> > > 
> > > TESTING THIS SERIES:
> > > 
> > > First of all, nothing should change. Without any intervention,
> > > everything should behave exactly as it was before. The only new
> > > information here comes from how to interact with and run the linters
> > > that will be enforcing code quality standards in this subdirectory.
> > > 
> > > To test those, CD to qemu/python first, and then:
> > > 
> > > 1. Try "make venv && pipenv shell" to get a venv with the package
> > > installed to it in editable mode. Ctrl+d exits this venv shell. While in
> > > this shell, any python script that uses "from qemu.[qmp|machine] import
> > > ..." should work correctly regardless of where the script is, or what
> > > your CWD is.
> > > 
> > 
> > Ack here, works as expected.
> > 
> 
> That's a relief!
> 
> > > You will need Python 3.6 installed on your system to do this step. For
> > > Fedora: "dnf install python3.6" will do the trick.
> > > 
> > 
> > You may have explained this before, so forgive me asking again.  Why
> > is this dependent on a given Python version, and not a *minimum*
> > Python version? Are the checkers themselves susceptible to different
> > behavior depending on the Python version used?  If so, any hint on the
> > strategy for developing with say, Python 3.8, and then having issues
> > caught only on Python 3.6?
> > 
> 
> It's a good question, and I have struggled with communicating the language.
> So here are a few points:
> 
> (1) Users will not need Python 3.6 on their local systems to be able to run
> the linters; they will be able to run "make check" to run it under *any*
> environment -- granted they have the necessary packages. (mypy, pylint,
> pytest, flake8, and isort.) See note #2 below:
> 
> (2) `pip install [--user] .[devel]` will install the needed dependencies to
> the local environment/venv regardless of python version. These dependencies
> are not pinned, but do express a minimum viable version (in setup.cfg) for
> each tool that I tested rigorously.
> 
> (3) The CI system will target Python 3.6 specifically, because that is our
> minimum version. This will work to prevent future features from bleeding
> into the codebase, which was a notable problem when we were targeting
> simultaneous 2.7 and 3.x deployments. If we were going to only run against
> one version, 3.6 is the defensibly correct version to run against. If we
> want to run against more, I'd say let's not let perfection be the enemy of
> good enough.
> 
> (4) I considered it important to be able to run, as much as is possible, the
> *EXACT SAME* environment for tests as the CI runs. For this purpose, "make
> venv-check" uses pipenv to install a pinned set of dependencies (that might
> be lower than what you'd get otherwise), and uses explicitly Python 3.6.
> This is to make it repeatable and simple to run a test that's as close to
> what the CI runner will do as possible. This takes a lot of the thinking out
> of it.
> 
> 
> So, to answer you more directly:
> 
> - 3.6 is a *minimum* for "make check". (setup.cfg)
> - 3.6 is a *dependency* for "make venv-check". (Pipfile, Pipfile.lock)
>

OK, this answers my question.  It wasn't completely clear to me that
you took the care of using Pipfile.lock with one, and only one, Python
version (via "make venv-check").

> And, as far as known version problems:
> 
> - pylint 2.6.0 is not compatible with Python 3.9. They are working on it.
> 2.6.1-dev works alright, but isn't released yet. The linters may be
> unavailable to folks with 3.9-only environment.
> 
> Workarounds:
> 
> - Make your own venv using 3.7 or 3.8
> - Use the make venv-check entry point to use 3.6.
> - Give up and push it to gitlab and see if the CI test passes.
> 
> 
> And, finally, here's my maintainer testing strategy/promises:
> 
> - I endeavor to make sure this package is tested and working on any non-EOL
> Python version (3.6 - 3.9 at time of writing)
> - I endeavor to ensure that it is easy to test and lint these files on your
> local development system with minimum hassle
> - Given the volatility of compatibility between different versions of
> linters and python, however, the current *canonical check* is Python 3.6,
> using the explicitly pinned versions in the Pipfile.lock. There may be times
> (like right now) where the local linting test may not work with your version
> of Python.
> 
> > > 2. Try "make check" while still in the shell to run the Python linters
> > > using the venv built in the previous step. This will pull some packages
> > > from PyPI and run pytest, which will in turn execute mypy, flake8, isort
> > > and pylint with the correct arguments.
> > > 
> > 
> > Works as expected.  I'll provide more feedback at the specific patches.
> > 
> > > 3. Having exited the shell from above, try "make venv-check". This will
> > > create and update the venv if needed, then run 'make check' within the
> > > context of that shell. It should pass as long as the above did.
> > > 
> > 
> > If this makes into a documentation (or on a v5), you may just want to
> > tell users to run "deactivate" instead of exiting the shell completely.
> > 
> 
> It depends on how you entered the shell. Literally "pipenv shell" does
> create a new shell process that you should exit from. Since I suggested
> using the pipenv invocation, I match that suggestion by telling the user to
> exit (instead of deactivate).
> 
> You know too much about python for your own good!
>

May be the exact opposite! It's that I'm much more used to plain venvs
and those will not create a shell, and exiting will quit a session,
which is very annoying!

> > > 4. Still outside of the venv, you may try running "make check". This
> > > will not install anything, but unless you have the right Python
> > > dependencies installed, these tests may fail for you. You might try
> > > using "pip install --user .[devel]" to install the development packages
> > > needed to run the tests successfully to your local user's python
> > > environment. Once done, you will probably want to "pip uninstall qemu"
> > > to remove that package to avoid it interfering with other things.
> > > 
> > 
> > This is good info for completeness, but I wonder if "make check"
> > should exist at all.  If it's a requirement for "make check-venv", the
> > question becomes if it should be advertised.  Hint: I don't think it
> > should, it just adds some a bit of confusion IMO.
> > 
> 
> I think it's cleanly separated... If you understand much about how python
> virtual environments work.
> 
> - You can run the tests on any environment you want! or,
> - You can run those tests on a very, very specific environment that is
> controlled with a militaristic, iron fist.
> 
> current belief: People who are not developing python can just wait for the
> little orb to turn green in Gitlab-CI and not worry about running any
> particular test at all, actually. This current patch-set does not integrate
> these tests into "make check" at all, on purpose.
> 
> People who ARE developing this package (infrequently) will need to know
> which they want to run. My suggestion is to use "make venv-check" as the
> best predictor of the CI check, though it can be slow and clunky.
> 
> If you develop on this package a *lot*, and you regularly use a venv to
> develop, "make check" starts to make a lot more sense.
> 
> "make" with no arguments produces the help message;
> 
> ```
> python packaging help:
> 
> make venv:       Create pipenv's virtual environment.
>     NOTE: Requires Python 3.6 and pipenv.
>           Will download packages from PyPI.
>     HINT: On Fedora: 'sudo dnf install python36 pipenv'
> 
> make venv-check: run linters using pipenv's virtual environment.
> 
> make check:      run linters using the current environment.
>     Hint: Install deps with: 'pip install ".[devel]"'
> ```
> 
> ... Which, I suppose if you don't use python much, it might not make sense
> to you which environment you want to run the tests under. I can probably add
> a hint:
> 
> "HINT: Run this test if you're unsure which to run."
>

Yeah, the "make" help message is useful as it is, but I'd indeed include
this extra bit of info.

Thanks for the very detailed explanation!
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 05/24] python: add qemu package installer

[John Snow]

On 2/16/21 9:23 PM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:37PM -0500, John Snow wrote:
>> Add setup.cfg and setup.py, necessary for installing a package via
>> pip. Add a rst document explaining the basics of what this package is
> 
> Nitpick 1: setup.cfg and setup.py are indeed used by pip, your
> statement is correct.  But, it hides the fact that these can be used
> without pip.  On a source tree based install, you may want to simply
> use "python setup.py develop" to achieve what "pip install -e" would
> do (without pip ever entering the picture).
> 

This is intentional. pip and setup.py actually use different pathways 
that are not identical. I do not recommend you call setup.py directly 
anymore. Once pyproject.toml is more widespread, there won't even *be* a 
setup.py.

> Nitpick 2: while most people will understand what you mean by "rst
> document", I believe that "Add a README file in reStructuredText
> format" would be more obvious.
> 

Sure.

>> for and who to contact for more information. This document will be used
>> as the landing page for the package on PyPI.
>>
>> I am not yet using a pyproject.toml style package manifest, because
>> "editable" installs are not defined by PEP-517 and pip did not have
>> support for this for some time; I consider the feature necessary for
>> development.
>>
> 
> I'm glad you kept it like this... I bet there's going to be another
> PEP out, replacing the status quo, by the time I finish this review.
> 

Actually, between writing this series last year and this latest respin, 
pip supports editable installs for pyproject.toml distributions now :)

...but I was still hesitant to take the leap, because maybe that's still 
too modern for the 3.6-based distributions we target and support.

...And I left the message alone, because I didn't re-research the exact 
reason I'm not using pyproject.toml now. Eh.

>> Use a light-weight setup.py instead.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   python/PACKAGE.rst | 32 ++++++++++++++++++++++++++++++++
>>   python/setup.cfg   | 19 +++++++++++++++++++
>>   python/setup.py    | 23 +++++++++++++++++++++++
>>   3 files changed, 74 insertions(+)
>>   create mode 100644 python/PACKAGE.rst
>>   create mode 100644 python/setup.cfg
>>   create mode 100755 python/setup.py
>>
>> diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
>> new file mode 100644
>> index 00000000000..0e714c87eb3
>> --- /dev/null
>> +++ b/python/PACKAGE.rst
>> @@ -0,0 +1,32 @@
>> +QEMU Python Tooling
>> +===================
>> +
>> +This package provides QEMU tooling used by the QEMU project to build,
>> +configure, and test QEMU. It is not a fully-fledged SDK and it is subject
>> +to change at any time.
>> +
>> +Usage
>> +-----
>> +
>> +The ``qemu.qmp`` subpackage provides a library for communicating with
>> +QMP servers. The ``qemu.machine`` subpackage offers rudimentary
>> +facilities for launching and managing QEMU processes. Refer to each
>> +package's documentation
>> +(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
>> +for more information.
>> +
> 
> This gives the impression that those are the only subpackages (and
> they were).  Better to reword it taking into account the qemu.utils
> subpackage and possibly others (leave it open so that it doesn't rot
> so quickly).
> 

Intentional again. I don't, for the moment, consider that utils package 
something "supported" in the public sense, so I didn't feel the urge to 
draw attention to it.

> - Cleber.
> 

Re: [PATCH v4 08/24] python: Add pipenv support

[John Snow]

On 2/16/21 9:59 PM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
>> pipenv is a tool used for managing virtual environments with pinned,
>> explicit dependencies. It is used for precisely recreating python
>> virtual environments.
>>
>> pipenv uses two files to do this:
>>
>> (1) Pipfile, which is similar in purpose and scope to what setup.py
>> lists. It specifies the requisite minimum to get a functional
>> environment for using this package.
>>
>> (2) Pipfile.lock, which is similar in purpose to `pip freeze >
>> requirements.txt`. It specifies a canonical virtual environment used for
>> deployment or testing. This ensures that all users have repeatable
>> results.
>>
>> The primary benefit of using this tool is to ensure repeatable CI
>> results with a known set of packages. Although I endeavor to support as
>> many versions as I can, the fluid nature of the Python toolchain often
>> means tailoring code for fairly specific versions.
>>
>> Note that pipenv is *not* required to install or use this module; this is
>> purely for the sake of repeatable testing by CI or developers.
>>
>> Here, a "blank" pipfile is added with no dependencies, but specifies
>> Python 3.6 for the virtual environment.
>>
>> Pipfile will specify our version minimums, while Pipfile.lock specifies
>> an exact loudout of packages that were known to operate correctly. This
> 
> Layout? Loadout?

Whoops, "loadout", yeah.

> 
>> latter file provides the real value for easy setup of container images
>> and CI environments.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   python/Pipfile | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>   create mode 100644 python/Pipfile
>>
> 
> Other than that,
> 
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> 

Thanks!

Re: [PATCH v4 09/24] python: add pylint import exceptions

[John Snow]

On 2/16/21 10:07 PM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:41PM -0500, John Snow wrote:
>> Pylint 2.5.x and 2.6.x have regressions that make import checking
>> inconsistent, see:
>>
>> https://github.com/PyCQA/pylint/issues/3609
>> https://github.com/PyCQA/pylint/issues/3624
>> https://github.com/PyCQA/pylint/issues/3651
>>
>> Pinning to 2.4.4 is worse, because it mandates versions of shared
>> dependencies that are too old for features we want in isort and mypy.
>> Oh well.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   python/qemu/machine/__init__.py | 3 +++
>>   python/qemu/machine/machine.py  | 2 +-
>>   python/qemu/machine/qtest.py    | 2 +-
>>   3 files changed, 5 insertions(+), 2 deletions(-)
>>
> 
> I can see your struggle on those issues, so I choose not to punish
> myself attempting to replicate them.  I'll forever trust you in these
> matters.
> 

:~)

> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> 

Thanks!

Re: [PATCH v4 11/24] python: add pylint to pipenv

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:43PM -0500, John Snow wrote:
> We are specifying >= pylint 2.6.x for two reasons:
> 
> 1. For setup.cfg support, added in pylint 2.5.x
> 2. To clarify that we are using a version that has incompatibly dropped
> bad-whitespace checks.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/Pipfile      |   1 +
>  python/Pipfile.lock | 137 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 138 insertions(+)
>  create mode 100644 python/Pipfile.lock
> 

Tested at this point with:

 $ pipenv install --dev
 $ pipenv run pip freeze
 astroid==2.4.2
 isort==5.7.0
 lazy-object-proxy==1.4.3
 mccabe==0.6.1
 pylint==2.6.0
 six==1.15.0
 toml==0.10.2
 typed-ast==1.4.2
 wrapt==1.12.1

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 12/24] python: move flake8 config to setup.cfg

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:44PM -0500, John Snow wrote:
> Update the comment concerning the flake8 exception to match commit
> 42c0dd12, whose commit message stated:
> 
> A note on the flake8 exception: flake8 will warn on *any* bare except,
> but pylint's is context-aware and will suppress the warning if you
> re-raise the exception.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/.flake8 | 2 --
>  python/setup.cfg            | 3 +++
>  2 files changed, 3 insertions(+), 2 deletions(-)
>  delete mode 100644 python/qemu/machine/.flake8
>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 13/24] python: Add flake8 to pipenv

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:45PM -0500, John Snow wrote:
> flake8 3.5.x does not support the --extend-ignore syntax used in the
> .flake8 file to gracefully extend default ignores, so 3.6.x is our
> minimum requirement. There is no known upper bound.
> 
> flake8 can be run from the python/ directory with no arguments.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/Pipfile      |  1 +
>  python/Pipfile.lock | 51 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 15/24] python: add mypy to pipenv

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:47PM -0500, John Snow wrote:
> 0.730 appears to be about the oldest version that works with the
> features we want, including nice human readable output (to make sure
> iotest 297 passes), and type-parameterized Popen generics.
> 
> 0.770, however, supports adding 'strict' to the config file, so require
> at least 0.770.
> 
> Now that we are checking a namespace package, we need to tell mypy to
> allow PEP420 namespaces, so modify the mypy config as part of the move.
> 
> mypy can now be run from the python root by typing 'mypy qemu'.
>

 $ mypy qemu 
 qemu/utils/accel.py: error: Source file found twice under different module names: 'qmp' and 'qemu.qmp'
 Found 1 error in 1 file (errors prevented further checking)

I guess you meant 'mypy -p qemu'.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/Pipfile      |  1 +
>  python/Pipfile.lock | 37 ++++++++++++++++++++++++++++++++++++-
>  python/setup.cfg    |  1 +
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 

With that change,

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 16/24] python: move .isort.cfg into setup.cfg

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:48PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/.isort.cfg | 7 -------
>  python/setup.cfg  | 8 ++++++++
>  2 files changed, 8 insertions(+), 7 deletions(-)
>  delete mode 100644 python/.isort.cfg
> 

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 17/24] python/qemu: add isort to pipenv

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:49PM -0500, John Snow wrote:
> isort 5.0.0 through 5.0.4 has a bug that causes it to misinterpret
> certain "from ..." clauses that are not related to imports.
> 
> Require 5.0.5 or greater.
> 
> isort can be run with 'isort -c qemu' from the python root.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  python/Pipfile      | 1 +
>  python/Pipfile.lock | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 18/24] python/qemu: add qemu package itself to pipenv

[Cleber Rosa]

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

On Thu, Feb 11, 2021 at 01:58:50PM -0500, John Snow wrote:
> This adds the python qemu packages themselves to the pipenv manifest.
> 'pipenv sync' will create a virtual environment sufficient to use the SDK.
> 'pipenv sync --dev' will create a virtual environment sufficient to use
> and test the SDK (with pylint, mypy, isort, flake8, etc.)
> 
> The qemu packages are installed in 'editable' mode; all changes made to
> the python package inside the git tree will be reflected in the
> installed package without reinstallation. This includes changes made
> via git pull and so on.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/Pipfile      | 1 +
>  python/Pipfile.lock | 9 +++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 15/24] python: add mypy to pipenv

[John Snow]

On 2/16/21 11:38 PM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:47PM -0500, John Snow wrote:
>> 0.730 appears to be about the oldest version that works with the
>> features we want, including nice human readable output (to make sure
>> iotest 297 passes), and type-parameterized Popen generics.
>>
>> 0.770, however, supports adding 'strict' to the config file, so require
>> at least 0.770.
>>
>> Now that we are checking a namespace package, we need to tell mypy to
>> allow PEP420 namespaces, so modify the mypy config as part of the move.
>>
>> mypy can now be run from the python root by typing 'mypy qemu'.
>>
> 
>   $ mypy qemu
>   qemu/utils/accel.py: error: Source file found twice under different module names: 'qmp' and 'qemu.qmp'
>   Found 1 error in 1 file (errors prevented further checking)
> 
> I guess you meant 'mypy -p qemu'.
> 

Ah, crud! Yes, this is something that has popped up recently.

mypy's "figure out where we are when run without arguments" 
functionality does not work exactly correct in some cases.

I forget the specifics, but "mypy qemu" used to work for this series, 
and at some point it ... stopped working. I updated the pytest 
invocation, but I didn't update the comments here.

There's a github meta-issue about this, and about how mypy's package 
discovery is extremely confusing:

https://github.com/python/mypy/issues/8584

It's extremely a big landmine on which you may hoist yourself.

>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   python/Pipfile      |  1 +
>>   python/Pipfile.lock | 37 ++++++++++++++++++++++++++++++++++++-
>>   python/setup.cfg    |  1 +
>>   3 files changed, 38 insertions(+), 1 deletion(-)
>>
> 
> With that change,
> 
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> Tested-by: Cleber Rosa <crosa@redhat.com>
> 

Re: [PATCH v4 18/24] python/qemu: add qemu package itself to pipenv

[John Snow]

On 2/16/21 11:47 PM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:50PM -0500, John Snow wrote:
>> This adds the python qemu packages themselves to the pipenv manifest.
>> 'pipenv sync' will create a virtual environment sufficient to use the SDK.
>> 'pipenv sync --dev' will create a virtual environment sufficient to use
>> and test the SDK (with pylint, mypy, isort, flake8, etc.)
>>
>> The qemu packages are installed in 'editable' mode; all changes made to
>> the python package inside the git tree will be reflected in the
>> installed package without reinstallation. This includes changes made
>> via git pull and so on.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   python/Pipfile      | 1 +
>>   python/Pipfile.lock | 9 +++++++--
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
> 
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> Tested-by: Cleber Rosa <crosa@redhat.com>
> 

Thanks for the reviews so far!

Re: [PATCH v4 08/24] python: Add pipenv support

[John Snow]

On 2/16/21 10:02 PM, Cleber Rosa wrote:
> On Tue, Feb 16, 2021 at 09:59:47PM -0500, Cleber Rosa wrote:
>> On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
>>> pipenv is a tool used for managing virtual environments with pinned,
>>> explicit dependencies. It is used for precisely recreating python
>>> virtual environments.
>>>
>>> pipenv uses two files to do this:
>>>
>>> (1) Pipfile, which is similar in purpose and scope to what setup.py
>>> lists. It specifies the requisite minimum to get a functional
>>> environment for using this package.
>>>
>>> (2) Pipfile.lock, which is similar in purpose to `pip freeze >
>>> requirements.txt`. It specifies a canonical virtual environment used for
>>> deployment or testing. This ensures that all users have repeatable
>>> results.
>>>
>>> The primary benefit of using this tool is to ensure repeatable CI
>>> results with a known set of packages. Although I endeavor to support as
>>> many versions as I can, the fluid nature of the Python toolchain often
>>> means tailoring code for fairly specific versions.
>>>
>>> Note that pipenv is *not* required to install or use this module; this is
>>> purely for the sake of repeatable testing by CI or developers.
>>>
>>> Here, a "blank" pipfile is added with no dependencies, but specifies
>>> Python 3.6 for the virtual environment.
>>>
>>> Pipfile will specify our version minimums, while Pipfile.lock specifies
>>> an exact loudout of packages that were known to operate correctly. This
>>
>> Layout? Loadout?
>>
>>> latter file provides the real value for easy setup of container images
>>> and CI environments.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   python/Pipfile | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>   create mode 100644 python/Pipfile
>>>
>>
>> Other than that,
>>
>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> 
> Actually, just one suggestion: bump the position of this patch twice.
> It makes it easier to understand its purpose if it is placed right
> before the "python: add pylint to pipenv" patch.
> 
> Cheers,
> - Cleber.
> 

The way the series is laid out is:

01-02: pre-requisite fixes
03-07: Create the package, readmes, etc.
08:    Pipenv support
09-11: Pylint
12-13: flake8
14-15: mypy
16-17: isort
18-20: Testing and pre-requisites
21-23: Polish
24: CI support

Moving the pipenv patch to just before the final pylint patch works OK, 
but breaks up the pylint section. Should I still do it?

--js


(Hm, by this layout, I should probably actually move the pylint fix in 
#01 down to appear after the pipenv patch. I could also move the flake8 
fixes in #21 up to be near the other flake8 patches.)

Re: [PATCH v4 08/24] python: Add pipenv support

[Cleber Rosa]

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

On Wed, Feb 17, 2021 at 12:28:22PM -0500, John Snow wrote:
> On 2/16/21 10:02 PM, Cleber Rosa wrote:
> > On Tue, Feb 16, 2021 at 09:59:47PM -0500, Cleber Rosa wrote:
> > > On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
> > > > pipenv is a tool used for managing virtual environments with pinned,
> > > > explicit dependencies. It is used for precisely recreating python
> > > > virtual environments.
> > > > 
> > > > pipenv uses two files to do this:
> > > > 
> > > > (1) Pipfile, which is similar in purpose and scope to what setup.py
> > > > lists. It specifies the requisite minimum to get a functional
> > > > environment for using this package.
> > > > 
> > > > (2) Pipfile.lock, which is similar in purpose to `pip freeze >
> > > > requirements.txt`. It specifies a canonical virtual environment used for
> > > > deployment or testing. This ensures that all users have repeatable
> > > > results.
> > > > 
> > > > The primary benefit of using this tool is to ensure repeatable CI
> > > > results with a known set of packages. Although I endeavor to support as
> > > > many versions as I can, the fluid nature of the Python toolchain often
> > > > means tailoring code for fairly specific versions.
> > > > 
> > > > Note that pipenv is *not* required to install or use this module; this is
> > > > purely for the sake of repeatable testing by CI or developers.
> > > > 
> > > > Here, a "blank" pipfile is added with no dependencies, but specifies
> > > > Python 3.6 for the virtual environment.
> > > > 
> > > > Pipfile will specify our version minimums, while Pipfile.lock specifies
> > > > an exact loudout of packages that were known to operate correctly. This
> > > 
> > > Layout? Loadout?
> > > 
> > > > latter file provides the real value for easy setup of container images
> > > > and CI environments.
> > > > 
> > > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > > ---
> > > >   python/Pipfile | 11 +++++++++++
> > > >   1 file changed, 11 insertions(+)
> > > >   create mode 100644 python/Pipfile
> > > > 
> > > 
> > > Other than that,
> > > 
> > > Reviewed-by: Cleber Rosa <crosa@redhat.com>
> > 
> > Actually, just one suggestion: bump the position of this patch twice.
> > It makes it easier to understand its purpose if it is placed right
> > before the "python: add pylint to pipenv" patch.
> > 
> > Cheers,
> > - Cleber.
> > 
> 
> The way the series is laid out is:
> 
> 01-02: pre-requisite fixes
> 03-07: Create the package, readmes, etc.
> 08:    Pipenv support
> 09-11: Pylint
> 12-13: flake8
> 14-15: mypy
> 16-17: isort
> 18-20: Testing and pre-requisites
> 21-23: Polish
> 24: CI support
> 
> Moving the pipenv patch to just before the final pylint patch works OK, but
> breaks up the pylint section. Should I still do it?
>

OK, now with that approach of groupping in min, it sounds reasonable.
My previous point was that pipenv is not needed until right before #10,
but that's just nitpicking and almost bikeshedding (I won't admit it
easily).

> --js
> 
> 
> (Hm, by this layout, I should probably actually move the pylint fix in #01
> down to appear after the pipenv patch. I could also move the flake8 fixes in
> #21 up to be near the other flake8 patches.)

Sounds more consistent.

Cheers,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 07/24] python: add directory structure README.rst files

[John Snow]

On 2/16/21 9:47 PM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:39PM -0500, John Snow wrote:
>> Add short readmes to python/, python/qemu/, python/qemu/machine,
>> python/qemu/qmp, and python/qemu/utils that explain the directory
>> hierarchy. These readmes are visible when browsing the source on
>> e.g. gitlab/github and are designed to help new developers/users quickly
>> make sense of the source tree.
>>
>> They are not designed for inclusion in a published manual.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   python/README.rst              | 41 ++++++++++++++++++++++++++++++++++
>>   python/qemu/README.rst         |  8 +++++++
>>   python/qemu/machine/README.rst |  9 ++++++++
>>   python/qemu/qmp/README.rst     |  9 ++++++++
>>   python/qemu/utils/README.rst   |  9 ++++++++
>>   5 files changed, 76 insertions(+)
>>   create mode 100644 python/README.rst
>>   create mode 100644 python/qemu/README.rst
>>   create mode 100644 python/qemu/machine/README.rst
>>   create mode 100644 python/qemu/qmp/README.rst
>>   create mode 100644 python/qemu/utils/README.rst
>>
> 
> It's not often I complain about too much documentation, but I honestly
> think this will not scale.  I understand that the intention is to help
> users browsing through the directory structure it has a huge potential
> for bit rot.
> 

Always a nice problem to have too much instead of too little. ;)

> The READMEs at the first two levels seem OK, but the ones at the
> subpackages level will be a maintainance nightmare.  I would *very
> much* move those (subpackage ones) documentation into the Python file
> themselves.
> 

I don't think there's much, if anything, to move into those files. There 
are some details about how the module relates to the rest of the QEMU 
tree, but I think those details aren't appropriate to have "in" the 
python package itself -- they won't apply to whatever environment they 
get installed in.

>> diff --git a/python/README.rst b/python/README.rst
>> new file mode 100644
>> index 00000000000..6a14b99e104
>> --- /dev/null
>> +++ b/python/README.rst
>> @@ -0,0 +1,41 @@
>> +QEMU Python Tooling
>> +===================
>> +
>> +This directory houses Python tooling used by the QEMU project to build,
>> +configure, and test QEMU. It is organized by namespace (``qemu``), and
>> +then by package (``qemu/machine``, ``qemu/qmp``).
>> +
>> +``setup.py`` is used by ``pip`` to install this tooling to the current
>> +environment. ``setup.cfg`` provides the packaging configuration used by
>> +setup.py in a setuptools specific format. You will generally invoke it
>> +by doing one of the following:
>> +
>> +1. ``pip3 install .`` will install these packages to your current
>> +   environment. If you are inside a virtual environment, they will
>> +   install there. If you are not, it will attempt to install to the
>> +   global environment, which is not recommended.
>> +
>> +2. ``pip3 install --user .`` will install these packages to your user's
>> +   local python packages. If you are inside of a virtual environment,
>> +   this will fail.
>> +
>> +If you amend the ``-e`` argument, pip will install in "editable" mode;
>> +which installs a version of the package that installs a forwarder
>> +pointing to these files, such that the package always reflects the
>> +latest version in your git tree.
>> +
>> +See `Installing packages using pip and virtual environments
>> +<https://packaging.python.org/guides/installing-using-pip-and-virtual-environments/>`_
>> +for more information.
>> +
>> +
>> +Files in this directory
>> +-----------------------
>> +
>> +- ``qemu/`` Python package source directory.
>> +- ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
>> +- ``README.rst`` you are here!
>> +- ``VERSION`` contains the PEP-440 compliant version used to describe
>> +  this package; it is referenced by ``setup.cfg``.
>> +- ``setup.cfg`` houses setuptools package configuration.
>> +- ``setup.py`` is the setuptools installer used by pip; See above.

Top-level doc seems fine to you though? I expect this directory to be 
reasonably low-traffic in terms of file additions/removals. If/when we 
start using sphinx to generate documentation for the qemu packages, we 
can probably link to e.g. readthedocs from here.

Not ready yet, of course.

>> diff --git a/python/qemu/README.rst b/python/qemu/README.rst
>> new file mode 100644
>> index 00000000000..d04943f526c
>> --- /dev/null
>> +++ b/python/qemu/README.rst
>> @@ -0,0 +1,8 @@
>> +QEMU Python Namespace
>> +=====================
>> +
>> +This directory serves as the root of a `Python PEP 420 implicit
>> +namespace package <https://www.python.org/dev/peps/pep-0420/>`_.
>> +
>> +Each directory below is assumed to be an installable Python package that
>> +is available under the ``qemu.<package>`` namespace.

This one is short and sweet. Not at risk of rotting. See below for a 
suggestion that might appease both of us WRT per-subpackage READMEs.

>> diff --git a/python/qemu/machine/README.rst b/python/qemu/machine/README.rst
>> new file mode 100644
>> index 00000000000..ac2b4fffb42
>> --- /dev/null
>> +++ b/python/qemu/machine/README.rst
>> @@ -0,0 +1,9 @@
>> +qemu.machine package
>> +====================
>> +
>> +This package provides core utilities used for testing and debugging
>> +QEMU. It is used by the iotests, vm tests, acceptance tests, and several
>> +other utilities in the ./scripts directory. It is not a fully-fledged
>> +SDK and it is subject to change at any time.
>> + >> +See the documentation in ``__init__.py`` for more information.

This is actually *pretty* brief, and I didn't intend for it to be 
exhaustively complete. I am trying to say "Please look at the python 
docstrings in __init__.py for real details!" because I was also worried 
about bitrot and duplicating docs.

Though, sure, it does duplicate at least some of the basic information. 
We have three-ish choices:

(1) Eh, fine, leave it here.
(2) Update it to be a simple pointer to __init__.py only; i.e. just the 
title heading and the "Please see __init__.py for details" hint.
(3) Just remove it. It should be common knowledge to investigate 
__init__.py for package-level documentation.
   (3b) Remove it, but update the PEP420-level document to contain an
        extra blurb that says "See each subpackage's __init__.py for more
        information."

Your preference?

>> diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
>> new file mode 100644
>> index 00000000000..c21951491cf
>> --- /dev/null
>> +++ b/python/qemu/qmp/README.rst
>> @@ -0,0 +1,9 @@
>> +qemu.qmp package
>> +================
>> +
>> +This package provides a library used for connecting to and communicating
>> +with QMP servers. It is used extensively by iotests, vm tests,
>> +acceptance tests, and other utilities in the ./scripts directory. It is
>> +not a fully-fledged SDK and is subject to change at any time.
>> +
>> +See the documentation in ``__init__.py`` for more information.

Same story as above. This is a pretty brief explanation that explains 
its role in the QEMU source tree, but doesn't explain much about the 
package itself. The same three options as above make sense to me here.

>> diff --git a/python/qemu/utils/README.rst b/python/qemu/utils/README.rst
>> new file mode 100644
>> index 00000000000..4b33c1f27e1
>> --- /dev/null
>> +++ b/python/qemu/utils/README.rst
>> @@ -0,0 +1,9 @@
>> +qemu.utils package
>> +==================
>> +
>> +This package provides misc utilities used for testing and debugging
>> +QEMU. It is used most directly by the qemu.machine package, but has some
>> +uses by the vm and acceptance tests for determining accelerator support.
>> +
>> +See the documentation in ``__init__.py`` and ``accel.py`` for more
>> +information.
> 

I broke my own convention here and mentioned something beside 
__init__.py. I will remove that.

> And example of the bit rot and the huge maintainance cost is when a
> new file is added here, let's say, "qemu/utils/network.py".  I think
> your good intentions would quickly backfire.
> 

As you point out, that little slip-up of mine gave you room to attack 
the bitrot :) I avoided it in two other places, but the utils package is 
sooooo tiny I gave in to trying to be helpful and pointed out the real 
implementation file.

> Regards,
> - Cleber.
> 

I'm still somewhat on the fence. I was trying to make the python 
directories accessible to outside contributors as much as was humanly 
possible, but the bitrot concern is a good point. I think it can be 
alleviated by making clear that these files are just little "see also" 
pointers that should quite likely not rot very quickly.

Lemme know your final thoughts here and I'll adjust accordingly.

--js