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

[Markus Armbruster <armbru@redhat.com>]

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?