Re: [PATCH v6 20/25] python: add devel package requirements to setuptools

[Cleber Rosa <crosa@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4996 bytes --]

On Tue, May 25, 2021 at 01:43:42PM -0400, John Snow wrote:
> On 5/25/21 12:13 PM, Cleber Rosa wrote:
> > On Wed, May 12, 2021 at 07:12:36PM -0400, John Snow wrote:
> > > setuptools doesn't have a formal understanding of development requires,
> > > but it has an optional feataures section. Fine; add a "devel" feature
> > > and add the requirements to it.
> > > 
> > > To avoid duplication, we can modify pipenv to install qemu[devel]
> > > instead. This enables us to run invocations like "pip install -e
> > > .[devel]" and test the package on bleeding-edge packages beyond those
> > > specified in Pipfile.lock.
> > > 
> > > Importantly, this also allows us to install the qemu development
> > > packages in a non-networked mode: `pip3 install --no-index -e .[devel]`
> > > will now fail if the proper development dependencies are not already
> > > met. This can be useful for automated build scripts where fetching
> > > network packages may be undesirable.
> > > 
> > 
> > This is a fairly exotic feature of setuptools, with very very few
> > packages that I know about using it.  With most users (I believe)
> > relying on pipenv to get the exact packages, the setuptools/pip use
> > case may fall into obscurity IMO.
> > 
> 
> Fair enough.
> 
> The intent is:
> 
> - Pipenv is more for CI, to deploy a consistent set of frozen packages that
> are known to behave in an extremely stable manner. My hope is to avoid
> breaking changes introduced unknowingly by pylint et al.
> 
> - pip install qemu[devel] is intended more for external/normal use by
> developers. It grabs the latest and greatest and it may indeed break as
> dependencies change beyond my awareness.
> 
> 
> Some packages like aiohttp use that optional dependency feature to install
> optional modules -- `pip install aiohttp[speedups]` installs optional
> dependencies that allow that module to work much faster, but aren't
> required.
> 
> Since these linting tools aren't *required* just to *use* the package, I am
> doing users a courtesy by listing them as optional. That way, they aren't
> pulled in when using "pip install qemu", and if I have to pin on specific
> sub-versions etc, it won't include conflict dependencies for people using
> other projects that DO declare a hard requirement on those packages.
> 
> I can amend the PACKAGE.rst file to mention this usage, though it's only
> useful for folks developing the package.
> 
> (Still, part of the ploy here is to attract outside help on developing the
> QEMU SDK, pull requests welcome etc, so it's worth a documentation blurb for
> now.)
>

Yep, I agree with your reasoning here.  I just felt like an extra bit
of documentation would do the trick.

> > So my suggestion is: consider better exposing the fact that this is
> > available (a documentation section perhaps).
> > 
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >   python/Pipfile      |  5 +----
> > >   python/Pipfile.lock | 14 +++++++++-----
> > >   python/setup.cfg    |  9 +++++++++
> > >   3 files changed, 19 insertions(+), 9 deletions(-)
> > > 
> > 
> > Either way,
> > 
> > Reviewed-by: Cleber Rosa <crosa@redhat.com>
> > 
> 
> Thanks! I am taking your R-B and I have applied the following diff.
> 
> Note that the PACKAGE.rst blurb references qemu[devel] instead because the
> PACKAGE.rst file is what is displayed theoretically on PyPI. That exact
> invocation will fail currently, because it's not on PyPI yet.
> 
> A little weird, but I *think* it's correct.
> 
> 
> diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
> index 1bbfe1b58e2..05ea7789fc1 100644
> --- a/python/PACKAGE.rst
> +++ b/python/PACKAGE.rst
> @@ -31,3 +31,7 @@ official `GitLab mirror
> <https://gitlab.com/qemu-project/qemu>`_.
>  Please report bugs on the `QEMU issue tracker
>  <https://gitlab.com/qemu-project/qemu/-/issues>`_ and tag ``@jsnow`` in
>  the report.
> +
> +Optional packages necessary for running code quality analysis for this
> +package can be installed with the optional dependency group "devel":
> +``pip install qemu[devel]``.
> diff --git a/python/README.rst b/python/README.rst
> index bf9bbca979a..954870973d0 100644
> --- a/python/README.rst
> +++ b/python/README.rst
> @@ -24,6 +24,10 @@ which installs a version of the package that installs a
> forwarder
>  pointing to these files, such that the package always reflects the
>  latest version in your git tree.
> 
> +Installing ".[devel]" instead of "." will additionally pull in required
> +packages for testing this package. They are not runtime requirements,
> +and are not needed to simply use these libraries.
> +
>  See `Installing packages using pip and virtual environments
> 
> <https://packaging.python.org/guides/installing-using-pip-and-virtual-environments/>`_
>  for more information.

Looks great to me.  And, let me be clear about it:

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]