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

[Markus Armbruster <armbru@redhat.com>]

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?