| Message ID | 20181127105034.21045-1-crosa@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] Introduce Python module structure | expand |
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 <crosa@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,
On Tue, Nov 27, 2018 at 05:00:07PM -0200, Caio Carrara wrote: > 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 <crosa@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. I'm don't see how it would be a problem: this won't be the name of a Python module, but just a directory to appear on sys.path. > > > {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. I'd prefer to have everything inside a "qemu" top-level package to avoid module namespace conflicts with other software. > > > 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) I would prefer to use pathlib: from pathlib import Path sys.path.append(str(Path(__file__).parent / '..' / 'python')) but pathlib is not available on Python 2, so we could at least avoid using dirname(dirname(abspath(...))) and use '..' instead: MY_DIR = os.path.dirname(__file__) sys.path.append(os.path.join(MY_DIR, '..', 'python')) > > 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. Avoiding the sys.path trick would be nice, but how can we do that while keeping scripts still working out of the box? In other words, how can we keep this working: $ git clone https://.../qemu.git $ cd qemu $ ./scripts/qmp-shell /path/to/qmp-socket > > > + > > 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` I don't disagree, but I would prefer to do this in a separate patch, to make review easier. > > > + > > > > 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) Same as above, I would prefer: MY_DIR = os.path.dirname(__file__) sys.path.append(os.path.join(MY_DIR, '..', '..', 'python')) > > + > > +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 > ccarrara@redhat.com
On 11/27/18 2:00 PM, Caio Carrara wrote: > 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 <crosa@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. > The point of the "python" directory, is that it's not supposed to be the *module* name, but a holder for the Python module, which is "qemu". Given that "python is not a module", has no __init__.py, I don't think it'll ever clash. The reason for picking the name is that it makes it obvious, within the QEMU tree, that this is Python stuff. I already have some tests, to test the "Python stuff", to put it under "python", probably in "python/tests". Anyway, it'd be nice to think of alternatives here. >> {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. > I still think having a uniform starting point for the module, such as "qemu" is a good practice for all QEMU Python code. Having independent first level modules will make it harder to understand where the code is coming from. For instance, we have the option of coming up with a structure that would allow: import os import struct import qmp import urllib And the origin of "qmp" would go unnoticed. Alternatively: import os import struct import urllib from qemu import qmp Is much clearer. Now, with regards to "qmp", it's pretty much standalone, uses only Python libraries, while "qemu" consumes "qmp" functionality. I believe having "qemu.qmp" is appropriate here. Now, for "qtest", you have a point, it uses QEMUMachine, and its content is very similar to what's available under "qemu" itself (that is, QEMUMachine itself). IMO, it would make sense to just have the QEMUQtestProtocol and QEMUQtestMachine at the "qemu" level at this point, that is, fold their content into "qemu/__init__.py". The API usage would then become: from qemu import QEMUQTestMachine Similar to: from qemu import QEMUMachine I refrained from changing too much at the initial RFC time, though, exactly to make less biased discussions. >> 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. > It's something that is confusing for mode experienced Python developers, but I have some experience with way too many people not understanding at all why an extra step (the module installation), was necessary for using Python code from within a source tree. We faced a LOT of questions, for a long time, when we did the switch in Avocado itself. To summarize, you're right about the end goal, and I hope we'll get there and remove this boiler plate code. But, I think we need an intermediary step. >> + >> 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` > Right, I avoided changing too much of the code using the modules, but I do agree with you here. Thanks for the feedback! - Cleber. >> + >> >> 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, >
On 11/27/18 2:49 PM, Eduardo Habkost wrote: > On Tue, Nov 27, 2018 at 05:00:07PM -0200, Caio Carrara wrote: >> 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 <crosa@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. > > I'm don't see how it would be a problem: this won't be the name > of a Python module, but just a directory to appear on sys.path. > > Right, that's my understanding. >> >>> {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. > > I'd prefer to have everything inside a "qemu" top-level package > to avoid module namespace conflicts with other software. > > Agreed. See my other reply on this thread. >> >>> 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) > > I would prefer to use pathlib: > > from pathlib import Path > sys.path.append(str(Path(__file__).parent / '..' / 'python')) > > but pathlib is not available on Python 2, so we could at least > avoid using dirname(dirname(abspath(...))) and use '..' instead: Right, I mostly have to live with Python compatible code, so pathlib is something that is constantly under my radar. > > MY_DIR = os.path.dirname(__file__) > sys.path.append(os.path.join(MY_DIR, '..', 'python')) > > Yep, I have no problem with alternative ways of getting to the Python module directory. What I've found though on many situations, though, is that having easy access to the top most directory is helpful in a lot of Python modules. For instance, both "tests/acceptance/avocado_qemu/__init__.py" and "tests/vm/basevm.py" are relevant examples. Another experience I have that confirms that is the Avocado selfests: $ cd avocado/selftests $ git grep BASEDIR | wc -l 96 But this is a really minor thing at this point. >> >> 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. > > Avoiding the sys.path trick would be nice, but how can we do that > while keeping scripts still working out of the box? In other > words, how can we keep this working: > > $ git clone https://.../qemu.git > $ cd qemu > $ ./scripts/qmp-shell /path/to/qmp-socket > > That's the point, it'd require an extra step. For Avocado itself, we have "make develop" that does a Python module "installation" (a fake one, for development purposes) in the user's environment. I'd avoid adding this extra step at this time to the QEMU workflow. >> >>> + >>> 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` > > I don't disagree, but I would prefer to do this in a separate > patch, to make review easier. > Me too. See the code snippet examples on my other reply. >> >>> + >>> >>> 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) > > Same as above, I would prefer: > > MY_DIR = os.path.dirname(__file__) > sys.path.append(os.path.join(MY_DIR, '..', '..', 'python')) > Sure, no hard preferences on my side here. - Cleber. >>> + >>> +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 >> ccarrara@redhat.com >
On Tue, Nov 27, 2018 at 03:02:03PM -0500, Cleber Rosa wrote: > > > On 11/27/18 2:00 PM, Caio Carrara wrote: > > 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 <crosa@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. > > > > The point of the "python" directory, is that it's not supposed to be the > *module* name, but a holder for the Python module, which is "qemu". > Given that "python is not a module", has no __init__.py, I don't think > it'll ever clash. > > The reason for picking the name is that it makes it obvious, within the > QEMU tree, that this is Python stuff. I already have some tests, to > test the "Python stuff", to put it under "python", probably in > "python/tests". Ok, now I got your point. No problem. > > Anyway, it'd be nice to think of alternatives here. > > >> {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. > > > > I still think having a uniform starting point for the module, such as > "qemu" is a good practice for all QEMU Python code. Having independent > first level modules will make it harder to understand where the code is > coming from. For instance, we have the option of coming up with a > structure that would allow: > > > import os > import struct > import qmp > import urllib > > > And the origin of "qmp" would go unnoticed. Alternatively: > > > import os > import struct > import urllib > from qemu import qmp > > > Is much clearer. Now, with regards to "qmp", it's pretty much > standalone, uses only Python libraries, while "qemu" consumes "qmp" > functionality. I believe having "qemu.qmp" is appropriate here. > > Now, for "qtest", you have a point, it uses QEMUMachine, and its content > is very similar to what's available under "qemu" itself (that is, > QEMUMachine itself). IMO, it would make sense to just have the > QEMUQtestProtocol and QEMUQtestMachine at the "qemu" level at this > point, that is, fold their content into "qemu/__init__.py". > > The API usage would then become: > > > from qemu import QEMUQTestMachine > > > Similar to: > > > from qemu import QEMUMachine > > > I refrained from changing too much at the initial RFC time, though, > exactly to make less biased discussions. Well, I agree with you here. For a first step we shouldn't be changing too much the structure. I'm ok moving forward this way for now. > > > >> 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. > > > > It's something that is confusing for mode experienced Python developers, > but I have some experience with way too many people not understanding at > all why an extra step (the module installation), was necessary for using > Python code from within a source tree. We faced a LOT of questions, for > a long time, when we did the switch in Avocado itself. > > To summarize, you're right about the end goal, and I hope we'll get > there and remove this boiler plate code. But, I think we need an > intermediary step. > > >> + > >> 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` > > > > Right, I avoided changing too much of the code using the modules, but I > do agree with you here. > > Thanks for the feedback! > - Cleber. > > >> + > >> > >> 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, > > > > -- > Cleber Rosa > [ Sr Software Engineer - Virtualization Team - Red Hat ] > [ Avocado Test Framework - avocado-framework.github.io ] > [ 7ABB 96EB 8B46 B94D 5E0F E9BB 657E 8D33 A5F2 09F3 ]
diff --git a/configure b/configure index 0a3c6a72c3..2b64c51009 100755 --- a/configure +++ b/configure @@ -7510,6 +7510,7 @@ LINKS="$LINKS pc-bios/qemu-icon.bmp" LINKS="$LINKS .gdbinit scripts" # scripts needed by relative path in .gdbinit LINKS="$LINKS tests/acceptance tests/data" LINKS="$LINKS tests/qemu-iotests/check" +LINKS="$LINKS python" for bios_file in \ $source_path/pc-bios/*.bin \ $source_path/pc-bios/*.lid \ diff --git a/scripts/qemu.py b/python/qemu/__init__.py similarity index 98% rename from scripts/qemu.py rename to python/qemu/__init__.py index 6e3b0e6771..2ae5e1f632 100644 --- a/scripts/qemu.py +++ b/python/qemu/__init__.py @@ -16,12 +16,13 @@ import errno import logging import os import subprocess -import qmp.qmp import re import shutil import socket import tempfile +from . import qmp + LOG = logging.getLogger(__name__) @@ -58,7 +59,7 @@ class QEMUMachineAddDeviceError(QEMUMachineError): failures reported by the QEMU binary itself. """ -class MonitorResponseError(qmp.qmp.QMPError): +class MonitorResponseError(qmp.QMPError): """ Represents erroneous QMP monitor reply """ @@ -259,8 +260,8 @@ class QEMUMachine(object): self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log") self._qemu_log_file = open(self._qemu_log_path, 'wb') - self._qmp = qmp.qmp.QEMUMonitorProtocol(self._vm_monitor, - server=True) + self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, + server=True) def _post_launch(self): self._qmp.accept() @@ -376,7 +377,7 @@ class QEMUMachine(object): """ reply = self.qmp(cmd, conv_keys, **args) if reply is None: - raise qmp.qmp.QMPError("Monitor is closed") + raise qmp.QMPError("Monitor is closed") if "error" in reply: raise MonitorResponseError(reply) return reply["return"] 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 @@ -13,7 +13,8 @@ import socket import os -import qemu + +from . import QEMUMachine class QEMUQtestProtocol(object): @@ -73,7 +74,7 @@ class QEMUQtestProtocol(object): self._sock.settimeout(timeout) -class QEMUQtestMachine(qemu.QEMUMachine): +class QEMUQtestMachine(QEMUMachine): '''A QEMU VM''' def __init__(self, binary, args=None, name=None, test_dir="/var/tmp", 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) + 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 index e69de29bb2..0000000000 diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py index 1e54fd5932..4f3e426ebd 100644 --- a/tests/acceptance/avocado_qemu/__init__.py +++ b/tests/acceptance/avocado_qemu/__init__.py @@ -13,9 +13,10 @@ import sys import avocado -SRC_ROOT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(__file__))) -SRC_ROOT_DIR = os.path.abspath(os.path.dirname(SRC_ROOT_DIR)) -sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts')) +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) from qemu import QEMUMachine @@ -34,7 +35,7 @@ def pick_default_qemu_bin(): if is_readable_executable_file(qemu_bin_relative_path): return qemu_bin_relative_path - qemu_bin_from_src_dir_path = os.path.join(SRC_ROOT_DIR, + qemu_bin_from_src_dir_path = os.path.join(TOP_DIR, qemu_bin_relative_path) if is_readable_executable_file(qemu_bin_from_src_dir_path): return qemu_bin_from_src_dir_path 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 + 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 import atexit import io -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts')) -import qtest +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 qtest # This will not work if arguments contain spaces but is necessary if we diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 5caf77d6b8..464234a7b2 100755 --- 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__),
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 <crosa@redhat.com> --- configure | 1 + scripts/qemu.py => python/qemu/__init__.py | 11 ++++++----- {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%) delete mode 100644 scripts/qmp/__init__.py