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

[Cleber Rosa]

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 <address@hidden>
>> ---
>>  configure                                  |  1 +
>>  scripts/qemu.py => python/qemu/__init__.py | 11 ++++++-----
> 
> Well, we all know how difficult is to pick up names, but I would avoid
> use `python` here. This is the name of python bin itself. Is there any
> chance of a clash? I do not have a specific case right now, I'm just
> wondering if it can happen we should avoid.
> 

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  ]