qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: John Snow <jsnow@redhat.com>
To: Cleber Rosa <crosa@redhat.com>
Cc: "Fam Zheng" <fam@euphon.net>, "Kevin Wolf" <kwolf@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	qemu-block@nongnu.org,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	qemu-devel@nongnu.org,
	"Wainer dos Santos Moschetta" <wainersm@redhat.com>,
	"Max Reitz" <mreitz@redhat.com>,
	"Willian Rampazzo" <wrampazz@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Beraldo Leal" <bleal@redhat.com>
Subject: Re: [PATCH v4 07/24] python: add directory structure README.rst files
Date: Thu, 18 Feb 2021 12:45:52 -0500	[thread overview]
Message-ID: <69a11981-8db5-a47e-74a9-8c47fac16270@redhat.com> (raw)
In-Reply-To: <YCyDwYYhpYEz2onl@localhost.localdomain>

On 2/16/21 9:47 PM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:39PM -0500, 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   |  9 ++++++++
>>   5 files changed, 76 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
>>
> 
> It's not often I complain about too much documentation, but I honestly
> think this will not scale.  I understand that the intention is to help
> users browsing through the directory structure it has a huge potential
> for bit rot.
> 

Always a nice problem to have too much instead of too little. ;)

> The READMEs at the first two levels seem OK, but the ones at the
> subpackages level will be a maintainance nightmare.  I would *very
> much* move those (subpackage ones) documentation into the Python file
> themselves.
> 

I don't think there's much, if anything, to move into those files. There 
are some details about how the module relates to the rest of the QEMU 
tree, but I think those details aren't appropriate to have "in" the 
python package itself -- they won't apply to whatever environment they 
get installed in.

>> diff --git a/python/README.rst b/python/README.rst
>> new file mode 100644
>> index 00000000000..6a14b99e104
>> --- /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 (``qemu/machine``, ``qemu/qmp``).
>> +
>> +``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:
>> +
>> +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.
>> +
>> +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.
>> +
>> +If you amend 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.

Top-level doc seems fine to you though? I expect this directory to be 
reasonably low-traffic in terms of file additions/removals. If/when we 
start using sphinx to generate documentation for the qemu packages, we 
can probably link to e.g. readthedocs from here.

Not ready yet, of course.

>> 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.

This one is short and sweet. Not at risk of rotting. See below for a 
suggestion that might appease both of us WRT per-subpackage READMEs.

>> 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.

This is actually *pretty* brief, and I didn't intend for it to be 
exhaustively complete. I am trying to say "Please look at the python 
docstrings in __init__.py for real details!" because I was also worried 
about bitrot and duplicating docs.

Though, sure, it does duplicate at least some of the basic information. 
We have three-ish choices:

(1) Eh, fine, leave it here.
(2) Update it to be a simple pointer to __init__.py only; i.e. just the 
title heading and the "Please see __init__.py for details" hint.
(3) Just remove it. It should be common knowledge to investigate 
__init__.py for package-level documentation.
   (3b) Remove it, but update the PEP420-level document to contain an
        extra blurb that says "See each subpackage's __init__.py for more
        information."

Your preference?

>> 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.

Same story as above. This is a pretty brief explanation that explains 
its role in the QEMU source tree, but doesn't explain much about the 
package itself. The same three options as above make sense to me here.

>> diff --git a/python/qemu/utils/README.rst b/python/qemu/utils/README.rst
>> new file mode 100644
>> index 00000000000..4b33c1f27e1
>> --- /dev/null
>> +++ b/python/qemu/utils/README.rst
>> @@ -0,0 +1,9 @@
>> +qemu.utils package
>> +==================
>> +
>> +This package provides misc utilities used for testing and debugging
>> +QEMU. It is used most directly by the qemu.machine package, but has some
>> +uses by the vm and acceptance tests for determining accelerator support.
>> +
>> +See the documentation in ``__init__.py`` and ``accel.py`` for more
>> +information.
> 

I broke my own convention here and mentioned something beside 
__init__.py. I will remove that.

> And example of the bit rot and the huge maintainance cost is when a
> new file is added here, let's say, "qemu/utils/network.py".  I think
> your good intentions would quickly backfire.
> 

As you point out, that little slip-up of mine gave you room to attack 
the bitrot :) I avoided it in two other places, but the utils package is 
sooooo tiny I gave in to trying to be helpful and pointed out the real 
implementation file.

> Regards,
> - Cleber.
> 

I'm still somewhat on the fence. I was trying to make the python 
directories accessible to outside contributors as much as was humanly 
possible, but the bitrot concern is a good point. I think it can be 
alleviated by making clear that these files are just little "see also" 
pointers that should quite likely not rot very quickly.

Lemme know your final thoughts here and I'll adjust accordingly.

--js



  reply	other threads:[~2021-02-18 17:59 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-11 18:58 [PATCH v4 00/24] python: create installable package John Snow
2021-02-11 18:58 ` [PATCH v4 01/24] python/console_socket: avoid one-letter variable John Snow
2021-02-12  4:47   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 02/24] iotests/297: add --namespace-packages to mypy arguments John Snow
2021-02-12  4:53   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 03/24] python: create qemu packages John Snow
2021-02-12  5:17   ` Cleber Rosa
2021-02-15 20:31     ` John Snow
2021-02-11 18:58 ` [PATCH v4 04/24] python: create utils sub-package John Snow
2021-02-17  1:20   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 05/24] python: add qemu package installer John Snow
2021-02-17  2:23   ` Cleber Rosa
2021-02-17  3:38     ` John Snow
2021-02-11 18:58 ` [PATCH v4 06/24] python: add VERSION file John Snow
2021-02-17  2:26   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 07/24] python: add directory structure README.rst files John Snow
2021-02-17  2:47   ` Cleber Rosa
2021-02-18 17:45     ` John Snow [this message]
2021-02-11 18:58 ` [PATCH v4 08/24] python: Add pipenv support John Snow
2021-02-17  2:59   ` Cleber Rosa
2021-02-17  3:02     ` Cleber Rosa
2021-02-17 17:28       ` John Snow
2021-02-17 19:39         ` Cleber Rosa
2021-02-17  3:42     ` John Snow
2021-02-11 18:58 ` [PATCH v4 09/24] python: add pylint import exceptions John Snow
2021-02-17  3:07   ` Cleber Rosa
2021-02-17  3:44     ` John Snow
2021-02-11 18:58 ` [PATCH v4 10/24] python: move pylintrc into setup.cfg John Snow
2021-02-17  3:09   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 11/24] python: add pylint to pipenv John Snow
2021-02-17  4:12   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 12/24] python: move flake8 config to setup.cfg John Snow
2021-02-17  4:17   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 13/24] python: Add flake8 to pipenv John Snow
2021-02-17  4:20   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 14/24] python: move mypy.ini into setup.cfg John Snow
2021-02-11 18:58 ` [PATCH v4 15/24] python: add mypy to pipenv John Snow
2021-02-17  4:38   ` Cleber Rosa
2021-02-17 16:40     ` John Snow
2021-02-11 18:58 ` [PATCH v4 16/24] python: move .isort.cfg into setup.cfg John Snow
2021-02-17  4:44   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 17/24] python/qemu: add isort to pipenv John Snow
2021-02-17  4:45   ` Cleber Rosa
2021-02-11 18:58 ` [PATCH v4 18/24] python/qemu: add qemu package itself " John Snow
2021-02-17  4:47   ` Cleber Rosa
2021-02-17 16:42     ` John Snow
2021-02-11 18:58 ` [PATCH v4 19/24] python: add devel package requirements to setuptools John Snow
2021-02-11 18:58 ` [PATCH v4 20/24] python: add pytest and tests John Snow
2021-02-11 18:58 ` [PATCH v4 21/24] python: add excluded dirs to flake8 config John Snow
2021-02-11 18:58 ` [PATCH v4 22/24] python: add Makefile for some common tasks John Snow
2021-02-11 18:58 ` [PATCH v4 23/24] python: add .gitignore John Snow
2021-02-11 18:58 ` [PATCH v4 24/24] gitlab: add python linters to CI John Snow
2021-02-12  2:52 ` [PATCH v4 00/24] python: create installable package Cleber Rosa
2021-02-15 21:32   ` John Snow
2021-02-17  3:37     ` Cleber Rosa

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=69a11981-8db5-a47e-74a9-8c47fac16270@redhat.com \
    --to=jsnow@redhat.com \
    --cc=alex.bennee@linaro.org \
    --cc=bleal@redhat.com \
    --cc=crosa@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=fam@euphon.net \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=philmd@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=thuth@redhat.com \
    --cc=wainersm@redhat.com \
    --cc=wrampazz@redhat.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).