Re: [PATCH 7/8] python/qapi: move scripts/qapi to python/qemu/qapi

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4537 bytes --]

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.

[-- Attachment #2: Type: text/html, Size: 5824 bytes --]