On Wed, Mar 26, 2025 at 2:13 AM Markus Armbruster <arm...@redhat.com> wrote:
> 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? > 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.
