Re: [PATCH 4/4] scripts/qmp: Fix QEMU Python scripts path

[John Snow]

On 4/30/20 1:04 AM, Markus Armbruster wrote:
> John Snow <address@hidden> writes:
> 
>> On 4/21/20 5:42 AM, Philippe Mathieu-Daudé wrote:
>>> QEMU Python scripts have been moved in commit 8f8fd9edba4 ("Introduce
>>> Python module structure"). Use the same sys.path modification used
>>> in the referenced commit to be able to use these scripts again.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>> ---
>>>  scripts/qmp/qmp      | 4 +++-
>>>  scripts/qmp/qom-fuse | 4 +++-
>>>  scripts/qmp/qom-get  | 4 +++-
>>>  scripts/qmp/qom-list | 4 +++-
>>>  scripts/qmp/qom-set  | 4 +++-
>>>  scripts/qmp/qom-tree | 4 +++-
>>>  6 files changed, 18 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp
>>> index 0625fc2aba..8e52e4a54d 100755
>>> --- a/scripts/qmp/qmp
>>> +++ b/scripts/qmp/qmp
>>> @@ -11,7 +11,9 @@
>>>  # See the COPYING file in the top-level directory.
>>>  
>>>  import sys, os
>>> -from qmp import QEMUMonitorProtocol
>>> +
>>> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 
>>> 'python'))
>>> +from qemu.qmp import QEMUMonitorProtocol
>>>  
>>
>> Try to avoid using sys.path hacks; they don't work in pylint or mypy and
>> it provides an active barrier to CQA work here.
>> (They also tend to be quite fragile.)
>>
>> We can discuss the right way to do this; one of those ways is to create
>> an installable package that we can install locally in a virtual environment.
>>
>> Another way is perhaps to set PYTHONPATH in the calling environment so
>> that standard "import" directives will work.
>>
>> Both ultimately involve changing the environment of the user to
>> accommodate the script.
> 
> For what it's worth, tests/Makefile.involve does the latter for
> tests/qapi-schema/test-qapi.py.  Simple enough, but makes manual
> invocation inconvenient.
> 
> Not necessary for scripts/qapi-gen.py, because its "import qmp.FOO"
> finds qmp right in scripts/qmp/.
> 

Yeah, I should be clear here: this is actually kind of a hard thing to
fix tree-wide right now.

What I didn't realize when I was reviewing this patch is that these
imports are *already broken* and these sys.path hacks actually make it
*work again*.

Under that premise, I rescind my feedback, and offer instead:

Reviewed-by: John Snow <address@hidden>

But we should look into how to fix the sys.path problems more long-term;
as part of the packaging work we should investigate this. I believe
similar problems are cropping up in iotests.

Sorry for the noise.

--js