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