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 <cr...@redhat.com> > --- > 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 ccarr...@redhat.com