Re: [Qemu-block] [RFC PATCH] Introduce Python module structure

[Caio Carrara]

Hi, Cleber.

On Tue, Nov 27, 2018 at 05:50:34AM -0500, Cleber Rosa wrote:
> This is a simple move of Python code that wraps common QEMU
> functionality, and are used by a number of different tests
> and scripts.
> 
> By treating that code as a real Python module, we can more easily,
> among other things:
>  * reuse more code
>  * apply a more consistent style
>  * add tests to that code
>  * generate documentation
> 
> Signed-off-by: Cleber Rosa <address@hidden>
> ---
>  configure                                  |  1 +
>  scripts/qemu.py => python/qemu/__init__.py | 11 ++++++-----

Well, we all know how difficult is to pick up names, but I would avoid
use `python` here. This is the name of python bin itself. Is there any
chance of a clash? I do not have a specific case right now, I'm just
wondering if it can happen we should avoid.

>  {scripts/qmp => python/qemu}/qmp.py        |  0
>  {scripts => python/qemu}/qtest.py          |  5 +++--
>  scripts/device-crash-test                  |  5 +++++
>  scripts/qmp/__init__.py                    |  0
>  tests/acceptance/avocado_qemu/__init__.py  |  9 +++++----
>  tests/migration/guestperf/engine.py        | 10 +++++++---
>  tests/qemu-iotests/iotests.py              |  8 ++++++--
>  tests/vm/basevm.py                         |  9 +++++++--
>  10 files changed, 40 insertions(+), 18 deletions(-)
>  rename scripts/qemu.py => python/qemu/__init__.py (98%)
>  rename {scripts/qmp => python/qemu}/qmp.py (100%)
>  rename {scripts => python/qemu}/qtest.py (97%)

What if we keep `qmp.py` and `qtest.py` directly under `/python`
directory?  It seems it can be more semantic regarding the subject of
each module. I'm not completely sure about `qmp.py`, but definetly I
think qtest should be under python directly.

>  delete mode 100644 scripts/qmp/__init__.py
> 
> diff --git a/configure b/configure
> index 0a3c6a72c3..2b64c51009 100755
[...]
> rename from scripts/qemu.py
> rename to python/qemu/__init__.py
[...]
> diff --git a/scripts/qmp/qmp.py b/python/qemu/qmp.py
> similarity index 100%
> rename from scripts/qmp/qmp.py
> rename to python/qemu/qmp.py
> diff --git a/scripts/qtest.py b/python/qemu/qtest.py
> similarity index 97%
> rename from scripts/qtest.py
> rename to python/qemu/qtest.py
> index adf1fe3f26..bff79cdd13 100644
> --- a/scripts/qtest.py
> +++ b/python/qemu/qtest.py
[...]
> diff --git a/scripts/device-crash-test b/scripts/device-crash-test
> index e93a7c0c84..c75ae0ecbc 100755
> --- a/scripts/device-crash-test
> +++ b/scripts/device-crash-test
> @@ -35,6 +35,11 @@ import random
>  import argparse
>  from itertools import chain
>  
> +THIS_DIR = os.path.dirname(os.path.abspath(__file__))
> +TOP_DIR = os.path.dirname(THIS_DIR)
> +PYTHON_MODULE_PATH = os.path.join(TOP_DIR, 'python')
> +sys.path.append(PYTHON_MODULE_PATH)

This sys.path handling to use the new QEMU Python module is not good. I
understand it can be a first step, but to expect everyone knows/do it to
use the module is a bad assumption because it's not intuitive and can
cause some confusion.

If we need something available from a Python script/module that is not
directly acessible from PYTHONPATH we should install it so Python can
turn it available. So, probably we need to think make `python/qemu` a
proper installable module.

> +
>  from qemu import QEMUMachine
>  
>  logger = logging.getLogger('device-crash-test')
> diff --git a/scripts/qmp/__init__.py b/scripts/qmp/__init__.py
> deleted file mode 100644
[...]
> diff --git a/tests/migration/guestperf/engine.py 
> b/tests/migration/guestperf/engine.py
> index 398e3f2706..73c9b66821 100644
> --- a/tests/migration/guestperf/engine.py
> +++ b/tests/migration/guestperf/engine.py
> @@ -24,13 +24,17 @@ import re
>  import sys
>  import time
>  
> -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', '..', 
> 'scripts'))
> -import qemu
> -import qmp.qmp
>  from guestperf.progress import Progress, ProgressStats
>  from guestperf.report import Report
>  from guestperf.timings import TimingRecord, Timings
>  
> +THIS_DIR = os.path.dirname(os.path.abspath(__file__))
> +TOP_DIR = os.path.dirname(os.path.dirname(os.path.dirname(THIS_DIR)))
> +PYTHON_MODULE_PATH = os.path.join(TOP_DIR, 'python')
> +sys.path.append(PYTHON_MODULE_PATH)
> +
> +import qemu

Since `qemu` is a common word here, I would rather import the members
directly than only the module. Just like you did in
`/tests/vm/basevm,py`

> +
>  
>  class Engine(object):
>  
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index d537538ba0..92fddd2a58 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -31,8 +31,12 @@ import logging
[...]
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -17,8 +17,6 @@ import sys
>  import logging
>  import time
>  import datetime
> -sys.path.append(os.path.join(os.path.dirname(__file__), "..", "..", 
> "scripts"))
> -from qemu import QEMUMachine, kvm_available
>  import subprocess
>  import hashlib
>  import optparse
> @@ -28,6 +26,13 @@ import shutil
>  import multiprocessing
>  import traceback
>  
> +THIS_DIR = os.path.dirname(os.path.abspath(__file__))
> +TOP_DIR = os.path.dirname(os.path.dirname(THIS_DIR))
> +PYTHON_MODULE_PATH = os.path.join(TOP_DIR, 'python')
> +sys.path.append(PYTHON_MODULE_PATH)
> +
> +from qemu import QEMUMachine, kvm_available
> +
>  SSH_KEY = open(os.path.join(os.path.dirname(__file__),
>                 "..", "keys", "id_rsa")).read()
>  SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__),
> -- 
> 2.19.1
> 

Thanks,
-- 
Caio Carrara
Software Engineer, Virt Team - Red Hat
address@hidden