On 4/30/20 1:04 AM, Markus Armbruster wrote:
> John Snow <js...@redhat.com> 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é <f4...@amsat.org>
>>> ---
>>> 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 <js...@redhat.com>
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
