Re: [PATCH v3 01/15] python: create qemu packages

[Cleber Rosa <crosa@redhat.com>]

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

On Tue, Oct 20, 2020 at 03:35:41PM -0400, 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>
> ---
>  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/297                      |  2 +-
>  tests/qemu-iotests/300                      |  4 +-
>  tests/qemu-iotests/iotests.py               |  2 +-
>  tests/vm/basevm.py                          |  3 +-
>  15 files changed, 71 insertions(+), 26 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 4ca06c34a4..0000000000
> --- 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 0000000000..27b0b19abd
> --- /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',
> +)

How far would this approach go?  I mean, should all future APIs be developed
inside module under "qemu/machine", say "qemu/machine/foo.py" and their main
functionality imported here?  Or do you see this as a temporary state?

IMO, this is hard to maintain, and is a very easy path to future
inconsistencies.

> 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 6420f01bed..a5dc305539 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
> @@ -314,7 +320,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
> @@ -534,7 +540,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 39a0cf62fe..53926e434a 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 2cd4d43036..9606248a3d 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 0055dc7cee..54f999e647 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
>

I think this reveals a slight downgrade of the API caused by the usage
of the symbols from "qemu.machine" namespace.

My point is that there are valid use cases for using "kvm_available"
without using a "QEMUMachine".  This presuposes that "qemu.machine" is
named like that because "QEMUMachine" are the main actors of this
namespace.

Naming is hard, (so don't assume I believe "utils" below is the best
or even a good name), but I'd be more confortable using an API along
the lines:

   from qemu.utils.accel import kvm_available
   kvm_available()

OR

   from qemu.utils import accel
   accel.kvm_available()

This would also remove the need to keep importing symbols into the
current "qemu.machine" namespace.

>  from avocado.utils import cloudinit
>  from avocado.utils import network
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 5c5420712b..8236875222 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -36,7 +36,7 @@ MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
>      --disallow-any-generics --disallow-incomplete-defs \
>      --disallow-untyped-decorators --no-implicit-optional \
>      --warn-redundant-casts --warn-unused-ignores \
> -    --no-implicit-reexport iotests.py
> +    --no-implicit-reexport --namespace-packages iotests.py
>  
>  # success, all done
>  echo "*** done"
> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> index 5b75121b84..a41fac4e1d 100755
> --- a/tests/qemu-iotests/300
> +++ b/tests/qemu-iotests/300
> @@ -23,7 +23,7 @@ import random
>  import re
>  from typing import Dict, List, Optional, Union
>  import iotests
> -import qemu
> +from qemu.machine import machine
>

I believe this gives my previous comment some extra strength.  I think
this shows that "qemu.machine" containing "machine" really means that
the first "machine" naming component doesn't really mean that.

"qemu.utils" is not perfect, but it's much more honest IMO.

Cheers,
- Cleber.

>  BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
>  
> @@ -454,7 +454,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 63d2ace93c..6574afd735 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 3fac20e929..b38969f61f 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.26.2
> 

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