All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Snow <jsnow@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Cleber Rosa <crosa@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	qemu-block@nongnu.org, Max Reitz <mreitz@redhat.com>
Subject: Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint
Date: Wed, 4 Mar 2020 13:59:09 -0500	[thread overview]
Message-ID: <025df334-9d9f-bee9-4f6a-28894137c6ba@redhat.com> (raw)
In-Reply-To: <76a0567c-9f96-7b55-8a25-ee0933d796a0@redhat.com>



On 3/3/20 7:02 PM, Philippe Mathieu-Daudé wrote:
> On 3/3/20 8:57 PM, John Snow wrote:
>> On 2/27/20 9:14 AM, Philippe Mathieu-Daudé wrote:
>>> On 2/27/20 1:06 AM, John Snow wrote:
>>>> The right way to solve this is to come up with a virtual environment
>>>> infrastructure that sets all the paths correctly, and/or to create
>>>> installable python modules that can be imported normally.
>>>>
>>>> That's hard, so just silence this error for now.
>>>
>>> I'm tempted to NAck this and require an "installable python module"...
>>>
>>> Let's discuss why it is that hard!
>>>
>>
>> I've been tricked into this before. It's not work I am interested in
>> doing right now; it's WAY beyond the scope of what I am doing here.
>>
>> It involves properly factoring all of our python code, deciding which
>> portions are meant to be installed separately from QEMU itself, coming
>> up with a versioning scheme, packaging code, and moving code around in
>> many places.
>>
>> Then it involves coming up with tooling and infrastructure for creating
>> virtual environments, installing the right packages to it, and using it
>> to run our python tests.
>>
>> No, that's way too invasive. I'm not doing it and I will scream loudly
>> if you make me.
>>
>> A less invasive hack involves setting the python import path in a
>> consolidated spot so that python knows where it can import from. This
>> works, but might break the ability to run such tests as one-offs without
>> executing the environment setup.
>>
>> Again, not work I care to do right now and so I won't. The benefit of
>> these patches is to provide some minimum viable CI CQA for Python where
>> we had none before, NOT fix every possible Python problem in one shot.
> 
> OK I guess we misunderstood each other :)
> 
> I didn't understood your comment as personal to you for this patch, but
> generic. It makes sense it is not your priority and it is obvious this
> task will take a single developer a lot of time resources. I am
> certainly NOT asking you to do it.
> 
> My question was rather community-oriented.
> (Cc'ing Eduardo because we talked about this after the last KVM forum).
> 

OK, *that* is a fair question -- but you did threaten to reject the
patch, implying it was in-scope for this series. In no uncertain terms,
it is not.


HOWEVER ... since we're here, let's discuss python packaging.

This is the iotests code base. It is not intended to be packaged as
such, it's intended to be run in-tree or in the build folder. It will
rely on files being in specific paths and so on.

(I don't remember if iotests is designed to detect features at runtime
or if it still uses the build options to skip tests. Cleber would know
better, as he's battled this distinction in the past.)

Regardless, I think iotests depends on the qemu version AND the build
configuration, so iotests shouldn't be independently packaged or
packagable. It should remain "a collection of scripts" instead.

In this paradigm, python can only find descendant files. Importing from
subfolders using relative paths.

In this case, we're trying to climb *up* the path tree, which causes
grief. One often quoted solution is to use syspath hacking to
dynamically, at runtime, add parent folders to the PYTHONPATH.

Most static analysis tooling I have used to date is unable to cope with
this workaround -- hence pylint's failure to find the package being
imported.

(If we progress to using mypy, mypy will also be unable to cope with
this statement. It becomes clear we need to create a well defined
environment so tools know where they are allowed to look for sources.)

However, the thing we are trying to import is something that arguably
can be turned into an installable package as a light Python SDK for
interfacing with QEMU. That's worth doing. This code does not (AFAIK)
depend on any build configuration. That's good!

So there are a few approaches, and they're not mutually exclusive.
Option 1 *can* be a stopgap to option 2.

1. Use the iotest runners to set the PYTHONPATH to include the
python/qemu source tree directory. The imports will fail outside of the
test runner now, but we won't have to do any syspath hacking. This is
likely not important because the tests already require a good number of
environment variables set to function properly anyway.

This is a good workaround, but still outside of the scope for this series.

Notably, any pylint gating will need to occur under this specialized
environment (with PYTHONPATH set.) This might be more of a burden at times.

2. Convert the "python/qemu" folder into an installable "qemu" module.
The version of this package can track the `git describe` version well.

Once it is installable, there are several ways to use it:

A: Install it using pip/setuptools to the system. (pip3 install .)

B: Install it to the user's environment (pip3 install --user .)

C: Create a virtual environment (using whichever virtual environment
tool of your choice) and then having entered the venv, installing it:
(pip3 install .)

D: Any of the options above, but in editable/develop mode. In this mode,
the package is installed using symlinks that point back to the source
code files. This allows you to edit the package as you work without
having to reinstall after each change.

You'll use something like this:

pip3 install [--user] -e .
python3 setup.py develop [--user]

E: Continue just setting PYTHONPATH to point to the package when needed.
This is essentially editable/develop mode but without invoking setuptools.



At the moment, I don't believe we use any virtualenv configurations or
declare our dependencies natively to python tooling -- we rely on
configure to check for distribution packages.

We may want to get serious about python and begin using
virtualenvironments in places where it makes sense (instead of
PYTHONPATH hacking), but they are expensive to build so they should be
persistent so they can be re-engaged quickly to run tests and other scripts.

(A development venv might include our qemu package, the sphinx packages
locked to the correct versions, etc. We have not had major problems here
yet, but as our usage of python expands for document building and we
revamp QAPI, it may become more important to start fashioning these
bespoke environments to create more reproducible builds across different
environments.)

--js



  reply	other threads:[~2020-03-04 19:00 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-27  0:06 [PATCH v6 0/9] iotests: use python logging John Snow
2020-02-27  0:06 ` [PATCH v6 1/9] iotests: do a light delinting John Snow
2020-02-27 12:59   ` Max Reitz
2020-03-03 21:25     ` John Snow
2020-03-04 11:12       ` Kevin Wolf
2020-03-04 18:35         ` John Snow
2020-02-27  0:06 ` [PATCH v6 2/9] iotests: add script_initialize John Snow
2020-02-27 13:47   ` Max Reitz
2020-03-03 21:12     ` John Snow
2020-02-27  0:06 ` [PATCH v6 3/9] iotests: replace mutable list default args John Snow
2020-02-27 13:50   ` Max Reitz
2020-02-27  0:06 ` [PATCH v6 4/9] iotest 258: use script_main John Snow
2020-02-27 13:55   ` Max Reitz
2020-02-27 14:10   ` Philippe Mathieu-Daudé
2020-02-27  0:06 ` [PATCH v6 5/9] iotests: Mark verify functions as private John Snow
2020-02-27 13:59   ` Max Reitz
2020-02-27  0:06 ` [PATCH v6 6/9] iotests: use python logging for iotests.log() John Snow
2020-02-27 14:21   ` Max Reitz
2020-03-03 20:00     ` John Snow
2020-02-27  0:06 ` [PATCH v6 7/9] iotests: ignore import warnings from pylint John Snow
2020-02-27 14:14   ` Philippe Mathieu-Daudé
2020-03-03 19:57     ` John Snow
2020-03-04  0:02       ` Philippe Mathieu-Daudé
2020-03-04 18:59         ` John Snow [this message]
2020-02-27  0:06 ` [PATCH v6 8/9] iotests: don't use 'format' for drive_add John Snow
2020-02-27 14:12   ` Philippe Mathieu-Daudé
2020-02-27 14:26   ` Max Reitz
2020-02-27  0:06 ` [PATCH v6 9/9] iotests: add pylintrc file John Snow
2020-02-27  1:57   ` Eric Blake
2020-02-27 14:11   ` Philippe Mathieu-Daudé
2020-03-03 19:52     ` John Snow
2020-03-04  7:22   ` Markus Armbruster
2020-03-04 19:17     ` John Snow
2020-03-05  5:49       ` Markus Armbruster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=025df334-9d9f-bee9-4f6a-28894137c6ba@redhat.com \
    --to=jsnow@redhat.com \
    --cc=crosa@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=philmd@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.