Re: [PATCH v4 00/24] python: create installable package

[Cleber Rosa]

On Mon, Feb 15, 2021 at 04:32:29PM -0500, John Snow wrote:
> On 2/11/21 9:52 PM, Cleber Rosa wrote:
> > On Thu, Feb 11, 2021 at 01:58:32PM -0500, John Snow wrote:
> > > This series factors the python/qemu directory as an installable
> > > package. It does not yet actually change the mechanics of how any other
> > > python source in the tree actually consumes it (yet), beyond the import
> > > path. (some import statements change in a few places.)
> > > 
> > > git: https://gitlab.com/jsnow/qemu/-/commits/python-package-mk3
> > > CI: https://gitlab.com/jsnow/qemu/-/pipelines/254940717
> > > (New CI job: https://gitlab.com/jsnow/qemu/-/jobs/1024230604)
> > > 
> > > The primary motivation of this series is primarily to formalize our
> > > dependencies on mypy, flake8, isort, and pylint alongside versions that
> > > are known to work. It does this using the setup.cfg and setup.py
> > > files. It also adds explicitly pinned versions (using Pipfile.lock) of
> > > these dependencies that should behave in a repeatable and known way for
> > > developers and CI environments both. Lastly, it enables those CI checks
> > > such that we can enforce Python coding quality checks via the CI tests.
> > > 
> > > An auxiliary motivation is that this package is formatted in such a way
> > > that it COULD be uploaded to https://pypi.org/project/qemu and installed
> > > independently of qemu.git with `pip install qemu`, but that button
> > > remains *unpushed* and this series *will not* cause any such
> > > releases. We have time to debate finer points like API guarantees and
> > > versioning even after this series is merged.
> > > 
> > > Some other things this enables that might be of interest:
> > > 
> > > With the python tooling as a proper package, you can install this
> > > package in editable or production mode to a virtual environment, your
> > > local user environment, or your system packages. The primary benefit of
> > > this is to gain access to QMP tooling regardless of CWD, without needing
> > > to battle sys.path (and confounding other python analysis tools).
> > > 
> > > For example: when developing, you may go to qemu/python/ and run `make
> > > venv` followed by `pipenv shell` to activate a virtual environment that
> > > contains the qemu python packages. These packages will always reflect
> > > the current version of the source files in the tree. When you are
> > > finished, you can simply exit the shell (^d) to remove these packages
> > > from your python environment.
> > > 
> > > When not developing, you could install a version of this package to your
> > > environment outright to gain access to the QMP and QEMUMachine classes
> > > for lightweight scripting and testing by using pip: "pip install [--user] 
> > > ."
> > > 
> > > TESTING THIS SERIES:
> > > 
> > > First of all, nothing should change. Without any intervention,
> > > everything should behave exactly as it was before. The only new
> > > information here comes from how to interact with and run the linters
> > > that will be enforcing code quality standards in this subdirectory.
> > > 
> > > To test those, CD to qemu/python first, and then:
> > > 
> > > 1. Try "make venv && pipenv shell" to get a venv with the package
> > > installed to it in editable mode. Ctrl+d exits this venv shell. While in
> > > this shell, any python script that uses "from qemu.[qmp|machine] import
> > > ..." should work correctly regardless of where the script is, or what
> > > your CWD is.
> > > 
> > 
> > Ack here, works as expected.
> > 
> 
> That's a relief!
> 
> > > You will need Python 3.6 installed on your system to do this step. For
> > > Fedora: "dnf install python3.6" will do the trick.
> > > 
> > 
> > You may have explained this before, so forgive me asking again.  Why
> > is this dependent on a given Python version, and not a *minimum*
> > Python version? Are the checkers themselves susceptible to different
> > behavior depending on the Python version used?  If so, any hint on the
> > strategy for developing with say, Python 3.8, and then having issues
> > caught only on Python 3.6?
> > 
> 
> It's a good question, and I have struggled with communicating the language.
> So here are a few points:
> 
> (1) Users will not need Python 3.6 on their local systems to be able to run
> the linters; they will be able to run "make check" to run it under *any*
> environment -- granted they have the necessary packages. (mypy, pylint,
> pytest, flake8, and isort.) See note #2 below:
> 
> (2) `pip install [--user] .[devel]` will install the needed dependencies to
> the local environment/venv regardless of python version. These dependencies
> are not pinned, but do express a minimum viable version (in setup.cfg) for
> each tool that I tested rigorously.
> 
> (3) The CI system will target Python 3.6 specifically, because that is our
> minimum version. This will work to prevent future features from bleeding
> into the codebase, which was a notable problem when we were targeting
> simultaneous 2.7 and 3.x deployments. If we were going to only run against
> one version, 3.6 is the defensibly correct version to run against. If we
> want to run against more, I'd say let's not let perfection be the enemy of
> good enough.
> 
> (4) I considered it important to be able to run, as much as is possible, the
> *EXACT SAME* environment for tests as the CI runs. For this purpose, "make
> venv-check" uses pipenv to install a pinned set of dependencies (that might
> be lower than what you'd get otherwise), and uses explicitly Python 3.6.
> This is to make it repeatable and simple to run a test that's as close to
> what the CI runner will do as possible. This takes a lot of the thinking out
> of it.
> 
> 
> So, to answer you more directly:
> 
> - 3.6 is a *minimum* for "make check". (setup.cfg)
> - 3.6 is a *dependency* for "make venv-check". (Pipfile, Pipfile.lock)
>

OK, this answers my question.  It wasn't completely clear to me that
you took the care of using Pipfile.lock with one, and only one, Python
version (via "make venv-check").

> And, as far as known version problems:
> 
> - pylint 2.6.0 is not compatible with Python 3.9. They are working on it.
> 2.6.1-dev works alright, but isn't released yet. The linters may be
> unavailable to folks with 3.9-only environment.
> 
> Workarounds:
> 
> - Make your own venv using 3.7 or 3.8
> - Use the make venv-check entry point to use 3.6.
> - Give up and push it to gitlab and see if the CI test passes.
> 
> 
> And, finally, here's my maintainer testing strategy/promises:
> 
> - I endeavor to make sure this package is tested and working on any non-EOL
> Python version (3.6 - 3.9 at time of writing)
> - I endeavor to ensure that it is easy to test and lint these files on your
> local development system with minimum hassle
> - Given the volatility of compatibility between different versions of
> linters and python, however, the current *canonical check* is Python 3.6,
> using the explicitly pinned versions in the Pipfile.lock. There may be times
> (like right now) where the local linting test may not work with your version
> of Python.
> 
> > > 2. Try "make check" while still in the shell to run the Python linters
> > > using the venv built in the previous step. This will pull some packages
> > > from PyPI and run pytest, which will in turn execute mypy, flake8, isort
> > > and pylint with the correct arguments.
> > > 
> > 
> > Works as expected.  I'll provide more feedback at the specific patches.
> > 
> > > 3. Having exited the shell from above, try "make venv-check". This will
> > > create and update the venv if needed, then run 'make check' within the
> > > context of that shell. It should pass as long as the above did.
> > > 
> > 
> > If this makes into a documentation (or on a v5), you may just want to
> > tell users to run "deactivate" instead of exiting the shell completely.
> > 
> 
> It depends on how you entered the shell. Literally "pipenv shell" does
> create a new shell process that you should exit from. Since I suggested
> using the pipenv invocation, I match that suggestion by telling the user to
> exit (instead of deactivate).
> 
> You know too much about python for your own good!
>

May be the exact opposite! It's that I'm much more used to plain venvs
and those will not create a shell, and exiting will quit a session,
which is very annoying!

> > > 4. Still outside of the venv, you may try running "make check". This
> > > will not install anything, but unless you have the right Python
> > > dependencies installed, these tests may fail for you. You might try
> > > using "pip install --user .[devel]" to install the development packages
> > > needed to run the tests successfully to your local user's python
> > > environment. Once done, you will probably want to "pip uninstall qemu"
> > > to remove that package to avoid it interfering with other things.
> > > 
> > 
> > This is good info for completeness, but I wonder if "make check"
> > should exist at all.  If it's a requirement for "make check-venv", the
> > question becomes if it should be advertised.  Hint: I don't think it
> > should, it just adds some a bit of confusion IMO.
> > 
> 
> I think it's cleanly separated... If you understand much about how python
> virtual environments work.
> 
> - You can run the tests on any environment you want! or,
> - You can run those tests on a very, very specific environment that is
> controlled with a militaristic, iron fist.
> 
> current belief: People who are not developing python can just wait for the
> little orb to turn green in Gitlab-CI and not worry about running any
> particular test at all, actually. This current patch-set does not integrate
> these tests into "make check" at all, on purpose.
> 
> People who ARE developing this package (infrequently) will need to know
> which they want to run. My suggestion is to use "make venv-check" as the
> best predictor of the CI check, though it can be slow and clunky.
> 
> If you develop on this package a *lot*, and you regularly use a venv to
> develop, "make check" starts to make a lot more sense.
> 
> "make" with no arguments produces the help message;
> 
> ```
> python packaging help:
> 
> make venv:       Create pipenv's virtual environment.
>     NOTE: Requires Python 3.6 and pipenv.
>           Will download packages from PyPI.
>     HINT: On Fedora: 'sudo dnf install python36 pipenv'
> 
> make venv-check: run linters using pipenv's virtual environment.
> 
> make check:      run linters using the current environment.
>     Hint: Install deps with: 'pip install ".[devel]"'
> ```
> 
> ... Which, I suppose if you don't use python much, it might not make sense
> to you which environment you want to run the tests under. I can probably add
> a hint:
> 
> "HINT: Run this test if you're unsure which to run."
>

Yeah, the "make" help message is useful as it is, but I'd indeed include
this extra bit of info.

Thanks for the very detailed explanation!
- Cleber.

signature.asc
Description: PGP signature