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
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
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__),
--
2.19.1
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,
--
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,
>
--
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 ]
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 ]
--
Caio Carrara
Software Engineer, Virt Team - Red Hat
ccarrara@redhat.com
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
--
Eduardo
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
>
--
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 ]
© 2016 - 2024 Red Hat, Inc.