Introduce Python module structure

RFC PATCH In review Cleber Rosa 3 participants 6 messages Nov 27, 2018

Share & tools

Cleber Rosa

author

2018-11-27 10:50 UTC #0 lore raw

RFC PATCH 1/1 Introduce Python module structure

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 <[email protected]>

---
 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

configure +1 -0

index 0a3c6a72c3..2b64c51009 100755
@@ -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 \

python/qemu/__init__.py renamed from scripts/qemu.py +6 -5

similarity index 98%
rename from scripts/qemu.py
rename to python/qemu/__init__.py
index 6e3b0e6771..2ae5e1f632 100644
@@ -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"]

python/qemu/qmp.py renamed from scripts/qmp/qmp.py +0 -0

similarity index 100%
rename from scripts/qmp/qmp.py
rename to python/qemu/qmp.py

python/qemu/qtest.py renamed from scripts/qtest.py +3 -2

similarity index 97%
rename from scripts/qtest.py
rename to python/qemu/qtest.py
index adf1fe3f26..bff79cdd13 100644
@@ -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",

scripts/device-crash-test +5 -0

index e93a7c0c84..c75ae0ecbc 100755
@@ -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')

scripts/qmp/__init__.py deleted +0 -0

deleted file mode 100644
index e69de29bb2..0000000000

tests/acceptance/avocado_qemu/__init__.py +5 -4

index 1e54fd5932..4f3e426ebd 100644
@@ -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

tests/migration/guestperf/engine.py +7 -3

index 398e3f2706..73c9b66821 100644
@@ -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):

tests/qemu-iotests/iotests.py +6 -2

index d537538ba0..92fddd2a58 100644
@@ -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

tests/vm/basevm.py +7 -2

index 5caf77d6b8..464234a7b2 100755
@@ -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__),

Signature · 1 line

2.19.1

Caio Carrara

2018-11-27 19:00 UTC #1 lore raw

Hi, Cleber.

Quoted 11 lines · Cleber Rosa

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

Quoted 4 lines

Signed-off-by Cleber Rosa <[email protected]>

---
 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.

Quoted 4 lines

 {scripts/qmp => python/qemu}/qmp.py        |  0
 {scripts => python/qemu}/qtest.py          |  5 +++--
 scripts/device-crash-test                  |  5 +++++
 scripts/qmp/__init__.py                    |  0

Quoted 8 lines

 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.

Quoted 4 lines

 delete mode 100644 scripts/qmp/__init__.py

configure +0 -0

index 0a3c6a72c3..2b64c51009 100755

[...]

Quoted 2 lines

rename from scripts/qemu.py
rename to python/qemu/__init__.py

[...]

Quoted 11 lines

python/qemu/qmp.py renamed from scripts/qmp/qmp.py +0 -0
similarity index 100%
rename from scripts/qmp/qmp.py
rename to python/qemu/qmp.py
python/qemu/qtest.py renamed from scripts/qtest.py +0 -0
similarity index 97%
rename from scripts/qtest.py
rename to python/qemu/qtest.py
index adf1fe3f26..bff79cdd13 100644

[...]

scripts/device-crash-test import random · line 41

Quoted 12 lines

scripts/device-crash-test +4 -0

index e93a7c0c84..c75ae0ecbc 100755
@@ -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.

Quoted 6 lines

scripts/device-crash-test +1 -0
+
 from qemu import QEMUMachine
 
 logger = logging.getLogger('device-crash-test')
scripts/qmp/__init__.py deleted +0 -0
deleted file mode 100644

[...]

Quoted 11 lines

tests/migration/guestperf/engine.py +0 -3

index 398e3f2706..73c9b66821 100644
@@ -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

tests/migration/guestperf/engine.py import re · line 36

Quoted 10 lines

tests/migration/guestperf/engine.py +6 -0

 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

Quoted 9 lines

tests/migration/guestperf/engine.py +1 -0
+
 
 class Engine(object):
 
tests/qemu-iotests/iotests.py +0 -0
index d537538ba0..92fddd2a58 100644
@@ -31,8 +31,12 @@ import logging

[...]

Quoted 28 lines

tests/vm/basevm.py +7 -2

@@ -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__),

Signature · 1 line

2.19.1

Thanks,

Signature · 3 lines

Caio Carrara
Software Engineer, Virt Team - Red Hat
[email protected]

Eduardo Habkost

2018-11-27 19:49 UTC #2 lore raw

Quoted 19 lines · Caio Carrara

Hi, Cleber.
Quoted 11 lines · Cleber Rosa
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
Quoted 4 lines
Signed-off-by Cleber Rosa <[email protected]>
---
 configure                                  |  1 +
 scripts/qemu.py => python/qemu/__init__.py | 11 ++++++-----

Quoted 4 lines

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.

Quoted 14 lines

Quoted 13 lines

 {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%)

Quoted 4 lines

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.

Quoted 29 lines

Quoted 5 lines
 delete mode 100644 scripts/qmp/__init__.py
configure +0 -0
index 0a3c6a72c3..2b64c51009 100755
[...]
Quoted 2 lines
rename from scripts/qemu.py
rename to python/qemu/__init__.py
[...]
Quoted 11 lines
python/qemu/qmp.py renamed from scripts/qmp/qmp.py +0 -0
similarity index 100%
rename from scripts/qmp/qmp.py
rename to python/qemu/qmp.py
python/qemu/qtest.py renamed from scripts/qtest.py +0 -0
similarity index 97%
rename from scripts/qtest.py
rename to python/qemu/qtest.py
index adf1fe3f26..bff79cdd13 100644
[...]
Quoted 8 lines
scripts/device-crash-test +0 -0
index e93a7c0c84..c75ae0ecbc 100755
@@ -35,6 +35,11 @@ import random
 import argparse
 from itertools import chain
 

Quoted 4 lines

scripts/device-crash-test import random · line 41
Quoted 4 lines
scripts/device-crash-test +4 -0
+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'))

Quoted 10 lines

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

Quoted 23 lines

Quoted 7 lines

scripts/device-crash-test +1 -0
+
 from qemu import QEMUMachine
 
 logger = logging.getLogger('device-crash-test')
scripts/qmp/__init__.py deleted +0 -0
deleted file mode 100644

[...]

Quoted 15 lines

tests/migration/guestperf/engine.py +0 -3

index 398e3f2706..73c9b66821 100644
@@ -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

Quoted 10 lines

tests/migration/guestperf/engine.py import re · line 36
Quoted 6 lines
tests/migration/guestperf/engine.py +6 -0
+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.

Quoted 26 lines

Quoted 10 lines

tests/migration/guestperf/engine.py +1 -0
+
 
 class Engine(object):
 
tests/qemu-iotests/iotests.py +0 -0
index d537538ba0..92fddd2a58 100644
@@ -31,8 +31,12 @@ import logging

[...]

Quoted 15 lines

tests/vm/basevm.py +0 -2

@@ -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

Quoted 4 lines

tests/vm/basevm.py import shutil · line 32

Quoted 4 lines

tests/vm/basevm.py +4 -0

+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'))

Quoted 15 lines

Quoted 9 lines

tests/vm/basevm.py +3 -0

+
+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__),

Signature · 1 line

2.19.1

Thanks,

Signature · 3 lines

Caio Carrara
Software Engineer, Virt Team - Red Hat
[email protected]

Signature · 1 line

Eduardo

Cleber Rosa

author

2018-11-27 20:13 UTC #3 lore raw

Quoted 20 lines · Eduardo Habkost

Quoted 19 lines · Caio Carrara
Hi, Cleber.
Quoted 15 lines · Cleber Rosa
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 <[email protected]>
---
 configure                                  |  1 +
 scripts/qemu.py => python/qemu/__init__.py | 11 ++++++-----

Quoted 9 lines

Quoted 4 lines
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.

Quoted 14 lines

Quoted 13 lines

 {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%)

Quoted 9 lines

Quoted 4 lines
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.

Quoted 36 lines

Quoted 29 lines
Quoted 5 lines
 delete mode 100644 scripts/qmp/__init__.py
configure +0 -0
index 0a3c6a72c3..2b64c51009 100755
[...]
Quoted 2 lines
rename from scripts/qemu.py
rename to python/qemu/__init__.py
[...]
Quoted 11 lines
python/qemu/qmp.py renamed from scripts/qmp/qmp.py +0 -0
similarity index 100%
rename from scripts/qmp/qmp.py
rename to python/qemu/qmp.py
python/qemu/qtest.py renamed from scripts/qtest.py +0 -0
similarity index 97%
rename from scripts/qtest.py
rename to python/qemu/qtest.py
index adf1fe3f26..bff79cdd13 100644
[...]
Quoted 8 lines
scripts/device-crash-test +0 -0
index e93a7c0c84..c75ae0ecbc 100755
@@ -35,6 +35,11 @@ import random
 import argparse
 from itertools import chain
 
Quoted 4 lines
scripts/device-crash-test import random · line 41
Quoted 4 lines
scripts/device-crash-test +4 -0
+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:

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 11 lines

Quoted 10 lines
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.

Quoted 9 lines

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.

Quoted 34 lines

Quoted 23 lines
Quoted 7 lines
scripts/device-crash-test +1 -0
+
 from qemu import QEMUMachine
 
 logger = logging.getLogger('device-crash-test')
scripts/qmp/__init__.py deleted +0 -0
deleted file mode 100644
[...]
Quoted 15 lines
tests/migration/guestperf/engine.py +0 -3
index 398e3f2706..73c9b66821 100644
@@ -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
 
Quoted 10 lines
tests/migration/guestperf/engine.py import re · line 36
Quoted 6 lines
tests/migration/guestperf/engine.py +6 -0
+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

Quoted 3 lines

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.

Quoted 33 lines

Quoted 26 lines

Quoted 10 lines

tests/migration/guestperf/engine.py +1 -0
+
 
 class Engine(object):
 
tests/qemu-iotests/iotests.py +0 -0
index d537538ba0..92fddd2a58 100644
@@ -31,8 +31,12 @@ import logging

[...]

Quoted 15 lines

tests/vm/basevm.py +0 -2

@@ -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

Quoted 4 lines

tests/vm/basevm.py import shutil · line 32

Quoted 4 lines

tests/vm/basevm.py +4 -0

+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:

Quoted 3 lines

MY_DIR = os.path.dirname(__file__) sys.path.append(os.path.join(MY_DIR, '..', '..', 'python'))

Sure, no hard preferences on my side here.

- Cleber.

Quoted 16 lines

Quoted 15 lines

Quoted 9 lines

tests/vm/basevm.py +3 -0

+
+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__),

Signature · 1 line

2.19.1

Thanks,

Signature · 3 lines

Caio Carrara
Software Engineer, Virt Team - Red Hat
[email protected]

Signature · 4 lines

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  ]

Cleber Rosa

author

2018-11-27 20:02 UTC #4 lore raw

In reply to Caio Carrara

Quoted 19 lines · Caio Carrara

Hi, Cleber.
Quoted 11 lines · Cleber Rosa
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
Quoted 4 lines
Signed-off-by Cleber Rosa <[email protected]>
---
 configure                                  |  1 +
 scripts/qemu.py => python/qemu/__init__.py | 11 ++++++-----

Quoted 5 lines

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.

Quoted 13 lines

Quoted 4 lines

 {scripts/qmp => python/qemu}/qmp.py        |  0
 {scripts => python/qemu}/qtest.py          |  5 +++--
 scripts/device-crash-test                  |  5 +++++
 scripts/qmp/__init__.py                    |  0

Quoted 8 lines

 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%)

Quoted 5 lines

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.

Quoted 38 lines

Quoted 4 lines
 delete mode 100644 scripts/qmp/__init__.py
configure +0 -0
index 0a3c6a72c3..2b64c51009 100755
[...]
Quoted 2 lines
rename from scripts/qemu.py
rename to python/qemu/__init__.py
[...]
Quoted 11 lines
python/qemu/qmp.py renamed from scripts/qmp/qmp.py +0 -0
similarity index 100%
rename from scripts/qmp/qmp.py
rename to python/qemu/qmp.py
python/qemu/qtest.py renamed from scripts/qtest.py +0 -0
similarity index 97%
rename from scripts/qtest.py
rename to python/qemu/qtest.py
index adf1fe3f26..bff79cdd13 100644
[...]
scripts/device-crash-test import random · line 41
Quoted 12 lines
scripts/device-crash-test +4 -0
index e93a7c0c84..c75ae0ecbc 100755
@@ -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.

Quoted 5 lines

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.

Quoted 22 lines

Quoted 6 lines

scripts/device-crash-test +1 -0
+
 from qemu import QEMUMachine
 
 logger = logging.getLogger('device-crash-test')
scripts/qmp/__init__.py deleted +0 -0
deleted file mode 100644

[...]

Quoted 15 lines

tests/migration/guestperf/engine.py +0 -3

index 398e3f2706..73c9b66821 100644
@@ -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

Quoted 11 lines

tests/migration/guestperf/engine.py import re · line 36
Quoted 6 lines
tests/migration/guestperf/engine.py +6 -0
+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.

Quoted 41 lines

Quoted 9 lines

tests/migration/guestperf/engine.py +1 -0
+
 
 class Engine(object):
 
tests/qemu-iotests/iotests.py +0 -0
index d537538ba0..92fddd2a58 100644
@@ -31,8 +31,12 @@ import logging

[...]

Quoted 28 lines

tests/vm/basevm.py +7 -2

@@ -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__),

Signature · 1 line

2.19.1

Thanks,

Signature · 4 lines

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

2018-11-27 20:32 UTC #5 lore raw

Quoted 33 lines · Cleber Rosa

Quoted 19 lines · Caio Carrara
Hi, Cleber.
Quoted 11 lines · Cleber Rosa
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
Quoted 4 lines
Signed-off-by Cleber Rosa <[email protected]>
---
 configure                                  |  1 +
 scripts/qemu.py => python/qemu/__init__.py | 11 ++++++-----
Quoted 5 lines
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.

Quoted 4 lines

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.

Quoted 60 lines

Anyway, it'd be nice to think of alternatives here.
Quoted 13 lines
Quoted 4 lines
 {scripts/qmp => python/qemu}/qmp.py        |  0
 {scripts => python/qemu}/qtest.py          |  5 +++--
 scripts/device-crash-test                  |  5 +++++
 scripts/qmp/__init__.py                    |  0
Quoted 8 lines
 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%)
Quoted 5 lines
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

Quoted 8 lines

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.

Quoted 143 lines

Quoted 40 lines
Quoted 6 lines
 delete mode 100644 scripts/qmp/__init__.py
configure +0 -0
index 0a3c6a72c3..2b64c51009 100755
[...]
Quoted 2 lines
rename from scripts/qemu.py
rename to python/qemu/__init__.py
[...]
Quoted 11 lines
python/qemu/qmp.py renamed from scripts/qmp/qmp.py +0 -0
similarity index 100%
rename from scripts/qmp/qmp.py
rename to python/qemu/qmp.py
python/qemu/qtest.py renamed from scripts/qtest.py +0 -0
similarity index 97%
rename from scripts/qtest.py
rename to python/qemu/qtest.py
index adf1fe3f26..bff79cdd13 100644
[...]
scripts/device-crash-test import random · line 41
Quoted 12 lines
scripts/device-crash-test +4 -0
index e93a7c0c84..c75ae0ecbc 100755
@@ -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.
Quoted 5 lines
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.
Quoted 22 lines
Quoted 6 lines
scripts/device-crash-test +1 -0
+
 from qemu import QEMUMachine
 
 logger = logging.getLogger('device-crash-test')
scripts/qmp/__init__.py deleted +0 -0
deleted file mode 100644
[...]
Quoted 15 lines
tests/migration/guestperf/engine.py +0 -3
index 398e3f2706..73c9b66821 100644
@@ -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
 
Quoted 11 lines
tests/migration/guestperf/engine.py import re · line 36
Quoted 6 lines
tests/migration/guestperf/engine.py +6 -0
+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.
Quoted 41 lines
Quoted 9 lines
tests/migration/guestperf/engine.py +1 -0
+
 
 class Engine(object):
 
tests/qemu-iotests/iotests.py +0 -0
index d537538ba0..92fddd2a58 100644
@@ -31,8 +31,12 @@ import logging
[...]
Quoted 28 lines
tests/vm/basevm.py +7 -2
@@ -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__),
Signature · 1 line
2.19.1
Thanks,
Signature · 4 lines
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  ]

Signature · 3 lines

Caio Carrara
Software Engineer, Virt Team - Red Hat
[email protected]