Re: [PATCH v6 06/25] python: add directory structure README.rst files

[John Snow]

On 5/24/21 10:33 PM, Cleber Rosa wrote:
> On Wed, May 12, 2021 at 07:12:22PM -0400, John Snow wrote:
> > Add short readmes to python/, python/qemu/, python/qemu/machine,
> > python/qemu/qmp, and python/qemu/utils that explain the directory
> > hierarchy. These readmes are visible when browsing the source on
> > e.g. gitlab/github and are designed to help new developers/users quickly
> > make sense of the source tree.
> >
> > They are not designed for inclusion in a published manual.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   python/README.rst              | 41 ++++++++++++++++++++++++++++++++++
> >   python/qemu/README.rst         |  8 +++++++
> >   python/qemu/machine/README.rst |  9 ++++++++
> >   python/qemu/qmp/README.rst     |  9 ++++++++
> >   python/qemu/utils/README.rst   |  7 ++++++
> >   5 files changed, 74 insertions(+)
> >   create mode 100644 python/README.rst
> >   create mode 100644 python/qemu/README.rst
> >   create mode 100644 python/qemu/machine/README.rst
> >   create mode 100644 python/qemu/qmp/README.rst
> >   create mode 100644 python/qemu/utils/README.rst
> >
> > diff --git a/python/README.rst b/python/README.rst
> > new file mode 100644
> > index 00000000000..7a0dc5dff4a
> > --- /dev/null
> > +++ b/python/README.rst
> > @@ -0,0 +1,41 @@
> > +QEMU Python Tooling
> > +===================
> > +
> > +This directory houses Python tooling used by the QEMU project to build,
> > +configure, and test QEMU. It is organized by namespace (``qemu``), and
> > +then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
> > +
> > +``setup.py`` is used by ``pip`` to install this tooling to the current
> > +environment. ``setup.cfg`` provides the packaging configuration used by
> > +setup.py in a setuptools specific format. You will generally invoke it
>
> For consistency, ``setup.py`` here?  Also, I guess ``setuptools`` as it
> falls in the same category of ``pip``.

Kinda-sorta, but `pip` is a command line executable and setuptools 
isn't. Along those lines I'll fix setup.py but leave setuptools as-is.

> > +by doing one of the following:
> > +
> > +1. ``pip3 install .`` will install these packages to your current
> > +   environment. If you are inside a virtual environment, they will
> > +   install there. If you are not, it will attempt to install to the
> > +   global environment, which is not recommended.
>
> Maybe some **emphasis** on **not**?

Sure :)

> > +
> > +2. ``pip3 install --user .`` will install these packages to your user's
> > +   local python packages. If you are inside of a virtual environment,
> > +   this will fail.
> > +
>
> Maybe note that, if you are inside of a virtual environment, option #1
> will probably be what users doing "--user" in a venv actually want.

Yes. It's frequently annoying how this works, because it's hard to relay 
succinctly :)

I think at least newer versions of pip give you good warnings when you 
use --user for virtual environments at least.

> > +If you append the ``-e`` argument, pip will install in "editable" mode;
> > +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.
> > +
> > +See `Installing packages using pip and virtual environments
> > +<https://packaging.python.org/guides/installing-using-pip-and-virtual-environments/>`_
> > +for more information.
> > +
> > +
> > +Files in this directory
> > +-----------------------
> > +
> > +- ``qemu/`` Python package source directory.
> > +- ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
> > +- ``README.rst`` you are here!
> > +- ``VERSION`` contains the PEP-440 compliant version used to describe
> > +  this package; it is referenced by ``setup.cfg``.
> > +- ``setup.cfg`` houses setuptools package configuration.
> > +- ``setup.py`` is the setuptools installer used by pip; See above.
>
> Not only used by pip... but I understand the reason for limiting the
> amount of information given here.

Yes ... suggesting broadly that I don't really support using 
setuptools/setup.py alone to install the package, but instead expect and 
consider 'pip' to be the canonical/supported interface.

There are sometimes minor differences between how they handle things, so 
I wanted less emphasis on setuptools et al.

> > diff --git a/python/qemu/README.rst b/python/qemu/README.rst
> > new file mode 100644
> > index 00000000000..d04943f526c
> > --- /dev/null
> > +++ b/python/qemu/README.rst
> > @@ -0,0 +1,8 @@
> > +QEMU Python Namespace
> > +=====================
> > +
> > +This directory serves as the root of a `Python PEP 420 implicit
> > +namespace package <https://www.python.org/dev/peps/pep-0420/>`_.
> > +
> > +Each directory below is assumed to be an installable Python package that
> > +is available under the ``qemu.<package>`` namespace.
> > diff --git a/python/qemu/machine/README.rst b/python/qemu/machine/README.rst
> > new file mode 100644
> > index 00000000000..ac2b4fffb42
> > --- /dev/null
> > +++ b/python/qemu/machine/README.rst
> > @@ -0,0 +1,9 @@
> > +qemu.machine package
> > +====================
> > +
> > +This package provides core utilities used for testing and debugging
> > +QEMU. It is used by the iotests, vm tests, acceptance tests, and several
> > +other utilities in the ./scripts directory. It is not a fully-fledged
> > +SDK and it is subject to change at any time.
> > +
> > +See the documentation in ``__init__.py`` for more information.
> > diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
> > new file mode 100644
> > index 00000000000..c21951491cf
> > --- /dev/null
> > +++ b/python/qemu/qmp/README.rst
> > @@ -0,0 +1,9 @@
> > +qemu.qmp package
> > +================
> > +
> > +This package provides a library used for connecting to and communicating
> > +with QMP servers. It is used extensively by iotests, vm tests,
> > +acceptance tests, and other utilities in the ./scripts directory. It is
> > +not a fully-fledged SDK and is subject to change at any time.
> > +
> > +See the documentation in ``__init__.py`` for more information.
> > diff --git a/python/qemu/utils/README.rst b/python/qemu/utils/README.rst
> > new file mode 100644
> > index 00000000000..975fbf4d7de
> > --- /dev/null
> > +++ b/python/qemu/utils/README.rst
> > @@ -0,0 +1,7 @@
> > +qemu.utils package
> > +==================
> > +
> > +This package provides miscellaneous utilities used for testing and
> > +debugging QEMU. It is used primarily by the vm and acceptance tests.
> > +
> > +See the documentation in ``__init__.py`` for more information.
> > --
> > 2.30.2
>
> With the ``setup.py`` and ``setuptools`` for consistency sake
> mentioned in my first comment, all other comments are suggestions, so:

I took one of those two review comments, and acted on one of two of your 
suggestions.

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

So technically I didn't meet your criteria for taking this RB :) Let me 
know if I can still apply it:

diff --git a/python/README.rst b/python/README.rst
index 7a0dc5dff4a..38b0c83f321 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -7,17 +7,17 @@ then by package (e.g. ``qemu/machine``, ``qemu/qmp``, 
etc).

 ``setup.py`` is used by ``pip`` to install this tooling to the current
 environment. ``setup.cfg`` provides the packaging configuration used by
-setup.py in a setuptools specific format. You will generally invoke it
-by doing one of the following:
+``setup.py`` in a setuptools specific format. You will generally invoke
+it by doing one of the following:

 1. ``pip3 install .`` will install these packages to your current
    environment. If you are inside a virtual environment, they will
    install there. If you are not, it will attempt to install to the
-   global environment, which is not recommended.
+   global environment, which is **not recommended**.

 2. ``pip3 install --user .`` will install these packages to your user's
    local python packages. If you are inside of a virtual environment,
-   this will fail.
+   this will fail; you likely want the first invocation above.

 If you append the ``-e`` argument, pip will install in "editable" mode;
 which installs a version of the package that installs a forwarder