This is being done for the sake of unifying the linting and static type
analysis configurations between scripts/qapi and python/qemu/*.

With this change, the qapi module will now be checked by mypy, flake8,
pylint, isort etc under all python versions from 3.8 through 3.13 under
a variety of different dependency configurations in the GitLab testing
pipelines.

The tests can be run locally, as always:

> cd qemu.git/python
> make check-minreqs
> make check-tox
> make check-dev

"check-minreqs" is the must-pass GitLab test.
"check-tox" is the optional allowed-to-fail GitLab test.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 MAINTAINERS                                 |  2 +-
 docs/conf.py                                |  2 +-
 docs/sphinx/qapidoc.py                      |  6 ++---
 meson.build                                 | 28 ++++++++++-----------
 {scripts => python/qemu}/qapi/.flake8       |  0
 {scripts => python/qemu}/qapi/.isort.cfg    |  0
 {scripts => python/qemu}/qapi/__init__.py   |  0
 {scripts => python/qemu}/qapi/commands.py   |  0
 {scripts => python/qemu}/qapi/common.py     |  0
 {scripts => python/qemu}/qapi/error.py      |  0
 {scripts => python/qemu}/qapi/events.py     |  0
 {scripts => python/qemu}/qapi/expr.py       |  0
 {scripts => python/qemu}/qapi/gen.py        |  0
 {scripts => python/qemu}/qapi/introspect.py |  0
 {scripts => python/qemu}/qapi/main.py       |  0
 {scripts => python/qemu}/qapi/mypy.ini      |  0
 {scripts => python/qemu}/qapi/parser.py     |  0
 {scripts => python/qemu}/qapi/pylintrc      |  0
 {scripts => python/qemu}/qapi/schema.py     |  0
 {scripts => python/qemu}/qapi/source.py     |  0
 {scripts => python/qemu}/qapi/types.py      |  0
 {scripts => python/qemu}/qapi/visit.py      |  0
 python/setup.cfg                            |  1 +
 scripts/qapi-gen.py                         |  4 ++-
 tests/qapi-schema/meson.build               |  2 +-
 tests/qapi-schema/test-qapi.py              |  4 +--
 26 files changed, 26 insertions(+), 23 deletions(-)
 rename {scripts => python/qemu}/qapi/.flake8 (100%)
 rename {scripts => python/qemu}/qapi/.isort.cfg (100%)
 rename {scripts => python/qemu}/qapi/__init__.py (100%)
 rename {scripts => python/qemu}/qapi/commands.py (100%)
 rename {scripts => python/qemu}/qapi/common.py (100%)
 rename {scripts => python/qemu}/qapi/error.py (100%)
 rename {scripts => python/qemu}/qapi/events.py (100%)
 rename {scripts => python/qemu}/qapi/expr.py (100%)
 rename {scripts => python/qemu}/qapi/gen.py (100%)
 rename {scripts => python/qemu}/qapi/introspect.py (100%)
 rename {scripts => python/qemu}/qapi/main.py (100%)
 rename {scripts => python/qemu}/qapi/mypy.ini (100%)
 rename {scripts => python/qemu}/qapi/parser.py (100%)
 rename {scripts => python/qemu}/qapi/pylintrc (100%)
 rename {scripts => python/qemu}/qapi/schema.py (100%)
 rename {scripts => python/qemu}/qapi/source.py (100%)
 rename {scripts => python/qemu}/qapi/types.py (100%)
 rename {scripts => python/qemu}/qapi/visit.py (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3584d6a6c6d..1912940631d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3214,7 +3214,7 @@ F: tests/unit/test-qapi-*.c
 F: tests/unit/test-qmp-*.c
 F: tests/unit/test-visitor-serialization.c
 F: scripts/qapi-gen.py
-F: scripts/qapi/*
+F: python/qemu/qapi/*
 F: docs/sphinx/qapidoc.py
 F: docs/devel/qapi*
 T: git https://repo.or.cz/qemu/armbru.git qapi-next
diff --git a/docs/conf.py b/docs/conf.py
index 876f6768815..6600db976b3 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -46,7 +46,7 @@
 # Our extensions are in docs/sphinx; the qapidoc extension requires
 # the QAPI modules from scripts/.
 sys.path.insert(0, os.path.join(qemu_docdir, "sphinx"))
-sys.path.insert(0, os.path.join(qemu_docdir, "../scripts"))
+sys.path.insert(0, os.path.join(qemu_docdir, "../python"))
 
 
 # -- General configuration ------------------------------------------------
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 738b2450fb1..777fd1ac836 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -33,9 +33,9 @@
 from docutils import nodes
 from docutils.parsers.rst import Directive, directives
 from docutils.statemachine import ViewList
-from qapi.error import QAPIError, QAPISemError
-from qapi.gen import QAPISchemaVisitor
-from qapi.schema import QAPISchema
+from qemu.qapi.error import QAPIError, QAPISemError
+from qemu.qapi.gen import QAPISchemaVisitor
+from qemu.qapi.schema import QAPISchema
 
 from sphinx import addnodes
 from sphinx.directives.code import CodeBlock
diff --git a/meson.build b/meson.build
index fbda17c987e..f96c9bebe0c 100644
--- a/meson.build
+++ b/meson.build
@@ -3274,20 +3274,20 @@ genh += configure_file(output: 'config-host.h', configuration: config_host_data)
 hxtool = find_program('scripts/hxtool')
 shaderinclude = find_program('scripts/shaderinclude.py')
 qapi_gen = find_program('scripts/qapi-gen.py')
-qapi_gen_depends = [ meson.current_source_dir() / 'scripts/qapi/__init__.py',
-                     meson.current_source_dir() / 'scripts/qapi/commands.py',
-                     meson.current_source_dir() / 'scripts/qapi/common.py',
-                     meson.current_source_dir() / 'scripts/qapi/error.py',
-                     meson.current_source_dir() / 'scripts/qapi/events.py',
-                     meson.current_source_dir() / 'scripts/qapi/expr.py',
-                     meson.current_source_dir() / 'scripts/qapi/gen.py',
-                     meson.current_source_dir() / 'scripts/qapi/introspect.py',
-                     meson.current_source_dir() / 'scripts/qapi/main.py',
-                     meson.current_source_dir() / 'scripts/qapi/parser.py',
-                     meson.current_source_dir() / 'scripts/qapi/schema.py',
-                     meson.current_source_dir() / 'scripts/qapi/source.py',
-                     meson.current_source_dir() / 'scripts/qapi/types.py',
-                     meson.current_source_dir() / 'scripts/qapi/visit.py',
+qapi_gen_depends = [ meson.current_source_dir() / 'python/qemu/qapi/__init__.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/commands.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/common.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/error.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/events.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/expr.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/gen.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/introspect.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/main.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/parser.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/schema.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/source.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/types.py',
+                     meson.current_source_dir() / 'python/qemu/qapi/visit.py',
                      meson.current_source_dir() / 'scripts/qapi-gen.py'
 ]
 
diff --git a/scripts/qapi/.flake8 b/python/qemu/qapi/.flake8
similarity index 100%
rename from scripts/qapi/.flake8
rename to python/qemu/qapi/.flake8
diff --git a/scripts/qapi/.isort.cfg b/python/qemu/qapi/.isort.cfg
similarity index 100%
rename from scripts/qapi/.isort.cfg
rename to python/qemu/qapi/.isort.cfg
diff --git a/scripts/qapi/__init__.py b/python/qemu/qapi/__init__.py
similarity index 100%
rename from scripts/qapi/__init__.py
rename to python/qemu/qapi/__init__.py
diff --git a/scripts/qapi/commands.py b/python/qemu/qapi/commands.py
similarity index 100%
rename from scripts/qapi/commands.py
rename to python/qemu/qapi/commands.py
diff --git a/scripts/qapi/common.py b/python/qemu/qapi/common.py
similarity index 100%
rename from scripts/qapi/common.py
rename to python/qemu/qapi/common.py
diff --git a/scripts/qapi/error.py b/python/qemu/qapi/error.py
similarity index 100%
rename from scripts/qapi/error.py
rename to python/qemu/qapi/error.py
diff --git a/scripts/qapi/events.py b/python/qemu/qapi/events.py
similarity index 100%
rename from scripts/qapi/events.py
rename to python/qemu/qapi/events.py
diff --git a/scripts/qapi/expr.py b/python/qemu/qapi/expr.py
similarity index 100%
rename from scripts/qapi/expr.py
rename to python/qemu/qapi/expr.py
diff --git a/scripts/qapi/gen.py b/python/qemu/qapi/gen.py
similarity index 100%
rename from scripts/qapi/gen.py
rename to python/qemu/qapi/gen.py
diff --git a/scripts/qapi/introspect.py b/python/qemu/qapi/introspect.py
similarity index 100%
rename from scripts/qapi/introspect.py
rename to python/qemu/qapi/introspect.py
diff --git a/scripts/qapi/main.py b/python/qemu/qapi/main.py
similarity index 100%
rename from scripts/qapi/main.py
rename to python/qemu/qapi/main.py
diff --git a/scripts/qapi/mypy.ini b/python/qemu/qapi/mypy.ini
similarity index 100%
rename from scripts/qapi/mypy.ini
rename to python/qemu/qapi/mypy.ini
diff --git a/scripts/qapi/parser.py b/python/qemu/qapi/parser.py
similarity index 100%
rename from scripts/qapi/parser.py
rename to python/qemu/qapi/parser.py
diff --git a/scripts/qapi/pylintrc b/python/qemu/qapi/pylintrc
similarity index 100%
rename from scripts/qapi/pylintrc
rename to python/qemu/qapi/pylintrc
diff --git a/scripts/qapi/schema.py b/python/qemu/qapi/schema.py
similarity index 100%
rename from scripts/qapi/schema.py
rename to python/qemu/qapi/schema.py
diff --git a/scripts/qapi/source.py b/python/qemu/qapi/source.py
similarity index 100%
rename from scripts/qapi/source.py
rename to python/qemu/qapi/source.py
diff --git a/scripts/qapi/types.py b/python/qemu/qapi/types.py
similarity index 100%
rename from scripts/qapi/types.py
rename to python/qemu/qapi/types.py
diff --git a/scripts/qapi/visit.py b/python/qemu/qapi/visit.py
similarity index 100%
rename from scripts/qapi/visit.py
rename to python/qemu/qapi/visit.py
diff --git a/python/setup.cfg b/python/setup.cfg
index 58dba90f815..d1582f74ddd 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -28,6 +28,7 @@ packages =
     qemu.qmp
     qemu.machine
     qemu.utils
+    qemu.qapi
 
 [options.package_data]
 * = py.typed
diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
index f3518d29a54..42912c91716 100644
--- a/scripts/qapi-gen.py
+++ b/scripts/qapi-gen.py
@@ -11,9 +11,11 @@
 execution environment.
 """
 
+import os
 import sys
 
-from qapi import main
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
+from qemu.qapi import main
 
 if __name__ == '__main__':
     sys.exit(main.main())
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index 0f479d93170..080444d0cd4 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -1,5 +1,5 @@
 test_env = environment()
-test_env.set('PYTHONPATH', meson.project_source_root() / 'scripts')
+test_env.set('PYTHONPATH', meson.project_source_root() / 'python')
 test_env.set('PYTHONIOENCODING', 'utf-8')
 
 schemas = [
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 7e3f9f4aa1f..dcc0d949971 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -18,8 +18,8 @@
 import sys
 from io import StringIO
 
-from qapi.error import QAPIError
-from qapi.schema import QAPISchema, QAPISchemaVisitor
+from qemu.qapi.error import QAPIError
+from qemu.qapi.schema import QAPISchema, QAPISchemaVisitor
 
 
 class QAPISchemaTestVisitor(QAPISchemaVisitor):
-- 
2.45.0

John Snow <jsnow@redhat.com> writes:

> This is being done for the sake of unifying the linting and static type
> analysis configurations between scripts/qapi and python/qemu/*.
>
> With this change, the qapi module will now be checked by mypy, flake8,
> pylint, isort etc under all python versions from 3.8 through 3.13 under
> a variety of different dependency configurations in the GitLab testing
> pipelines.
>
> The tests can be run locally, as always:
>
>> cd qemu.git/python
>> make check-minreqs
>> make check-tox
>> make check-dev
>
> "check-minreqs" is the must-pass GitLab test.
> "check-tox" is the optional allowed-to-fail GitLab test.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

I don't understand why we have to keep Python code in its own directory
just to get it checked.  We wouldn't do that say for Rust code, would
we?  Anyway, if it's the price of checking, I'll pay[*].

[...]

> diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> index f3518d29a54..42912c91716 100644
> --- a/scripts/qapi-gen.py
> +++ b/scripts/qapi-gen.py
> @@ -11,9 +11,11 @@
>  execution environment.
>  """
>  
> +import os
>  import sys
>  
> -from qapi import main
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
> +from qemu.qapi import main
>  
>  if __name__ == '__main__':
>      sys.exit(main.main())

Suggest to use the opportunity to rename to just qapi-gen (no .py) and
chmod +x, possibly in a separate patch.

[...]


[*] Grudgingly.

On Fri, Aug 30, 2024 at 7:20 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This is being done for the sake of unifying the linting and static type
> > analysis configurations between scripts/qapi and python/qemu/*.
> >
> > With this change, the qapi module will now be checked by mypy, flake8,
> > pylint, isort etc under all python versions from 3.8 through 3.13 under
> > a variety of different dependency configurations in the GitLab testing
> > pipelines.
> >
> > The tests can be run locally, as always:
> >
> >> cd qemu.git/python
> >> make check-minreqs
> >> make check-tox
> >> make check-dev
> >
> > "check-minreqs" is the must-pass GitLab test.
> > "check-tox" is the optional allowed-to-fail GitLab test.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> I don't understand why we have to keep Python code in its own directory
> just to get it checked.  We wouldn't do that say for Rust code, would
> we?  Anyway, if it's the price of checking, I'll pay[*].
>

Gave Dan a related answer. For you, my explanation is:

- It's nice to have just one configuration for static analysis in just one
place
- It's nice to have that configuration follow python ecosystem norms
- It's nice to use standard python management tools to configure and test
the supported versions of static analysis tools, again kept in one place.
- Just moving the folder costs virtually nothing.
- Moving it here makes it easier for me to test the eventual integration
with make check in one place.
- I like being able to say that anything under `python/` is fiercely
guarded by high standards (and the gitlab pipelines) and everything else is
not. I consider this to be organizationally simple and easy to communicate.
i.e., I find it attractive to say that "python is maintained, scripts are
YMMV." I am *not* willing to maintain everything under `scripts/` with the
same level of effort I apply to `python/`. I think it's important to allow
people to commit low-development-cost scripts ("contrib quality") that they
can run from time to time and not everything needs to be held to a
crystalline perfect standard, but some stuff does.

If I'm being honest, I also just don't want to develop more testing
infrastructure and scaffolding to start picking up scattershot python
modules from elsewhere in the tree. I'd rather bring qapi into the fold and
then continue working on integrating `python/` static analysis tests to the
make check suite instead. I've spent enough time already writing and
carrying around my little ad-hoc static analysis scripts for qapi during
the strict typing conversion, and ensuring static analysis passes with
totally arbitrary versions of whatever tools the user happens to have
installed sounds like a colossal pain. I already have a system set up to do
all of the environment prep work so that it Just Works :tm: and is tested
across a large matrix of tooling versions to ensure it continues to work
both locally (for developer ease) and in the gitlab pipeline (for rigorous
testing) with both forms of test readily accessible in the local developer
environment: I'd deeply appreciate just being able to let that system do
what I designed it to do.

This series is 99% "converge on a static analysis configuration standard"
and 1% "move it into place so it starts being checked regularly." I think
that's worth a simple "git mv", honestly. Do we each lose some control over
our preferred standard of formatting? Yes, but we gain consistency and ease
of testing.

As for rust: I dunno! I imagine there are similar benefits there for
modeling things as standards compliant packages, too. I'm not doing rust
tooling right now, so I can't say.


>
> [...]
>
> > diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> > index f3518d29a54..42912c91716 100644
> > --- a/scripts/qapi-gen.py
> > +++ b/scripts/qapi-gen.py
> > @@ -11,9 +11,11 @@
> >  execution environment.
> >  """
> >
> > +import os
> >  import sys
> >
> > -from qapi import main
> > +sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
> > +from qemu.qapi import main
> >
> >  if __name__ == '__main__':
> >      sys.exit(main.main())
>
> Suggest to use the opportunity to rename to just qapi-gen (no .py) and
> chmod +x, possibly in a separate patch.
>
> [...]
>
>
> [*] Grudgingly.
>

Why the resistance? Is there some harm I've overlooked? This seems fairly
benign to me.

On Fri, Aug 30, 2024 at 02:22:50PM -0400, John Snow wrote:
> Gave Dan a related answer. For you, my explanation is:
> 
> - It's nice to have just one configuration for static analysis in just one
> place
> - It's nice to have that configuration follow python ecosystem norms
> - It's nice to use standard python management tools to configure and test
> the supported versions of static analysis tools, again kept in one place.
> - Just moving the folder costs virtually nothing.
> - Moving it here makes it easier for me to test the eventual integration
> with make check in one place.
> - I like being able to say that anything under `python/` is fiercely
> guarded by high standards (and the gitlab pipelines) and everything else is
> not. I consider this to be organizationally simple and easy to communicate.
> i.e., I find it attractive to say that "python is maintained, scripts are
> YMMV." I am *not* willing to maintain everything under `scripts/` with the
> same level of effort I apply to `python/`. I think it's important to allow
> people to commit low-development-cost scripts ("contrib quality") that they
> can run from time to time and not everything needs to be held to a
> crystalline perfect standard, but some stuff does.

FYI, I was NOT suggesting that you maintain anything under scripts/.

Rather I'm saying that if we want to apply python code standards, we
should (ultimately) apply them to all python code in the tree, and
that *ALL* maintainers and contributors should comply.

Consider our C standards - we don't apply them selectively to a subset
of the tree - we expect all maintainers to comply. I'd like us to have
the same be true of Python.

The only real issue we have with python standards is bringing existing
code upto par, before we can enable the checks.

Currently we have no easy way for other maintainers to enable their
python code be checked, without moving their code under python/ which
is undesirable IMHO.

If we move the python coding standards to meson.build, such that apply
to all of the source tree, and then exclude everything except python/,
we make it easier for other maintainers to bring stuff upto par. All
need do at that point is remove the exclusion rule for files incrementally
as they fix them.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Sep 2, 2024, 4:51 AM Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Fri, Aug 30, 2024 at 02:22:50PM -0400, John Snow wrote:
> > Gave Dan a related answer. For you, my explanation is:
> >
> > - It's nice to have just one configuration for static analysis in just
> one
> > place
> > - It's nice to have that configuration follow python ecosystem norms
> > - It's nice to use standard python management tools to configure and test
> > the supported versions of static analysis tools, again kept in one place.
> > - Just moving the folder costs virtually nothing.
> > - Moving it here makes it easier for me to test the eventual integration
> > with make check in one place.
> > - I like being able to say that anything under `python/` is fiercely
> > guarded by high standards (and the gitlab pipelines) and everything else
> is
> > not. I consider this to be organizationally simple and easy to
> communicate.
> > i.e., I find it attractive to say that "python is maintained, scripts are
> > YMMV." I am *not* willing to maintain everything under `scripts/` with
> the
> > same level of effort I apply to `python/`. I think it's important to
> allow
> > people to commit low-development-cost scripts ("contrib quality") that
> they
> > can run from time to time and not everything needs to be held to a
> > crystalline perfect standard, but some stuff does.
>
> FYI, I was NOT suggesting that you maintain anything under scripts/.
>
> Rather I'm saying that if we want to apply python code standards, we
> should (ultimately) apply them to all python code in the tree, and
> that *ALL* maintainers and contributors should comply.
>

Right, it's just that the level of care to bring everything up to that
standard is currently more effort than I'd like to spend.

Ideally everything WOULD be on the same standard, but...


> Consider our C standards - we don't apply them selectively to a subset
> of the tree - we expect all maintainers to comply. I'd like us to have
> the same be true of Python.
>
> The only real issue we have with python standards is bringing existing
> code upto par, before we can enable the checks.
>

Yeah, exactly. It took me long enough to do this with qmp, machine, qom and
qapi. It'll take aeons for iotests.

It's just not on my radar right now as a priority.

More tractable: protect everything used for the build to that high
standard, worry about the rest later.


> Currently we have no easy way for other maintainers to enable their
> python code be checked, without moving their code under python/ which
> is undesirable IMHO.
>

It's possible I can extend a "lower standard" to everything outside of
python/ to help enforce the basics, without mandatory typing.

(I did add an iotest checker to python/ tests that enforces a lower
standard to those tests. I intend to remove iotests 297 once I get a
top-level "make check-python" instituted.)

I don't think one unified standard is something we can do in the near term.
Mechanically I think it's easy, but in practice pushing all the linting
patches through has been like pulling teeth and I've lost appetite for
pursuing it beyond whatever is *super duper important*.

In retrospect, 3.6 was too early to try adding static typing. I think I
won't bother for anything else until we have 3.9 as a minimum. After the
pushback last time, I doubt I'll make the push any time soon unless
something really urgent comes up.


> If we move the python coding standards to meson.build, such that apply
> to all of the source tree, and then exclude everything except python/,
> we make it easier for other maintainers to bring stuff upto par. All
> need do at that point is remove the exclusion rule for files incrementally
> as they fix them.
>

Not against this in the future, I just think there are some higher priority
items to take care of first:

* Begin protecting qapi with the existing python static analysis regime
* Add a "make check-python" target to the top level makefile that can set
up the environment it needs on-demand and handles what python/'s "make
check-minreqs" currently does (incl iotest 297)
* Drop python/qemu/qmp from the tree in favor of the standalone package.
There are mkvenv changes I'm currently developing here to make this happen.
It's a little involved due to the wide spread of setuptools versions on
platforms we support.

Once I drill through these, I'm more than happy to try to support/maintain
everything else to a lower baseline of care. Maybe this winter, if you'd be
willing to volunteer some review time for the push.


>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

On Mon, 2 Sept 2024 at 09:51, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Fri, Aug 30, 2024 at 02:22:50PM -0400, John Snow wrote:
> > Gave Dan a related answer. For you, my explanation is:
> >
> > - It's nice to have just one configuration for static analysis in just one
> > place
> > - It's nice to have that configuration follow python ecosystem norms
> > - It's nice to use standard python management tools to configure and test
> > the supported versions of static analysis tools, again kept in one place.
> > - Just moving the folder costs virtually nothing.
> > - Moving it here makes it easier for me to test the eventual integration
> > with make check in one place.
> > - I like being able to say that anything under `python/` is fiercely
> > guarded by high standards (and the gitlab pipelines) and everything else is
> > not. I consider this to be organizationally simple and easy to communicate.
> > i.e., I find it attractive to say that "python is maintained, scripts are
> > YMMV." I am *not* willing to maintain everything under `scripts/` with the
> > same level of effort I apply to `python/`. I think it's important to allow
> > people to commit low-development-cost scripts ("contrib quality") that they
> > can run from time to time and not everything needs to be held to a
> > crystalline perfect standard, but some stuff does.
>
> FYI, I was NOT suggesting that you maintain anything under scripts/.
>
> Rather I'm saying that if we want to apply python code standards, we
> should (ultimately) apply them to all python code in the tree, and
> that *ALL* maintainers and contributors should comply.

To be clear up front: this is more of a tangential musing than anything
else, and I'm not making a concrete proposal wrt this patchset.

scripts/, like contrib/, is a bit of an odd directory structure from my
point of view. I don't think something happening to be a script
ought to affect where we put it in our source tree -- scripts/
is really more like "tooling used at build time and by developers".
If we had a user-facing utility that happened to be written in
python or in shell, that ought to go in tools/ I think, for instance.
To the extent that our standards are lower in scripts/ it should
be because we know that the audience and usage pattern for those
utilities is limited and so it's not necessarily worth the effort to
bring them up to the standards we'd apply to user-facing code.
(Similarly we're a bit sloppier in C code in tests/ than we are
in C code that goes into QEMU proper.)

IMHO half of contrib/ ought to be in tools/, and for contrib/
I'm particularly not a fan of having a bit of the directory tree
that's labelled as "put stuff here that we don't care about". Either
we care about it and it should go in the appropriate place in
the tree for what it is, or else we don't care about it and it
should live out-of-tree...

-- PMM

On Fri, Aug 30, 2024 at 01:20:35PM +0200, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
> > This is being done for the sake of unifying the linting and static type
> > analysis configurations between scripts/qapi and python/qemu/*.
> >
> > With this change, the qapi module will now be checked by mypy, flake8,
> > pylint, isort etc under all python versions from 3.8 through 3.13 under
> > a variety of different dependency configurations in the GitLab testing
> > pipelines.
> >
> > The tests can be run locally, as always:
> >
> >> cd qemu.git/python
> >> make check-minreqs
> >> make check-tox
> >> make check-dev
> >
> > "check-minreqs" is the must-pass GitLab test.
> > "check-tox" is the optional allowed-to-fail GitLab test.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> 
> I don't understand why we have to keep Python code in its own directory
> just to get it checked.  We wouldn't do that say for Rust code, would
> we?  Anyway, if it's the price of checking, I'll pay[*].

The 'check-tox' target is defined in python/Makefile, and is
written to only check code below that location, which is a
somewhat arbitrary choice.

Having this in "make" at all is a bit outdated. Ideally all
the targets in python/Makefile should be converted into meson
targets and/or tests, in a "python" suite.

IOW, we should make 'check-tox' a target in meson.build at the
top level, and have it check any .py file in the source tree,
with an exclude list for files we know are not "clean" yet,
so we don't have to move stuff around as we clean up individual
files.

> 
> [...]
> 
> > diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> > index f3518d29a54..42912c91716 100644
> > --- a/scripts/qapi-gen.py
> > +++ b/scripts/qapi-gen.py
> > @@ -11,9 +11,11 @@
> >  execution environment.
> >  """
> >  
> > +import os
> >  import sys
> >  
> > -from qapi import main
> > +sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
> > +from qemu.qapi import main
> >  
> >  if __name__ == '__main__':
> >      sys.exit(main.main())
> 
> Suggest to use the opportunity to rename to just qapi-gen (no .py) and
> chmod +x, possibly in a separate patch.
> 
> [...]
> 
> 
> [*] Grudgingly.
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Fri, Aug 30, 2024 at 7:29 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, Aug 30, 2024 at 01:20:35PM +0200, Markus Armbruster wrote:
> > John Snow <jsnow@redhat.com> writes:
> >
> > > This is being done for the sake of unifying the linting and static type
> > > analysis configurations between scripts/qapi and python/qemu/*.
> > >
> > > With this change, the qapi module will now be checked by mypy, flake8,
> > > pylint, isort etc under all python versions from 3.8 through 3.13 under
> > > a variety of different dependency configurations in the GitLab testing
> > > pipelines.
> > >
> > > The tests can be run locally, as always:
> > >
> > >> cd qemu.git/python
> > >> make check-minreqs
> > >> make check-tox
> > >> make check-dev
> > >
> > > "check-minreqs" is the must-pass GitLab test.
> > > "check-tox" is the optional allowed-to-fail GitLab test.
> > >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > I don't understand why we have to keep Python code in its own directory
> > just to get it checked.  We wouldn't do that say for Rust code, would
> > we?  Anyway, if it's the price of checking, I'll pay[*].
>
> The 'check-tox' target is defined in python/Makefile, and is
> written to only check code below that location, which is a
> somewhat arbitrary choice.
>
> Having this in "make" at all is a bit outdated. Ideally all
> the targets in python/Makefile should be converted into meson
> targets and/or tests, in a "python" suite.
>

Yes, ideally, but there are still gaps between the python tooling ecosystem
and our own conventions in the QEMU source tree. Saying "everything under
python/ should adhere to python ecosystem conventions" was just an
extremely convenient way to make python tooling not throw a hissy fit over
stuff in that directory. The "make" targets in that directory are really
just simple script invocations that we don't really need "make" for at all,
it's just so I don't have to re-explain how to invoke the tests as they
stand. I don't think it's "outdated" in that sense, it's just a nice little
syntactic shorthand using a tool people are familiar with - not too far out
from what we do for the VM tests.

One of the barriers to implementing this in "make check" et al is setting
up the package installation needed for e.g. mypy and the optional feature
packages (like urwid, fusepy, etc) in a way that is unobtrusive to the
build system. The build-time configure venv was a step in that direction,
but the work remains unfinished.

I am in the process (right now!) of setting up mkvenv to install the
in-tree python packages to the configure venv - one of the barriers *here*
is that this is a little bit slow with older setuptools, and due to very
tumultuous changes in Python packaging in the last several years (fallout
from PEP-517, PEP-518 and PEP-660) is that it is taking me a good long
while to ensure it's rock solid across all of our supported platforms while
supporting isolated offline builds. (Python stuff was really, really,
really not designed to cope very well with that restriction, I have learned
in the last few years.)

Another reason for the decision to treat the python/ subdirectory as a
"mini-repository" was purely for sake of ease with splitting out
subpackages that became useful beyond the confines of our castle walls:
qemu.qmp in particular. By structuring everything here like an upstream
package, it becomes pretty trivial to fork things out into standalone
packages, because they're already structured identically to how standalone
packages would be.

TLDR: "It makes my life easier to follow python ecosystem norms in this
directory, but I still want to integrate it more formally to the QEMU build
system and am working on that."


>
> IOW, we should make 'check-tox' a target in meson.build at the
> top level, and have it check any .py file in the source tree,
> with an exclude list for files we know are not "clean" yet,
> so we don't have to move stuff around as we clean up individual
> files.
>

Maybe someday.

Right now, I consider anything under python/ to be "maintained" for the
python ecosystem and anything outside of this to be "good luck!".

I do not volunteer to apply the same level of effort I do for python/ to
*every last python script in the tree*. The standards I apply in python/
are vastly more strict than those that could apply to everything else; I
don't think I could handle the workload of polishing every last python
thing in the tree up to the same standard. qemu.qmp, qemu.machine and qapi
are just vastly more important than the majority of one-off scripts that
automate niche tasks I have no domain expertise in.

For any python scripts with import dependencies on other in-tree modules,
it is useful to structure those imported modules as normal packages so that
mypy, pylint, etc can function with minimal fuss and without needing lots
of prone-to-breakage environment hacks and custom launcher scripts to make
them work. Anything that *doesn't* need to import anything else from the
tree is relatively easy to check where it lives, I just don't do that at
the moment. It's possible we could make a separate test that applies a much
lower bar of type checking and linting that really only catches *hard*
errors and applies to everything in the tree automatically, but I don't
think I want to lower the bar for the stuff already in python/.

That said, I *do* want to move the python tests into our usual build
testing infrastructure, there's just some more work to do with mkvenv to
bridge the gap, which I'm working on currently.


>
> >
> > [...]
> >
> > > diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> > > index f3518d29a54..42912c91716 100644
> > > --- a/scripts/qapi-gen.py
> > > +++ b/scripts/qapi-gen.py
> > > @@ -11,9 +11,11 @@
> > >  execution environment.
> > >  """
> > >
> > > +import os
> > >  import sys
> > >
> > > -from qapi import main
> > > +sys.path.append(os.path.join(os.path.dirname(__file__), '..',
> 'python'))
> > > +from qemu.qapi import main
> > >
> > >  if __name__ == '__main__':
> > >      sys.exit(main.main())
> >
> > Suggest to use the opportunity to rename to just qapi-gen (no .py) and
> > chmod +x, possibly in a separate patch.
> >
> > [...]
> >
> >
> > [*] Grudgingly.
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>