[PATCH 0/5] python: add QAPI and qapidoc et al to python linter tests

[PATCH 0/5] python: add QAPI and qapidoc et al to python linter tests

[John Snow]

Hiya, this series turns on automated linting for scripts/qapi,
docs/sphinx/qapidoc.py and docs/sphinx/qapi_domain.py.

This includes flake8/isort/pylint/mypy for scripts/qapi, but omits mypy
from the Sphinx plugins owing to my inability to strictly type the
extensions given the wide versions of Sphinx we actually support.

Though I have been using black in my own development, I have not yet
enabled it anywhere automatically. Maybe soon.

John Snow (5):
  qapi: Add some pylint ignores
  docs/qapidoc: linting fixes
  python: update missing dependencies from minreqs
  python: add qapi static analysis tests
  qapi: delete un-needed python static analysis configs

 docs/sphinx/qapi_domain.py  | 25 ++++++++++++++-----------
 docs/sphinx/qapidoc.py      |  5 +++--
 python/setup.cfg            |  1 +
 python/tests/minreqs.txt    | 25 +++++++++++++++++++++++++
 python/tests/qapi-flake8.sh |  4 ++++
 python/tests/qapi-isort.sh  |  6 ++++++
 python/tests/qapi-mypy.sh   |  2 ++
 python/tests/qapi-pylint.sh |  6 ++++++
 scripts/qapi/.flake8        |  3 ---
 scripts/qapi/.isort.cfg     |  7 -------
 scripts/qapi/backend.py     |  2 ++
 scripts/qapi/mypy.ini       |  4 ----
 scripts/qapi/pylintrc       |  1 +
 13 files changed, 64 insertions(+), 27 deletions(-)
 create mode 100755 python/tests/qapi-flake8.sh
 create mode 100755 python/tests/qapi-isort.sh
 create mode 100755 python/tests/qapi-mypy.sh
 create mode 100755 python/tests/qapi-pylint.sh
 delete mode 100644 scripts/qapi/.flake8
 delete mode 100644 scripts/qapi/.isort.cfg
 delete mode 100644 scripts/qapi/mypy.ini

-- 
2.48.1

[PATCH 1/5] qapi: Add some pylint ignores

[John Snow]

This restores the linting baseline in QAPI.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/backend.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/qapi/backend.py b/scripts/qapi/backend.py
index 14e60aa67af..49ae6ecdd33 100644
--- a/scripts/qapi/backend.py
+++ b/scripts/qapi/backend.py
@@ -13,6 +13,7 @@
 
 
 class QAPIBackend(ABC):
+    # pylint: disable=too-few-public-methods
 
     @abstractmethod
     def generate(self,
@@ -36,6 +37,7 @@ def generate(self,
 
 
 class QAPICBackend(QAPIBackend):
+    # pylint: disable=too-few-public-methods
 
     def generate(self,
                  schema: QAPISchema,
-- 
2.48.1

[PATCH 2/5] docs/qapidoc: linting fixes

[John Snow]

This restores the linting baseline in qapidoc. The order of some imports
have changed slightly due to configuring isort a little better: isort
was having difficulty understanding that "compat" and "qapidoc_legacy"
were local modules because docs/sphinx "isn't a python package".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapi_domain.py | 25 ++++++++++++++-----------
 docs/sphinx/qapidoc.py     |  5 +++--
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/docs/sphinx/qapi_domain.py b/docs/sphinx/qapi_domain.py
index c94af5719ca..ebc46a72c61 100644
--- a/docs/sphinx/qapi_domain.py
+++ b/docs/sphinx/qapi_domain.py
@@ -20,16 +20,6 @@
 
 from docutils import nodes
 from docutils.parsers.rst import directives
-
-from compat import (
-    CompatField,
-    CompatGroupedField,
-    CompatTypedField,
-    KeywordNode,
-    ParserFix,
-    Signature,
-    SpaceNode,
-)
 from sphinx import addnodes
 from sphinx.directives import ObjectDescription
 from sphinx.domains import (
@@ -44,6 +34,16 @@
 from sphinx.util.docutils import SphinxDirective
 from sphinx.util.nodes import make_id, make_refnode
 
+from compat import (
+    CompatField,
+    CompatGroupedField,
+    CompatTypedField,
+    KeywordNode,
+    ParserFix,
+    Signature,
+    SpaceNode,
+)
+
 
 if TYPE_CHECKING:
     from typing import (
@@ -56,7 +56,6 @@
     )
 
     from docutils.nodes import Element, Node
-
     from sphinx.addnodes import desc_signature, pending_xref
     from sphinx.application import Sphinx
     from sphinx.builders import Builder
@@ -168,6 +167,8 @@ class QAPIDescription(ParserFix):
     """
 
     def handle_signature(self, sig: str, signode: desc_signature) -> Signature:
+        # pylint: disable=unused-argument
+
         # Do nothing. The return value here is the "name" of the entity
         # being documented; for QAPI, this is the same as the
         # "signature", which is just a name.
@@ -210,6 +211,8 @@ def _get_fqn(self, name: Signature) -> str:
     def add_target_and_index(
         self, name: Signature, sig: str, signode: desc_signature
     ) -> None:
+        # pylint: disable=unused-argument
+
         # name is the return value of handle_signature.
         # sig is the original, raw text argument to handle_signature.
         # For QAPI, these are identical, currently.
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 661b2c4ed0e..8011ac9efaf 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -27,6 +27,7 @@
 
 from __future__ import annotations
 
+
 __version__ = "2.0"
 
 from contextlib import contextmanager
@@ -56,8 +57,6 @@
     QAPISchemaVisitor,
 )
 from qapi.source import QAPISourceInfo
-
-from qapidoc_legacy import QAPISchemaGenRSTVisitor  # type: ignore
 from sphinx import addnodes
 from sphinx.directives.code import CodeBlock
 from sphinx.errors import ExtensionError
@@ -65,6 +64,8 @@
 from sphinx.util.docutils import SphinxDirective, switch_source_input
 from sphinx.util.nodes import nested_parse_with_titles
 
+from qapidoc_legacy import QAPISchemaGenRSTVisitor  # type: ignore
+
 
 if TYPE_CHECKING:
     from typing import (
-- 
2.48.1

Re: [PATCH 2/5] docs/qapidoc: linting fixes

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This restores the linting baseline in qapidoc. The order of some imports
> have changed slightly due to configuring isort a little better: isort

Changed since when / what?

> was having difficulty understanding that "compat" and "qapidoc_legacy"
> were local modules because docs/sphinx "isn't a python package".
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Re: [PATCH 2/5] docs/qapidoc: linting fixes

[John Snow]

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

On Tue, Mar 25, 2025 at 3:36 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This restores the linting baseline in qapidoc. The order of some imports
> > have changed slightly due to configuring isort a little better: isort
>
> Changed since when / what?
>

Changed as of this patch. Before, I was running isort directly from the
docs/sphinx folder, but now I'm running it from the Python directory with
some improved configuration as I explain in this commit: by teaching isort
that compat and qapidoc_legacy are local modules, isort decides to arrange
them differently.

Imports should always go in three sections, in order:

1. Standard library imports
2. Third party imports
3. First party (local) imports

Before, it was not, because it does not understand "docs/sphinx" as a local
package with local modules - I think it gets confused because of the folder
being named "sphinx".

Really, I am just explaining why some imports get shuffled around a little
bit - the new order is "correct" and the old order was slightly wrong.


>
> > was having difficulty understanding that "compat" and "qapidoc_legacy"
> > were local modules because docs/sphinx "isn't a python package".
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
>

[-- Attachment #2: Type: text/html, Size: 2137 bytes --]

Re: [PATCH 2/5] docs/qapidoc: linting fixes

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Mar 25, 2025 at 3:36 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This restores the linting baseline in qapidoc. The order of some imports
>> > have changed slightly due to configuring isort a little better: isort
>>
>> Changed since when / what?
>>
>
> Changed as of this patch. Before, I was running isort directly from the
> docs/sphinx folder, but now I'm running it from the Python directory with
> some improved configuration as I explain in this commit: by teaching isort
> that compat and qapidoc_legacy are local modules, isort decides to arrange
> them differently.
>
> Imports should always go in three sections, in order:
>
> 1. Standard library imports
> 2. Third party imports
> 3. First party (local) imports
>
> Before, it was not, because it does not understand "docs/sphinx" as a local
> package with local modules - I think it gets confused because of the folder
> being named "sphinx".
>
> Really, I am just explaining why some imports get shuffled around a little
> bit - the new order is "correct" and the old order was slightly wrong.

Please use "The order of imorts change" instead of "have changed".
Present tense makes it clear that you're talking about this patch.

>> > was having difficulty understanding that "compat" and "qapidoc_legacy"
>> > were local modules because docs/sphinx "isn't a python package".
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>

[PATCH 3/5] python: update missing dependencies from minreqs

[John Snow]

A few transitive dependencies were left floating; as a result, pip's
dependency solver can pull in newer dependencies, which we don't
want. Pin them down.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/tests/minreqs.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
index a3f423efd84..19c0f5e4c50 100644
--- a/python/tests/minreqs.txt
+++ b/python/tests/minreqs.txt
@@ -38,10 +38,14 @@ pyflakes==2.5.0
 
 # Transitive mypy dependencies
 mypy-extensions==1.0.0
+tomli==1.1.0
 typing-extensions==4.7.1
 
 # Transitive pylint dependencies
 astroid==2.15.4
+dill==0.2
 lazy-object-proxy==1.4.0
+platformdirs==2.2.0
 toml==0.10.0
+tomlkit==0.10.1
 wrapt==1.14.0
-- 
2.48.1

Re: [PATCH 3/5] python: update missing dependencies from minreqs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> A few transitive dependencies were left floating; as a result, pip's
> dependency solver can pull in newer dependencies, which we don't
> want. Pin them down.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

What problem exactly does this fix?  Make target check-minreqs?

> ---
>  python/tests/minreqs.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
> index a3f423efd84..19c0f5e4c50 100644
> --- a/python/tests/minreqs.txt
> +++ b/python/tests/minreqs.txt
> @@ -38,10 +38,14 @@ pyflakes==2.5.0
>  
>  # Transitive mypy dependencies
>  mypy-extensions==1.0.0
> +tomli==1.1.0
>  typing-extensions==4.7.1
>  
>  # Transitive pylint dependencies
>  astroid==2.15.4
> +dill==0.2
>  lazy-object-proxy==1.4.0
> +platformdirs==2.2.0
>  toml==0.10.0
> +tomlkit==0.10.1
>  wrapt==1.14.0

Re: [PATCH 3/5] python: update missing dependencies from minreqs

[John Snow]

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

On Wed, Mar 26, 2025 at 2:08 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > A few transitive dependencies were left floating; as a result, pip's
> > dependency solver can pull in newer dependencies, which we don't
> > want. Pin them down.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> What problem exactly does this fix?  Make target check-minreqs?
>

I'm not sure it's a "problem" as such, but an inconsistency. Yes, it's with
check-minreqs -- without this patch, pip is free to choose newer versions
of these dependencies as appropriate. Though unlikely at this point, in
theory, new dependency updates could be selected by pip and invalidate the
concept of an entirely fixed/pinned virtual environment.

That these transitive dependencies were not frozen initially was an
oversight.

check-minreqs is supposed to build the exact same venv every time without
fail. Without this change, it's *possible* that it might do something
different on release day if someone releases a new package. No good,
probably.


>
> > ---
> >  python/tests/minreqs.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
> > index a3f423efd84..19c0f5e4c50 100644
> > --- a/python/tests/minreqs.txt
> > +++ b/python/tests/minreqs.txt
> > @@ -38,10 +38,14 @@ pyflakes==2.5.0
> >
> >  # Transitive mypy dependencies
> >  mypy-extensions==1.0.0
> > +tomli==1.1.0
> >  typing-extensions==4.7.1
> >
> >  # Transitive pylint dependencies
> >  astroid==2.15.4
> > +dill==0.2
> >  lazy-object-proxy==1.4.0
> > +platformdirs==2.2.0
> >  toml==0.10.0
> > +tomlkit==0.10.1
> >  wrapt==1.14.0
>
>

[-- Attachment #2: Type: text/html, Size: 2593 bytes --]

Re: [PATCH 3/5] python: update missing dependencies from minreqs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Mar 26, 2025 at 2:08 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > A few transitive dependencies were left floating; as a result, pip's
>> > dependency solver can pull in newer dependencies, which we don't
>> > want. Pin them down.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> What problem exactly does this fix?  Make target check-minreqs?
>>
>
> I'm not sure it's a "problem" as such, but an inconsistency. Yes, it's with
> check-minreqs -- without this patch, pip is free to choose newer versions
> of these dependencies as appropriate. Though unlikely at this point, in
> theory, new dependency updates could be selected by pip and invalidate the
> concept of an entirely fixed/pinned virtual environment.
>
> That these transitive dependencies were not frozen initially was an
> oversight.
>
> check-minreqs is supposed to build the exact same venv every time without
> fail. Without this change, it's *possible* that it might do something
> different on release day if someone releases a new package. No good,
> probably.

I see.

You've been spoiling me with really nice commit messages...  If you'd
like to push this one to that level, I'd suggest to start with a short
paragraph explaining why we pin versions for check-minreq, then state
the issue being fixed: we missed some pins.

[...]

Re: [PATCH 3/5] python: update missing dependencies from minreqs

[John Snow]

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

On Thu, Mar 27, 2025 at 1:36 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Wed, Mar 26, 2025 at 2:08 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > A few transitive dependencies were left floating; as a result, pip's
> >> > dependency solver can pull in newer dependencies, which we don't
> >> > want. Pin them down.
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >>
> >> What problem exactly does this fix?  Make target check-minreqs?
> >>
> >
> > I'm not sure it's a "problem" as such, but an inconsistency. Yes, it's
> with
> > check-minreqs -- without this patch, pip is free to choose newer versions
> > of these dependencies as appropriate. Though unlikely at this point, in
> > theory, new dependency updates could be selected by pip and invalidate
> the
> > concept of an entirely fixed/pinned virtual environment.
> >
> > That these transitive dependencies were not frozen initially was an
> > oversight.
> >
> > check-minreqs is supposed to build the exact same venv every time without
> > fail. Without this change, it's *possible* that it might do something
> > different on release day if someone releases a new package. No good,
> > probably.
>
> I see.
>
> You've been spoiling me with really nice commit messages...  If you'd
> like to push this one to that level, I'd suggest to start with a short
> paragraph explaining why we pin versions for check-minreq, then state
> the issue being fixed: we missed some pins.
>

"If you give a mouse a cookie, ..."

Already typed it all out to you, might as well update the commit message at
this point.

--js

[-- Attachment #2: Type: text/html, Size: 2614 bytes --]

[PATCH 4/5] python: add qapi static analysis tests

[John Snow]

Update the python tests to also check qapi. No idea why I didn't do this
before. I guess I was counting on moving it under python/ and then just
forgot after that was NACKed. Oops, this turns out to be really easy.

flake8, isort and mypy use the tool configuration from the existing
python directory (in setup.cfg). pylint continues to use the special
configuration located in scripts/qapi/ - that configuration is more
permissive. If we wish to unify the two configurations, that's a
separate series and a discussion for a later date.

As a result of this patch, one would be able to run any of the following
tests locally from the qemu.git/python directory and have it cover the
scripts/qapi/ module as well. All of the following options run the
python tests, static analysis tests, and linter checks; but with
different combinations of dependencies and interpreters.

- "make check-minreqs" Run tests specifically under our oldest supported
  Python and our oldest supported dependencies. This is the test that
  runs on GitLab as "check-python-minreqs". This helps ensure we do not
  regress support on older platforms accidentally.

- "make check-tox" Runs the tests under the newest supported
  dependencies, but under each supported version of Python in turn. At
  time of writing, this is Python 3.8 to 3.13 inclusive. This test helps
  catch bleeding-edge problems before they become problems for developer
  workstations. This is the GitLab test "check-python-tox" and is an
  optionally run, may-fail test due to the unpredictable nature of new
  dependencies being released into the ecosystem that may cause
  regressions.

- "make check-dev" Runs the tests under the newest supported
  dependencies using whatever version of Python the user happens to have
  installed. This is a quick convenience check that does not map to any
  particular GitLab test.

(Note! check-dev may be busted on Fedora 41 and bleeding edge versions
of setuptools. That's unrelated to this patch and I'll address it
separately and soon. Thank you for your patience, --mgmt)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/setup.cfg            |  1 +
 python/tests/minreqs.txt    | 21 +++++++++++++++++++++
 python/tests/qapi-flake8.sh |  4 ++++
 python/tests/qapi-isort.sh  |  6 ++++++
 python/tests/qapi-mypy.sh   |  2 ++
 python/tests/qapi-pylint.sh |  6 ++++++
 scripts/qapi/pylintrc       |  1 +
 7 files changed, 41 insertions(+)
 create mode 100755 python/tests/qapi-flake8.sh
 create mode 100755 python/tests/qapi-isort.sh
 create mode 100755 python/tests/qapi-mypy.sh
 create mode 100755 python/tests/qapi-pylint.sh

diff --git a/python/setup.cfg b/python/setup.cfg
index cf5af7e6641..84d8a1fd30d 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -47,6 +47,7 @@ devel =
     urwid >= 2.1.2
     urwid-readline >= 0.13
     Pygments >= 2.9.0
+    sphinx >= 3.4.3
 
 # Provides qom-fuse functionality
 fuse =
diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
index 19c0f5e4c50..94928936d44 100644
--- a/python/tests/minreqs.txt
+++ b/python/tests/minreqs.txt
@@ -11,6 +11,9 @@
 # When adding new dependencies, pin the very oldest non-yanked version
 # on PyPI that allows the test suite to pass.
 
+# Dependencies for qapidoc/qapi_domain et al
+sphinx==3.4.3
+
 # Dependencies for the TUI addon (Required for successful linting)
 urwid==2.1.2
 urwid-readline==0.13
@@ -49,3 +52,21 @@ platformdirs==2.2.0
 toml==0.10.0
 tomlkit==0.10.1
 wrapt==1.14.0
+
+# Transitive sphinx dependencies
+Jinja2==2.7
+MarkupSafe==1.1.0
+alabaster==0.7.1
+babel==1.3
+docutils==0.12
+imagesize==0.5.0
+packaging==14.0
+pytz==2011b0
+requests==2.5.0
+snowballstemmer==1.1
+sphinxcontrib-applehelp==1.0.0
+sphinxcontrib-devhelp==1.0.0
+sphinxcontrib-htmlhelp==1.0.0
+sphinxcontrib-jsmath==1.0.0
+sphinxcontrib-qthelp==1.0.0
+sphinxcontrib-serializinghtml==1.0.0
diff --git a/python/tests/qapi-flake8.sh b/python/tests/qapi-flake8.sh
new file mode 100755
index 00000000000..7b5983d64a9
--- /dev/null
+++ b/python/tests/qapi-flake8.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+python3 -m flake8 ../scripts/qapi/ \
+        ../docs/sphinx/qapidoc.py \
+        ../docs/sphinx/qapi_domain.py
diff --git a/python/tests/qapi-isort.sh b/python/tests/qapi-isort.sh
new file mode 100755
index 00000000000..f31f12d3425
--- /dev/null
+++ b/python/tests/qapi-isort.sh
@@ -0,0 +1,6 @@
+#!/bin/sh -e
+python3 -m isort --sp . -c ../scripts/qapi/
+# Force isort to recognize "compat" as a local module and not third-party
+python3 -m isort --sp . -c -p compat -p qapidoc_legacy \
+        ../docs/sphinx/qapi_domain.py \
+        ../docs/sphinx/qapidoc.py
diff --git a/python/tests/qapi-mypy.sh b/python/tests/qapi-mypy.sh
new file mode 100755
index 00000000000..377b29b873b
--- /dev/null
+++ b/python/tests/qapi-mypy.sh
@@ -0,0 +1,2 @@
+#!/bin/sh -e
+python3 -m mypy ../scripts/qapi
diff --git a/python/tests/qapi-pylint.sh b/python/tests/qapi-pylint.sh
new file mode 100755
index 00000000000..f4bb7a5a795
--- /dev/null
+++ b/python/tests/qapi-pylint.sh
@@ -0,0 +1,6 @@
+#!/bin/sh -e
+SETUPTOOLS_USE_DISTUTILS=stdlib python3 -m pylint \
+                                --rcfile=../scripts/qapi/pylintrc \
+                                ../scripts/qapi/ \
+                                ../docs/sphinx/qapidoc.py \
+                                ../docs/sphinx/qapi_domain.py
diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index d24eece7411..e16283ada3d 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -19,6 +19,7 @@ disable=consider-using-f-string,
         too-many-instance-attributes,
         too-many-positional-arguments,
         too-many-statements,
+        unknown-option-value,
         useless-option-value,
 
 [REPORTS]
-- 
2.48.1

Re: [PATCH 4/5] python: add qapi static analysis tests

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Update the python tests to also check qapi. No idea why I didn't do this
> before. I guess I was counting on moving it under python/ and then just
> forgot after that was NACKed. Oops, this turns out to be really easy.
>
> flake8, isort and mypy use the tool configuration from the existing
> python directory (in setup.cfg). pylint continues to use the special
> configuration located in scripts/qapi/ - that configuration is more
> permissive. If we wish to unify the two configurations, that's a
> separate series and a discussion for a later date.
>
> As a result of this patch, one would be able to run any of the following
> tests locally from the qemu.git/python directory and have it cover the
> scripts/qapi/ module as well. All of the following options run the
> python tests, static analysis tests, and linter checks; but with
> different combinations of dependencies and interpreters.
>
> - "make check-minreqs" Run tests specifically under our oldest supported
>   Python and our oldest supported dependencies. This is the test that
>   runs on GitLab as "check-python-minreqs". This helps ensure we do not
>   regress support on older platforms accidentally.
>
> - "make check-tox" Runs the tests under the newest supported
>   dependencies, but under each supported version of Python in turn. At
>   time of writing, this is Python 3.8 to 3.13 inclusive. This test helps
>   catch bleeding-edge problems before they become problems for developer
>   workstations. This is the GitLab test "check-python-tox" and is an
>   optionally run, may-fail test due to the unpredictable nature of new
>   dependencies being released into the ecosystem that may cause
>   regressions.
>
> - "make check-dev" Runs the tests under the newest supported
>   dependencies using whatever version of Python the user happens to have
>   installed. This is a quick convenience check that does not map to any
>   particular GitLab test.
>
> (Note! check-dev may be busted on Fedora 41 and bleeding edge versions
> of setuptools. That's unrelated to this patch and I'll address it
> separately and soon. Thank you for your patience, --mgmt)
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Let's mention this is a step towards having "make check" run the static
analysis we want developers to run, but we're not there, yet.

> ---
>  python/setup.cfg            |  1 +
>  python/tests/minreqs.txt    | 21 +++++++++++++++++++++
>  python/tests/qapi-flake8.sh |  4 ++++
>  python/tests/qapi-isort.sh  |  6 ++++++
>  python/tests/qapi-mypy.sh   |  2 ++
>  python/tests/qapi-pylint.sh |  6 ++++++
>  scripts/qapi/pylintrc       |  1 +
>  7 files changed, 41 insertions(+)
>  create mode 100755 python/tests/qapi-flake8.sh
>  create mode 100755 python/tests/qapi-isort.sh
>  create mode 100755 python/tests/qapi-mypy.sh
>  create mode 100755 python/tests/qapi-pylint.sh
>
> diff --git a/python/setup.cfg b/python/setup.cfg
> index cf5af7e6641..84d8a1fd30d 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -47,6 +47,7 @@ devel =
>      urwid >= 2.1.2
>      urwid-readline >= 0.13
>      Pygments >= 2.9.0
> +    sphinx >= 3.4.3
>  
>  # Provides qom-fuse functionality
>  fuse =
> diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
> index 19c0f5e4c50..94928936d44 100644
> --- a/python/tests/minreqs.txt
> +++ b/python/tests/minreqs.txt
> @@ -11,6 +11,9 @@
>  # When adding new dependencies, pin the very oldest non-yanked version
>  # on PyPI that allows the test suite to pass.
>  
> +# Dependencies for qapidoc/qapi_domain et al
> +sphinx==3.4.3
> +
>  # Dependencies for the TUI addon (Required for successful linting)
>  urwid==2.1.2
>  urwid-readline==0.13
> @@ -49,3 +52,21 @@ platformdirs==2.2.0
>  toml==0.10.0
>  tomlkit==0.10.1
>  wrapt==1.14.0
> +
> +# Transitive sphinx dependencies
> +Jinja2==2.7
> +MarkupSafe==1.1.0
> +alabaster==0.7.1
> +babel==1.3
> +docutils==0.12
> +imagesize==0.5.0
> +packaging==14.0
> +pytz==2011b0
> +requests==2.5.0
> +snowballstemmer==1.1
> +sphinxcontrib-applehelp==1.0.0
> +sphinxcontrib-devhelp==1.0.0
> +sphinxcontrib-htmlhelp==1.0.0
> +sphinxcontrib-jsmath==1.0.0
> +sphinxcontrib-qthelp==1.0.0
> +sphinxcontrib-serializinghtml==1.0.0

This wasn't there when I last saw this patch.  The previous patch also
updates this file.  How did you decide which updates go where?  Or is
this an accident?

> diff --git a/python/tests/qapi-flake8.sh b/python/tests/qapi-flake8.sh
> new file mode 100755
> index 00000000000..7b5983d64a9
> --- /dev/null
> +++ b/python/tests/qapi-flake8.sh
> @@ -0,0 +1,4 @@
> +#!/bin/sh -e
> +python3 -m flake8 ../scripts/qapi/ \
> +        ../docs/sphinx/qapidoc.py \
> +        ../docs/sphinx/qapi_domain.py

Not linting docs/sphinx/qapidoc_legacy.py.  This is intentional (see its
initial commit message).  Since we plan to drop it soon, there's no real
need for a comment here, but mentioning it in the commit message
wouldn't hurt.

> diff --git a/python/tests/qapi-isort.sh b/python/tests/qapi-isort.sh
> new file mode 100755
> index 00000000000..f31f12d3425
> --- /dev/null
> +++ b/python/tests/qapi-isort.sh
> @@ -0,0 +1,6 @@
> +#!/bin/sh -e
> +python3 -m isort --sp . -c ../scripts/qapi/
> +# Force isort to recognize "compat" as a local module and not third-party
> +python3 -m isort --sp . -c -p compat -p qapidoc_legacy \
> +        ../docs/sphinx/qapi_domain.py \
> +        ../docs/sphinx/qapidoc.py

Comment on flake8 applies.

> diff --git a/python/tests/qapi-mypy.sh b/python/tests/qapi-mypy.sh
> new file mode 100755
> index 00000000000..377b29b873b
> --- /dev/null
> +++ b/python/tests/qapi-mypy.sh
> @@ -0,0 +1,2 @@
> +#!/bin/sh -e
> +python3 -m mypy ../scripts/qapi

Not type-checking docs/sphinx/qapi_domain.py and docs/sphinx/qapidoc.py?
Impractical due to us targeting an isanely wide Sphinx version range?

> diff --git a/python/tests/qapi-pylint.sh b/python/tests/qapi-pylint.sh
> new file mode 100755
> index 00000000000..f4bb7a5a795
> --- /dev/null
> +++ b/python/tests/qapi-pylint.sh
> @@ -0,0 +1,6 @@
> +#!/bin/sh -e
> +SETUPTOOLS_USE_DISTUTILS=stdlib python3 -m pylint \
> +                                --rcfile=../scripts/qapi/pylintrc \
> +                                ../scripts/qapi/ \
> +                                ../docs/sphinx/qapidoc.py \
> +                                ../docs/sphinx/qapi_domain.py

Comment on flake8 applies.

> diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
> index d24eece7411..e16283ada3d 100644
> --- a/scripts/qapi/pylintrc
> +++ b/scripts/qapi/pylintrc
> @@ -19,6 +19,7 @@ disable=consider-using-f-string,
>          too-many-instance-attributes,
>          too-many-positional-arguments,
>          too-many-statements,
> +        unknown-option-value,
>          useless-option-value,
>  
>  [REPORTS]

This wasn't there when I last saw this patch.  PATCH 1 also updates this
file.  How did you decide which updates go where?  Or is this an
accident?

Re: [PATCH 4/5] python: add qapi static analysis tests

[John Snow]

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

On Tue, Mar 25, 2025 at 3:53 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > Update the python tests to also check qapi. No idea why I didn't do this
> > before. I guess I was counting on moving it under python/ and then just
> > forgot after that was NACKed. Oops, this turns out to be really easy.
> >
> > flake8, isort and mypy use the tool configuration from the existing
> > python directory (in setup.cfg). pylint continues to use the special
> > configuration located in scripts/qapi/ - that configuration is more
> > permissive. If we wish to unify the two configurations, that's a
> > separate series and a discussion for a later date.
> >
> > As a result of this patch, one would be able to run any of the following
> > tests locally from the qemu.git/python directory and have it cover the
> > scripts/qapi/ module as well. All of the following options run the
> > python tests, static analysis tests, and linter checks; but with
> > different combinations of dependencies and interpreters.
> >
> > - "make check-minreqs" Run tests specifically under our oldest supported
> >   Python and our oldest supported dependencies. This is the test that
> >   runs on GitLab as "check-python-minreqs". This helps ensure we do not
> >   regress support on older platforms accidentally.
> >
> > - "make check-tox" Runs the tests under the newest supported
> >   dependencies, but under each supported version of Python in turn. At
> >   time of writing, this is Python 3.8 to 3.13 inclusive. This test helps
> >   catch bleeding-edge problems before they become problems for developer
> >   workstations. This is the GitLab test "check-python-tox" and is an
> >   optionally run, may-fail test due to the unpredictable nature of new
> >   dependencies being released into the ecosystem that may cause
> >   regressions.
> >
> > - "make check-dev" Runs the tests under the newest supported
> >   dependencies using whatever version of Python the user happens to have
> >   installed. This is a quick convenience check that does not map to any
> >   particular GitLab test.
> >
> > (Note! check-dev may be busted on Fedora 41 and bleeding edge versions
> > of setuptools. That's unrelated to this patch and I'll address it
> > separately and soon. Thank you for your patience, --mgmt)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Let's mention this is a step towards having "make check" run the static
> analysis we want developers to run, but we're not there, yet.
>

It both is and isn't. That we can now check qapi and the qapi sphinx
extensions from the same place as we do linting for python/ is sufficient
justification in and of itself, regardless of how we improve and integrate
this testing later on.


>
> > ---
> >  python/setup.cfg            |  1 +
> >  python/tests/minreqs.txt    | 21 +++++++++++++++++++++
> >  python/tests/qapi-flake8.sh |  4 ++++
> >  python/tests/qapi-isort.sh  |  6 ++++++
> >  python/tests/qapi-mypy.sh   |  2 ++
> >  python/tests/qapi-pylint.sh |  6 ++++++
> >  scripts/qapi/pylintrc       |  1 +
> >  7 files changed, 41 insertions(+)
> >  create mode 100755 python/tests/qapi-flake8.sh
> >  create mode 100755 python/tests/qapi-isort.sh
> >  create mode 100755 python/tests/qapi-mypy.sh
> >  create mode 100755 python/tests/qapi-pylint.sh
> >
> > diff --git a/python/setup.cfg b/python/setup.cfg
> > index cf5af7e6641..84d8a1fd30d 100644
> > --- a/python/setup.cfg
> > +++ b/python/setup.cfg
> > @@ -47,6 +47,7 @@ devel =
> >      urwid >= 2.1.2
> >      urwid-readline >= 0.13
> >      Pygments >= 2.9.0
> > +    sphinx >= 3.4.3
> >
> >  # Provides qom-fuse functionality
> >  fuse =
> > diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
> > index 19c0f5e4c50..94928936d44 100644
> > --- a/python/tests/minreqs.txt
> > +++ b/python/tests/minreqs.txt
> > @@ -11,6 +11,9 @@
> >  # When adding new dependencies, pin the very oldest non-yanked version
> >  # on PyPI that allows the test suite to pass.
> >
> > +# Dependencies for qapidoc/qapi_domain et al
> > +sphinx==3.4.3
> > +
> >  # Dependencies for the TUI addon (Required for successful linting)
> >  urwid==2.1.2
> >  urwid-readline==0.13
> > @@ -49,3 +52,21 @@ platformdirs==2.2.0
> >  toml==0.10.0
> >  tomlkit==0.10.1
> >  wrapt==1.14.0
> > +
> > +# Transitive sphinx dependencies
> > +Jinja2==2.7
> > +MarkupSafe==1.1.0
> > +alabaster==0.7.1
> > +babel==1.3
> > +docutils==0.12
> > +imagesize==0.5.0
> > +packaging==14.0
> > +pytz==2011b0
> > +requests==2.5.0
> > +snowballstemmer==1.1
> > +sphinxcontrib-applehelp==1.0.0
> > +sphinxcontrib-devhelp==1.0.0
> > +sphinxcontrib-htmlhelp==1.0.0
> > +sphinxcontrib-jsmath==1.0.0
> > +sphinxcontrib-qthelp==1.0.0
> > +sphinxcontrib-serializinghtml==1.0.0
>
> This wasn't there when I last saw this patch.  The previous patch also
> updates this file.  How did you decide which updates go where?  Or is
> this an accident?
>

The previous patch pins dependencies that already existed, but we neglected
to pin in this file. It's fixing an existing oversight.

This patch adds a bunch of new pinned dependencies for Sphinx, which we
need for type-checking Sphinx extensions.


>
> > diff --git a/python/tests/qapi-flake8.sh b/python/tests/qapi-flake8.sh
> > new file mode 100755
> > index 00000000000..7b5983d64a9
> > --- /dev/null
> > +++ b/python/tests/qapi-flake8.sh
> > @@ -0,0 +1,4 @@
> > +#!/bin/sh -e
> > +python3 -m flake8 ../scripts/qapi/ \
> > +        ../docs/sphinx/qapidoc.py \
> > +        ../docs/sphinx/qapi_domain.py
>
> Not linting docs/sphinx/qapidoc_legacy.py.  This is intentional (see its
> initial commit message).  Since we plan to drop it soon, there's no real
> need for a comment here, but mentioning it in the commit message
> wouldn't hurt.
>

Alright.


>
> > diff --git a/python/tests/qapi-isort.sh b/python/tests/qapi-isort.sh
> > new file mode 100755
> > index 00000000000..f31f12d3425
> > --- /dev/null
> > +++ b/python/tests/qapi-isort.sh
> > @@ -0,0 +1,6 @@
> > +#!/bin/sh -e
> > +python3 -m isort --sp . -c ../scripts/qapi/
> > +# Force isort to recognize "compat" as a local module and not
> third-party
> > +python3 -m isort --sp . -c -p compat -p qapidoc_legacy \
> > +        ../docs/sphinx/qapi_domain.py \
> > +        ../docs/sphinx/qapidoc.py
>
> Comment on flake8 applies.
>
> > diff --git a/python/tests/qapi-mypy.sh b/python/tests/qapi-mypy.sh
> > new file mode 100755
> > index 00000000000..377b29b873b
> > --- /dev/null
> > +++ b/python/tests/qapi-mypy.sh
> > @@ -0,0 +1,2 @@
> > +#!/bin/sh -e
> > +python3 -m mypy ../scripts/qapi
>
> Not type-checking docs/sphinx/qapi_domain.py and docs/sphinx/qapidoc.py?
> Impractical due to us targeting an isanely wide Sphinx version range?
>

Yes.


>
> > diff --git a/python/tests/qapi-pylint.sh b/python/tests/qapi-pylint.sh
> > new file mode 100755
> > index 00000000000..f4bb7a5a795
> > --- /dev/null
> > +++ b/python/tests/qapi-pylint.sh
> > @@ -0,0 +1,6 @@
> > +#!/bin/sh -e
> > +SETUPTOOLS_USE_DISTUTILS=stdlib python3 -m pylint \
> > +                                --rcfile=../scripts/qapi/pylintrc \
> > +                                ../scripts/qapi/ \
> > +                                ../docs/sphinx/qapidoc.py \
> > +                                ../docs/sphinx/qapi_domain.py
>
> Comment on flake8 applies.
>
> > diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
> > index d24eece7411..e16283ada3d 100644
> > --- a/scripts/qapi/pylintrc
> > +++ b/scripts/qapi/pylintrc
> > @@ -19,6 +19,7 @@ disable=consider-using-f-string,
> >          too-many-instance-attributes,
> >          too-many-positional-arguments,
> >          too-many-statements,
> > +        unknown-option-value,
> >          useless-option-value,
> >
> >  [REPORTS]
>
> This wasn't there when I last saw this patch.  PATCH 1 also updates this
> file.  How did you decide which updates go where?  Or is this an
> accident?


I didn't add the Sphinx extensions last time you saw this series, so that's
new. This winds up being needed to tolerate the "too many positional
arguments" option which only applies to newer pylint versions - older
versions will complain about the option being unrecognized. In order to
continue allowing a wide version of pylint versions, we need this option.

[-- Attachment #2: Type: text/html, Size: 10841 bytes --]

Re: [PATCH 4/5] python: add qapi static analysis tests

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Mar 25, 2025 at 3:53 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Update the python tests to also check qapi. No idea why I didn't do this
>> > before. I guess I was counting on moving it under python/ and then just
>> > forgot after that was NACKed. Oops, this turns out to be really easy.
>> >
>> > flake8, isort and mypy use the tool configuration from the existing
>> > python directory (in setup.cfg). pylint continues to use the special
>> > configuration located in scripts/qapi/ - that configuration is more
>> > permissive. If we wish to unify the two configurations, that's a
>> > separate series and a discussion for a later date.
>> >
>> > As a result of this patch, one would be able to run any of the following
>> > tests locally from the qemu.git/python directory and have it cover the
>> > scripts/qapi/ module as well. All of the following options run the
>> > python tests, static analysis tests, and linter checks; but with
>> > different combinations of dependencies and interpreters.
>> >
>> > - "make check-minreqs" Run tests specifically under our oldest supported
>> >   Python and our oldest supported dependencies. This is the test that
>> >   runs on GitLab as "check-python-minreqs". This helps ensure we do not
>> >   regress support on older platforms accidentally.
>> >
>> > - "make check-tox" Runs the tests under the newest supported
>> >   dependencies, but under each supported version of Python in turn. At
>> >   time of writing, this is Python 3.8 to 3.13 inclusive. This test helps
>> >   catch bleeding-edge problems before they become problems for developer
>> >   workstations. This is the GitLab test "check-python-tox" and is an
>> >   optionally run, may-fail test due to the unpredictable nature of new
>> >   dependencies being released into the ecosystem that may cause
>> >   regressions.
>> >
>> > - "make check-dev" Runs the tests under the newest supported
>> >   dependencies using whatever version of Python the user happens to have
>> >   installed. This is a quick convenience check that does not map to any
>> >   particular GitLab test.
>> >
>> > (Note! check-dev may be busted on Fedora 41 and bleeding edge versions
>> > of setuptools. That's unrelated to this patch and I'll address it
>> > separately and soon. Thank you for your patience, --mgmt)
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> Let's mention this is a step towards having "make check" run the static
>> analysis we want developers to run, but we're not there, yet.
>>
>
> It both is and isn't. That we can now check qapi and the qapi sphinx
> extensions from the same place as we do linting for python/ is sufficient
> justification in and of itself, regardless of how we improve and integrate
> this testing later on.

Alright.

>> > ---
>> >  python/setup.cfg            |  1 +
>> >  python/tests/minreqs.txt    | 21 +++++++++++++++++++++
>> >  python/tests/qapi-flake8.sh |  4 ++++
>> >  python/tests/qapi-isort.sh  |  6 ++++++
>> >  python/tests/qapi-mypy.sh   |  2 ++
>> >  python/tests/qapi-pylint.sh |  6 ++++++
>> >  scripts/qapi/pylintrc       |  1 +
>> >  7 files changed, 41 insertions(+)
>> >  create mode 100755 python/tests/qapi-flake8.sh
>> >  create mode 100755 python/tests/qapi-isort.sh
>> >  create mode 100755 python/tests/qapi-mypy.sh
>> >  create mode 100755 python/tests/qapi-pylint.sh
>> >
>> > diff --git a/python/setup.cfg b/python/setup.cfg
>> > index cf5af7e6641..84d8a1fd30d 100644
>> > --- a/python/setup.cfg
>> > +++ b/python/setup.cfg
>> > @@ -47,6 +47,7 @@ devel =
>> >      urwid >= 2.1.2
>> >      urwid-readline >= 0.13
>> >      Pygments >= 2.9.0
>> > +    sphinx >= 3.4.3
>> >
>> >  # Provides qom-fuse functionality
>> >  fuse =
>> > diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
>> > index 19c0f5e4c50..94928936d44 100644
>> > --- a/python/tests/minreqs.txt
>> > +++ b/python/tests/minreqs.txt
>> > @@ -11,6 +11,9 @@
>> >  # When adding new dependencies, pin the very oldest non-yanked version
>> >  # on PyPI that allows the test suite to pass.
>> >
>> > +# Dependencies for qapidoc/qapi_domain et al
>> > +sphinx==3.4.3
>> > +
>> >  # Dependencies for the TUI addon (Required for successful linting)
>> >  urwid==2.1.2
>> >  urwid-readline==0.13
>> > @@ -49,3 +52,21 @@ platformdirs==2.2.0
>> >  toml==0.10.0
>> >  tomlkit==0.10.1
>> >  wrapt==1.14.0
>> > +
>> > +# Transitive sphinx dependencies
>> > +Jinja2==2.7
>> > +MarkupSafe==1.1.0
>> > +alabaster==0.7.1
>> > +babel==1.3
>> > +docutils==0.12
>> > +imagesize==0.5.0
>> > +packaging==14.0
>> > +pytz==2011b0
>> > +requests==2.5.0
>> > +snowballstemmer==1.1
>> > +sphinxcontrib-applehelp==1.0.0
>> > +sphinxcontrib-devhelp==1.0.0
>> > +sphinxcontrib-htmlhelp==1.0.0
>> > +sphinxcontrib-jsmath==1.0.0
>> > +sphinxcontrib-qthelp==1.0.0
>> > +sphinxcontrib-serializinghtml==1.0.0
>>
>> This wasn't there when I last saw this patch.  The previous patch also
>> updates this file.  How did you decide which updates go where?  Or is
>> this an accident?
>>
>
> The previous patch pins dependencies that already existed, but we neglected
> to pin in this file. It's fixing an existing oversight.
>
> This patch adds a bunch of new pinned dependencies for Sphinx, which we
> need for type-checking Sphinx extensions.

So... the previous patch fixes existing tests, and this one extends
their coverage to the modern parts of docs/sphinx/.  Correct?

Which tests exactly?  I just asked that on the previous patch.

[...]

>> > diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
>> > index d24eece7411..e16283ada3d 100644
>> > --- a/scripts/qapi/pylintrc
>> > +++ b/scripts/qapi/pylintrc
>> > @@ -19,6 +19,7 @@ disable=consider-using-f-string,
>> >          too-many-instance-attributes,
>> >          too-many-positional-arguments,
>> >          too-many-statements,
>> > +        unknown-option-value,
>> >          useless-option-value,
>> >
>> >  [REPORTS]
>>
>> This wasn't there when I last saw this patch.  PATCH 1 also updates this
>> file.  How did you decide which updates go where?  Or is this an
>> accident?
>
>
> I didn't add the Sphinx extensions last time you saw this series, so that's
> new. This winds up being needed to tolerate the "too many positional
> arguments" option which only applies to newer pylint versions - older
> versions will complain about the option being unrecognized. In order to
> continue allowing a wide version of pylint versions, we need this option.

Got it.  Worth a comment?

Re: [PATCH 4/5] python: add qapi static analysis tests

[John Snow]

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

On Wed, Mar 26, 2025 at 2:13 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Tue, Mar 25, 2025 at 3:53 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > Update the python tests to also check qapi. No idea why I didn't do
> this
> >> > before. I guess I was counting on moving it under python/ and then
> just
> >> > forgot after that was NACKed. Oops, this turns out to be really easy.
> >> >
> >> > flake8, isort and mypy use the tool configuration from the existing
> >> > python directory (in setup.cfg). pylint continues to use the special
> >> > configuration located in scripts/qapi/ - that configuration is more
> >> > permissive. If we wish to unify the two configurations, that's a
> >> > separate series and a discussion for a later date.
> >> >
> >> > As a result of this patch, one would be able to run any of the
> following
> >> > tests locally from the qemu.git/python directory and have it cover the
> >> > scripts/qapi/ module as well. All of the following options run the
> >> > python tests, static analysis tests, and linter checks; but with
> >> > different combinations of dependencies and interpreters.
> >> >
> >> > - "make check-minreqs" Run tests specifically under our oldest
> supported
> >> >   Python and our oldest supported dependencies. This is the test that
> >> >   runs on GitLab as "check-python-minreqs". This helps ensure we do
> not
> >> >   regress support on older platforms accidentally.
> >> >
> >> > - "make check-tox" Runs the tests under the newest supported
> >> >   dependencies, but under each supported version of Python in turn. At
> >> >   time of writing, this is Python 3.8 to 3.13 inclusive. This test
> helps
> >> >   catch bleeding-edge problems before they become problems for
> developer
> >> >   workstations. This is the GitLab test "check-python-tox" and is an
> >> >   optionally run, may-fail test due to the unpredictable nature of new
> >> >   dependencies being released into the ecosystem that may cause
> >> >   regressions.
> >> >
> >> > - "make check-dev" Runs the tests under the newest supported
> >> >   dependencies using whatever version of Python the user happens to
> have
> >> >   installed. This is a quick convenience check that does not map to
> any
> >> >   particular GitLab test.
> >> >
> >> > (Note! check-dev may be busted on Fedora 41 and bleeding edge versions
> >> > of setuptools. That's unrelated to this patch and I'll address it
> >> > separately and soon. Thank you for your patience, --mgmt)
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >>
> >> Let's mention this is a step towards having "make check" run the static
> >> analysis we want developers to run, but we're not there, yet.
> >>
> >
> > It both is and isn't. That we can now check qapi and the qapi sphinx
> > extensions from the same place as we do linting for python/ is sufficient
> > justification in and of itself, regardless of how we improve and
> integrate
> > this testing later on.
>
> Alright.
>
> >> > ---
> >> >  python/setup.cfg            |  1 +
> >> >  python/tests/minreqs.txt    | 21 +++++++++++++++++++++
> >> >  python/tests/qapi-flake8.sh |  4 ++++
> >> >  python/tests/qapi-isort.sh  |  6 ++++++
> >> >  python/tests/qapi-mypy.sh   |  2 ++
> >> >  python/tests/qapi-pylint.sh |  6 ++++++
> >> >  scripts/qapi/pylintrc       |  1 +
> >> >  7 files changed, 41 insertions(+)
> >> >  create mode 100755 python/tests/qapi-flake8.sh
> >> >  create mode 100755 python/tests/qapi-isort.sh
> >> >  create mode 100755 python/tests/qapi-mypy.sh
> >> >  create mode 100755 python/tests/qapi-pylint.sh
> >> >
> >> > diff --git a/python/setup.cfg b/python/setup.cfg
> >> > index cf5af7e6641..84d8a1fd30d 100644
> >> > --- a/python/setup.cfg
> >> > +++ b/python/setup.cfg
> >> > @@ -47,6 +47,7 @@ devel =
> >> >      urwid >= 2.1.2
> >> >      urwid-readline >= 0.13
> >> >      Pygments >= 2.9.0
> >> > +    sphinx >= 3.4.3
> >> >
> >> >  # Provides qom-fuse functionality
> >> >  fuse =
> >> > diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
> >> > index 19c0f5e4c50..94928936d44 100644
> >> > --- a/python/tests/minreqs.txt
> >> > +++ b/python/tests/minreqs.txt
> >> > @@ -11,6 +11,9 @@
> >> >  # When adding new dependencies, pin the very oldest non-yanked
> version
> >> >  # on PyPI that allows the test suite to pass.
> >> >
> >> > +# Dependencies for qapidoc/qapi_domain et al
> >> > +sphinx==3.4.3
> >> > +
> >> >  # Dependencies for the TUI addon (Required for successful linting)
> >> >  urwid==2.1.2
> >> >  urwid-readline==0.13
> >> > @@ -49,3 +52,21 @@ platformdirs==2.2.0
> >> >  toml==0.10.0
> >> >  tomlkit==0.10.1
> >> >  wrapt==1.14.0
> >> > +
> >> > +# Transitive sphinx dependencies
> >> > +Jinja2==2.7
> >> > +MarkupSafe==1.1.0
> >> > +alabaster==0.7.1
> >> > +babel==1.3
> >> > +docutils==0.12
> >> > +imagesize==0.5.0
> >> > +packaging==14.0
> >> > +pytz==2011b0
> >> > +requests==2.5.0
> >> > +snowballstemmer==1.1
> >> > +sphinxcontrib-applehelp==1.0.0
> >> > +sphinxcontrib-devhelp==1.0.0
> >> > +sphinxcontrib-htmlhelp==1.0.0
> >> > +sphinxcontrib-jsmath==1.0.0
> >> > +sphinxcontrib-qthelp==1.0.0
> >> > +sphinxcontrib-serializinghtml==1.0.0
> >>
> >> This wasn't there when I last saw this patch.  The previous patch also
> >> updates this file.  How did you decide which updates go where?  Or is
> >> this an accident?
> >>
> >
> > The previous patch pins dependencies that already existed, but we
> neglected
> > to pin in this file. It's fixing an existing oversight.
> >
> > This patch adds a bunch of new pinned dependencies for Sphinx, which we
> > need for type-checking Sphinx extensions.
>
> So... the previous patch fixes existing tests, and this one extends
> their coverage to the modern parts of docs/sphinx/.  Correct?
>

Yep.


>
> Which tests exactly?  I just asked that on the previous patch.
>

minreqs.txt concerns only make check-minreqs, but the shell script tests
that just invoke a linter concern all launch avenues: make check, make
check-dev, make check-tox, make check-minreqs.


>
> [...]
>
> >> > diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
> >> > index d24eece7411..e16283ada3d 100644
> >> > --- a/scripts/qapi/pylintrc
> >> > +++ b/scripts/qapi/pylintrc
> >> > @@ -19,6 +19,7 @@ disable=consider-using-f-string,
> >> >          too-many-instance-attributes,
> >> >          too-many-positional-arguments,
> >> >          too-many-statements,
> >> > +        unknown-option-value,
> >> >          useless-option-value,
> >> >
> >> >  [REPORTS]
> >>
> >> This wasn't there when I last saw this patch.  PATCH 1 also updates this
> >> file.  How did you decide which updates go where?  Or is this an
> >> accident?
> >
> >
> > I didn't add the Sphinx extensions last time you saw this series, so
> that's
> > new. This winds up being needed to tolerate the "too many positional
> > arguments" option which only applies to newer pylint versions - older
> > versions will complain about the option being unrecognized. In order to
> > continue allowing a wide version of pylint versions, we need this option.
>
> Got it.  Worth a comment?
>

Yep, I can update the commit message.

[-- Attachment #2: Type: text/html, Size: 10082 bytes --]

[PATCH 5/5] qapi: delete un-needed python static analysis configs

[John Snow]

The pylint config is being left in place because the settings differ
enough from the python/ directory settings that we need a chit-chat on
how to merge them O:-)

Everything else can go.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/.flake8    | 3 ---
 scripts/qapi/.isort.cfg | 7 -------
 scripts/qapi/mypy.ini   | 4 ----
 3 files changed, 14 deletions(-)
 delete mode 100644 scripts/qapi/.flake8
 delete mode 100644 scripts/qapi/.isort.cfg
 delete mode 100644 scripts/qapi/mypy.ini

diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
deleted file mode 100644
index a873ff67309..00000000000
--- a/scripts/qapi/.flake8
+++ /dev/null
@@ -1,3 +0,0 @@
-[flake8]
-# Prefer pylint's bare-except checks to flake8's
-extend-ignore = E722
diff --git a/scripts/qapi/.isort.cfg b/scripts/qapi/.isort.cfg
deleted file mode 100644
index 643caa1fbd6..00000000000
--- a/scripts/qapi/.isort.cfg
+++ /dev/null
@@ -1,7 +0,0 @@
-[settings]
-force_grid_wrap=4
-force_sort_within_sections=True
-include_trailing_comma=True
-line_length=72
-lines_after_imports=2
-multi_line_output=3
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
deleted file mode 100644
index 8109470a031..00000000000
--- a/scripts/qapi/mypy.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[mypy]
-strict = True
-disallow_untyped_calls = False
-python_version = 3.8
-- 
2.48.1

Re: [PATCH 5/5] qapi: delete un-needed python static analysis configs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The pylint config is being left in place because the settings differ
> enough from the python/ directory settings that we need a chit-chat on
> how to merge them O:-)
>
> Everything else can go.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/.flake8    | 3 ---
>  scripts/qapi/.isort.cfg | 7 -------
>  scripts/qapi/mypy.ini   | 4 ----
>  3 files changed, 14 deletions(-)
>  delete mode 100644 scripts/qapi/.flake8
>  delete mode 100644 scripts/qapi/.isort.cfg
>  delete mode 100644 scripts/qapi/mypy.ini
>
> diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
> deleted file mode 100644
> index a873ff67309..00000000000
> --- a/scripts/qapi/.flake8
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -[flake8]
> -# Prefer pylint's bare-except checks to flake8's
> -extend-ignore = E722

python/setup.cfg has:

   [flake8]
   # Prefer pylint's bare-except checks to flake8's
   extend-ignore = E722
   exclude = __pycache__,

Good.

> diff --git a/scripts/qapi/.isort.cfg b/scripts/qapi/.isort.cfg
> deleted file mode 100644
> index 643caa1fbd6..00000000000
> --- a/scripts/qapi/.isort.cfg
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -[settings]
> -force_grid_wrap=4
> -force_sort_within_sections=True
> -include_trailing_comma=True
> -line_length=72
> -lines_after_imports=2
> -multi_line_output=3

python/setup.cfg has:

   [isort]
   force_grid_wrap=4
   force_sort_within_sections=True
   include_trailing_comma=True
   line_length=72
   lines_after_imports=2
   multi_line_output=3

Good.

> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> deleted file mode 100644
> index 8109470a031..00000000000
> --- a/scripts/qapi/mypy.ini
> +++ /dev/null
> @@ -1,4 +0,0 @@
> -[mypy]
> -strict = True
> -disallow_untyped_calls = False
> -python_version = 3.8

python/setup.cfg has:

   [mypy]
   strict = True
   python_version = 3.8
   warn_unused_configs = True
   namespace_packages = True
   warn_unused_ignores = False

Also a bunch of [mypy-FOO] sections that don't apply here.

You explained the differences in review of a prior iteration.  Recap:

} warn_unused_configs: Catches config values that aren't actually recognized
} or used. Was helpful once upon a time when re-arranging the Python
} directory to behave like a package to ensure that the conf files were
} working correctly.

Could this be culled now?

Hmm, according to mypy(1), strict implies warn-unused-configs.

The question does not block this patch.

} namespace_packages: Needed for the python/ directory structure (nested
} packages under a namespace, "qemu"). Doesn't impact scripts/qapi at all.
} Read up on PEP420 if you are curious. Details in commit message, see below
} if you're still curious.

mypy(1) makes me suspect this is the default.  If that's true across the
versions we care for, this could be culled.

Also does not block this patch.

} warn_unused_ignores: Needed once upon a time for cross-version mypy support
} where some versions would warn in some cases and others would not. Adding
} an ignore would effectively just invert which versions complained. Probably
} still needed, but it's hard to measure.

Harmless enough.

} python_version: Changes mypy behavior regardless of the invoking python
} interpreter to check the file as if it were to be executed by Python 3.8. I
} actually want to remove this value from setup.cfg but haven't yet. I
} removed it from the python-qemu-qmp repo and never added it for qapi.
} Removing it is actually probably correct as it will catch errors specific
} to various python versions we support, but there are some nits to iron out
} in my neck of the woods. This is a case where scripts/qapi/ is stricter
} than python/ :)
} (Not reasonable to solve for this series.)

Also present in the deleted file, so no change.

} lack of disallow_untyped_calls = False: I think this might be a remnant
} from when we gradually typed qapi; it's evidently no longer needed since
} qapi still checks fine without this affordance. The default under strict is
} True.

Fair enough.

Let's mention the differences in the commit message.  Here's my try:

    Since the previous commit, python/setup.cfg applies to scripts/qapi/
    as well.  Configuration files in scripts/qapi/ override
    python/setup.cfg.

    scripts/qapi/.flake8 and scripts/qapi/.isort.cfg actually match
    python/setup.cfg exactly, and can go.

    The differences between scripts/qapi/mypy.ini and python/setup.cfg
    are harmless: [list the differences, explain why they're harmless as
    long as you can keep it brief, and if not, fall back to "trust me"].
    So scripts/qapi/mypy.ini can go, too.

    The pylint config is being left in place because the settings differ
    enough from the python/ directory settings that we need a chit-chat on
    how to merge them O:-)

With something like that
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 5/5] qapi: delete un-needed python static analysis configs

[John Snow]

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

On Tue, Mar 25, 2025 at 4:05 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > The pylint config is being left in place because the settings differ
> > enough from the python/ directory settings that we need a chit-chat on
> > how to merge them O:-)
> >
> > Everything else can go.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/.flake8    | 3 ---
> >  scripts/qapi/.isort.cfg | 7 -------
> >  scripts/qapi/mypy.ini   | 4 ----
> >  3 files changed, 14 deletions(-)
> >  delete mode 100644 scripts/qapi/.flake8
> >  delete mode 100644 scripts/qapi/.isort.cfg
> >  delete mode 100644 scripts/qapi/mypy.ini
> >
> > diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
> > deleted file mode 100644
> > index a873ff67309..00000000000
> > --- a/scripts/qapi/.flake8
> > +++ /dev/null
> > @@ -1,3 +0,0 @@
> > -[flake8]
> > -# Prefer pylint's bare-except checks to flake8's
> > -extend-ignore = E722
>
> python/setup.cfg has:
>
>    [flake8]
>    # Prefer pylint's bare-except checks to flake8's
>    extend-ignore = E722
>    exclude = __pycache__,
>
> Good.
>
> > diff --git a/scripts/qapi/.isort.cfg b/scripts/qapi/.isort.cfg
> > deleted file mode 100644
> > index 643caa1fbd6..00000000000
> > --- a/scripts/qapi/.isort.cfg
> > +++ /dev/null
> > @@ -1,7 +0,0 @@
> > -[settings]
> > -force_grid_wrap=4
> > -force_sort_within_sections=True
> > -include_trailing_comma=True
> > -line_length=72
> > -lines_after_imports=2
> > -multi_line_output=3
>
> python/setup.cfg has:
>
>    [isort]
>    force_grid_wrap=4
>    force_sort_within_sections=True
>    include_trailing_comma=True
>    line_length=72
>    lines_after_imports=2
>    multi_line_output=3
>
> Good.
>
> > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> > deleted file mode 100644
> > index 8109470a031..00000000000
> > --- a/scripts/qapi/mypy.ini
> > +++ /dev/null
> > @@ -1,4 +0,0 @@
> > -[mypy]
> > -strict = True
> > -disallow_untyped_calls = False
> > -python_version = 3.8
>
> python/setup.cfg has:
>
>    [mypy]
>    strict = True
>    python_version = 3.8
>    warn_unused_configs = True
>    namespace_packages = True
>    warn_unused_ignores = False
>
> Also a bunch of [mypy-FOO] sections that don't apply here.
>
> You explained the differences in review of a prior iteration.  Recap:
>
> } warn_unused_configs: Catches config values that aren't actually
> recognized
> } or used. Was helpful once upon a time when re-arranging the Python
> } directory to behave like a package to ensure that the conf files were
> } working correctly.
>
> Could this be culled now?
>

Maybe!


>
> Hmm, according to mypy(1), strict implies warn-unused-configs.
>
> The question does not block this patch.
>

Send me a patch to drop it O:-)


>
> } namespace_packages: Needed for the python/ directory structure (nested
> } packages under a namespace, "qemu"). Doesn't impact scripts/qapi at all.
> } Read up on PEP420 if you are curious. Details in commit message, see
> below
> } if you're still curious.
>
> mypy(1) makes me suspect this is the default.  If that's true across the
> versions we care for, this could be culled.
>
> Also does not block this patch.
>

It definitely wasn't once upon a time. It may still not be true on the
oldest mypy we currently support. We don't have a clear policy for what
versions of python libraries we need to support - this is a muddy, gray
area. So far I just try to avoid breaking support on older versions
needlessly, but I don't have an upgrade policy.

If we want to integrate this directly into make check, we'll likely need to
formalize this policy.


>
> } warn_unused_ignores: Needed once upon a time for cross-version mypy
> support
> } where some versions would warn in some cases and others would not. Adding
> } an ignore would effectively just invert which versions complained.
> Probably
> } still needed, but it's hard to measure.
>
> Harmless enough.
>
> } python_version: Changes mypy behavior regardless of the invoking python
> } interpreter to check the file as if it were to be executed by Python
> 3.8. I
> } actually want to remove this value from setup.cfg but haven't yet. I
> } removed it from the python-qemu-qmp repo and never added it for qapi.
> } Removing it is actually probably correct as it will catch errors specific
> } to various python versions we support, but there are some nits to iron
> out
> } in my neck of the woods. This is a case where scripts/qapi/ is stricter
> } than python/ :)
> } (Not reasonable to solve for this series.)
>
> Also present in the deleted file, so no change.
>
> } lack of disallow_untyped_calls = False: I think this might be a remnant
> } from when we gradually typed qapi; it's evidently no longer needed since
> } qapi still checks fine without this affordance. The default under strict
> is
> } True.
>
> Fair enough.
>
> Let's mention the differences in the commit message.  Here's my try:
>
>     Since the previous commit, python/setup.cfg applies to scripts/qapi/
>     as well.  Configuration files in scripts/qapi/ override
>     python/setup.cfg.
>
>     scripts/qapi/.flake8 and scripts/qapi/.isort.cfg actually match
>     python/setup.cfg exactly, and can go.
>
>     The differences between scripts/qapi/mypy.ini and python/setup.cfg
>     are harmless: [list the differences, explain why they're harmless as
>     long as you can keep it brief, and if not, fall back to "trust me"].
>     So scripts/qapi/mypy.ini can go, too.
>
>     The pylint config is being left in place because the settings differ
>     enough from the python/ directory settings that we need a chit-chat on
>     how to merge them O:-)
>
> With something like that
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>

okey-dokey, let me integrate this feedback and I'll re-send, but I'm going
to wait until we hash everything else out - you had some questions on other
bits in this series.

[-- Attachment #2: Type: text/html, Size: 7806 bytes --]

Re: [PATCH 5/5] qapi: delete un-needed python static analysis configs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Mar 25, 2025 at 4:05 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > The pylint config is being left in place because the settings differ
>> > enough from the python/ directory settings that we need a chit-chat on
>> > how to merge them O:-)
>> >
>> > Everything else can go.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>

[...]

>> > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
>> > deleted file mode 100644
>> > index 8109470a031..00000000000
>> > --- a/scripts/qapi/mypy.ini
>> > +++ /dev/null
>> > @@ -1,4 +0,0 @@
>> > -[mypy]
>> > -strict = True
>> > -disallow_untyped_calls = False
>> > -python_version = 3.8
>>
>> python/setup.cfg has:
>>
>>    [mypy]
>>    strict = True
>>    python_version = 3.8
>>    warn_unused_configs = True
>>    namespace_packages = True
>>    warn_unused_ignores = False
>>
>> Also a bunch of [mypy-FOO] sections that don't apply here.
>>
>> You explained the differences in review of a prior iteration.  Recap:
>>
>> } warn_unused_configs: Catches config values that aren't actually recognized
>> } or used. Was helpful once upon a time when re-arranging the Python
>> } directory to behave like a package to ensure that the conf files were
>> } working correctly.
>>
>> Could this be culled now?
>>
>
> Maybe!
>
>
>>
>> Hmm, according to mypy(1), strict implies warn-unused-configs.
>>
>> The question does not block this patch.
>>
>
> Send me a patch to drop it O:-)

Done:

    [PATCH] python: Drop redundant warn_unused_configs = True
    Message-ID: <20250326071203.238931-1-armbru@redhat.com>

>> } namespace_packages: Needed for the python/ directory structure (nested
>> } packages under a namespace, "qemu"). Doesn't impact scripts/qapi at all.
>> } Read up on PEP420 if you are curious. Details in commit message, see below
>> } if you're still curious.
>>
>> mypy(1) makes me suspect this is the default.  If that's true across the
>> versions we care for, this could be culled.
>>
>> Also does not block this patch.
>>
>
> It definitely wasn't once upon a time. It may still not be true on the
> oldest mypy we currently support. We don't have a clear policy for what
> versions of python libraries we need to support - this is a muddy, gray
> area. So far I just try to avoid breaking support on older versions
> needlessly, but I don't have an upgrade policy.

So what is the oldest mypy we currently support?  Unknown, best effort
to fix any breakage we see?  Wouldn't quite match my dictionary's
definition of "support"...

> If we want to integrate this directly into make check, we'll likely need to
> formalize this policy.

My gut feeling: supporting old mypy isn't worth much (if any) trouble.

>> } warn_unused_ignores: Needed once upon a time for cross-version mypy support
>> } where some versions would warn in some cases and others would not. Adding
>> } an ignore would effectively just invert which versions complained. Probably
>> } still needed, but it's hard to measure.
>>
>> Harmless enough.
>>
>> } python_version: Changes mypy behavior regardless of the invoking python
>> } interpreter to check the file as if it were to be executed by Python 3.8. I
>> } actually want to remove this value from setup.cfg but haven't yet. I
>> } removed it from the python-qemu-qmp repo and never added it for qapi.
>> } Removing it is actually probably correct as it will catch errors specific
>> } to various python versions we support, but there are some nits to iron out
>> } in my neck of the woods. This is a case where scripts/qapi/ is stricter
>> } than python/ :)
>> } (Not reasonable to solve for this series.)
>>
>> Also present in the deleted file, so no change.
>>
>> } lack of disallow_untyped_calls = False: I think this might be a remnant
>> } from when we gradually typed qapi; it's evidently no longer needed since
>> } qapi still checks fine without this affordance. The default under strict is
>> } True.
>>
>> Fair enough.
>>
>> Let's mention the differences in the commit message.  Here's my try:
>>
>>     Since the previous commit, python/setup.cfg applies to scripts/qapi/
>>     as well.  Configuration files in scripts/qapi/ override
>>     python/setup.cfg.
>>
>>     scripts/qapi/.flake8 and scripts/qapi/.isort.cfg actually match
>>     python/setup.cfg exactly, and can go.
>>
>>     The differences between scripts/qapi/mypy.ini and python/setup.cfg
>>     are harmless: [list the differences, explain why they're harmless as
>>     long as you can keep it brief, and if not, fall back to "trust me"].
>>     So scripts/qapi/mypy.ini can go, too.
>>
>>     The pylint config is being left in place because the settings differ
>>     enough from the python/ directory settings that we need a chit-chat on
>>     how to merge them O:-)
>>
>> With something like that
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>
> okey-dokey, let me integrate this feedback and I'll re-send, but I'm going
> to wait until we hash everything else out - you had some questions on other
> bits in this series.

Thanks!

Re: [PATCH 5/5] qapi: delete un-needed python static analysis configs

[John Snow]

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

On Wed, Mar 26, 2025 at 3:18 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Tue, Mar 25, 2025 at 4:05 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > The pylint config is being left in place because the settings differ
> >> > enough from the python/ directory settings that we need a chit-chat on
> >> > how to merge them O:-)
> >> >
> >> > Everything else can go.
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> [...]
>
> >> > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> >> > deleted file mode 100644
> >> > index 8109470a031..00000000000
> >> > --- a/scripts/qapi/mypy.ini
> >> > +++ /dev/null
> >> > @@ -1,4 +0,0 @@
> >> > -[mypy]
> >> > -strict = True
> >> > -disallow_untyped_calls = False
> >> > -python_version = 3.8
> >>
> >> python/setup.cfg has:
> >>
> >>    [mypy]
> >>    strict = True
> >>    python_version = 3.8
> >>    warn_unused_configs = True
> >>    namespace_packages = True
> >>    warn_unused_ignores = False
> >>
> >> Also a bunch of [mypy-FOO] sections that don't apply here.
> >>
> >> You explained the differences in review of a prior iteration.  Recap:
> >>
> >> } warn_unused_configs: Catches config values that aren't actually
> recognized
> >> } or used. Was helpful once upon a time when re-arranging the Python
> >> } directory to behave like a package to ensure that the conf files were
> >> } working correctly.
> >>
> >> Could this be culled now?
> >>
> >
> > Maybe!
> >
> >
> >>
> >> Hmm, according to mypy(1), strict implies warn-unused-configs.
> >>
> >> The question does not block this patch.
> >>
> >
> > Send me a patch to drop it O:-)
>
> Done:
>
>     [PATCH] python: Drop redundant warn_unused_configs = True
>     Message-ID: <20250326071203.238931-1-armbru@redhat.com>
>

yay :)


>
> >> } namespace_packages: Needed for the python/ directory structure (nested
> >> } packages under a namespace, "qemu"). Doesn't impact scripts/qapi at
> all.
> >> } Read up on PEP420 if you are curious. Details in commit message, see
> below
> >> } if you're still curious.
> >>
> >> mypy(1) makes me suspect this is the default.  If that's true across the
> >> versions we care for, this could be culled.
> >>
> >> Also does not block this patch.
> >>
> >
> > It definitely wasn't once upon a time. It may still not be true on the
> > oldest mypy we currently support. We don't have a clear policy for what
> > versions of python libraries we need to support - this is a muddy, gray
> > area. So far I just try to avoid breaking support on older versions
> > needlessly, but I don't have an upgrade policy.
>
> So what is the oldest mypy we currently support?  Unknown, best effort
> to fix any breakage we see?  Wouldn't quite match my dictionary's
> definition of "support"...
>

from python/setup.cfg; mypy >= 1.4.0
from python/tests/minreqs.txt; mypy==1.4.0

This version is used for the minreqs test so I can be assured that we
haven't broken anything for the old pythons and the old type checkers. I
guarantee this version *will* work. I also guarantee the latest version
will work. I don't necessarily guarantee everything in between, but I make
my best effort.


>
> > If we want to integrate this directly into make check, we'll likely need
> to
> > formalize this policy.
>
> My gut feeling: supporting old mypy isn't worth much (if any) trouble.
>

Very extremely likely true! If people are fine with "you need PyPI and and
internet connection to run linter tests *at all*" we could move to the
latest version(s) (as supported per-python interpreter, at least) and be
done with it.

As I recall, some folks (Kevin Wolf?) had some requirements that they be
able to run iotests and so forth outside of configure, which makes this
kind of thing harder.

The tests grew out of ad-hoc scripts that had to work with whatever you
happened to have installed, though, and so it still supports a wide range
of linter versions for that reason. If I had my way, I'd stipulate we only
ever use a specific version that I pin quite close to the current bleeding
edge and then just periodically update the pin as needed for new features
or bugfixes or compatibility for shifting python interpreter support
windows.

If you want to discuss this and tackle it next release, I'm open to it,
I've just lost a *lot* of appetite for making new package version
requirement edicts in and around the build/doc/test system.


>
> >> } warn_unused_ignores: Needed once upon a time for cross-version mypy
> support
> >> } where some versions would warn in some cases and others would not.
> Adding
> >> } an ignore would effectively just invert which versions complained.
> Probably
> >> } still needed, but it's hard to measure.
> >>
> >> Harmless enough.
> >>
> >> } python_version: Changes mypy behavior regardless of the invoking
> python
> >> } interpreter to check the file as if it were to be executed by Python
> 3.8. I
> >> } actually want to remove this value from setup.cfg but haven't yet. I
> >> } removed it from the python-qemu-qmp repo and never added it for qapi.
> >> } Removing it is actually probably correct as it will catch errors
> specific
> >> } to various python versions we support, but there are some nits to
> iron out
> >> } in my neck of the woods. This is a case where scripts/qapi/ is
> stricter
> >> } than python/ :)
> >> } (Not reasonable to solve for this series.)
> >>
> >> Also present in the deleted file, so no change.
> >>
> >> } lack of disallow_untyped_calls = False: I think this might be a
> remnant
> >> } from when we gradually typed qapi; it's evidently no longer needed
> since
> >> } qapi still checks fine without this affordance. The default under
> strict is
> >> } True.
> >>
> >> Fair enough.
> >>
> >> Let's mention the differences in the commit message.  Here's my try:
> >>
> >>     Since the previous commit, python/setup.cfg applies to scripts/qapi/
> >>     as well.  Configuration files in scripts/qapi/ override
> >>     python/setup.cfg.
> >>
> >>     scripts/qapi/.flake8 and scripts/qapi/.isort.cfg actually match
> >>     python/setup.cfg exactly, and can go.
> >>
> >>     The differences between scripts/qapi/mypy.ini and python/setup.cfg
> >>     are harmless: [list the differences, explain why they're harmless as
> >>     long as you can keep it brief, and if not, fall back to "trust me"].
> >>     So scripts/qapi/mypy.ini can go, too.
> >>
> >>     The pylint config is being left in place because the settings differ
> >>     enough from the python/ directory settings that we need a chit-chat
> on
> >>     how to merge them O:-)
> >>
> >> With something like that
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >>
> >
> > okey-dokey, let me integrate this feedback and I'll re-send, but I'm
> going
> > to wait until we hash everything else out - you had some questions on
> other
> > bits in this series.
>
> Thanks!
>
>

[-- Attachment #2: Type: text/html, Size: 9623 bytes --]