John Snow <js...@redhat.com> writes: > On Tue, Mar 25, 2025 at 3:53 AM Markus Armbruster <arm...@redhat.com> wrote: > >> John Snow <js...@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 <js...@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?
