[PATCH 0/8] move qapi under python/qemu/

[PATCH 0/8] move qapi under python/qemu/

[John Snow]

Move the QAPI generator module to python/qemu/qapi so that it's checked
by the same standard linter/static analysis regime as the other python
code.

This has the additional side-effect of marking me as a co-maintainer of
the QAPI generator.

John Snow (8):
  python/qapi: correct re.Match type hints for 3.13
  python/qapi: change "FIXME" to "TODO"
  python/qapi: add pylint pragmas
  python/qapi: remove outdated pragmas
  python/qapi: ignore missing docstrings in pylint
  python: allow short names for variables on older pylint
  python/qapi: move scripts/qapi to python/qemu/qapi
  python/qapi: remove redundant linter configuration

 MAINTAINERS                                 |  2 +-
 docs/conf.py                                |  2 +-
 docs/sphinx/qapidoc.py                      |  6 +-
 meson.build                                 | 28 +++++-----
 {scripts => python/qemu}/qapi/__init__.py   |  0
 {scripts => python/qemu}/qapi/commands.py   |  4 +-
 {scripts => python/qemu}/qapi/common.py     | 12 +++-
 {scripts => python/qemu}/qapi/error.py      |  0
 {scripts => python/qemu}/qapi/events.py     |  4 +-
 {scripts => python/qemu}/qapi/expr.py       |  3 +
 {scripts => python/qemu}/qapi/gen.py        |  4 +-
 {scripts => python/qemu}/qapi/introspect.py |  3 +
 {scripts => python/qemu}/qapi/main.py       |  2 +
 {scripts => python/qemu}/qapi/parser.py     |  9 ++-
 {scripts => python/qemu}/qapi/schema.py     |  2 +-
 {scripts => python/qemu}/qapi/source.py     |  2 +
 {scripts => python/qemu}/qapi/types.py      |  2 +
 {scripts => python/qemu}/qapi/visit.py      |  3 +
 python/setup.cfg                            | 22 ++++----
 scripts/qapi-gen.py                         |  4 +-
 scripts/qapi/.flake8                        |  3 -
 scripts/qapi/.isort.cfg                     |  7 ---
 scripts/qapi/mypy.ini                       |  4 --
 scripts/qapi/pylintrc                       | 61 ---------------------
 tests/qapi-schema/meson.build               |  2 +-
 tests/qapi-schema/test-qapi.py              |  4 +-
 26 files changed, 78 insertions(+), 117 deletions(-)
 rename {scripts => python/qemu}/qapi/__init__.py (100%)
 rename {scripts => python/qemu}/qapi/commands.py (99%)
 rename {scripts => python/qemu}/qapi/common.py (96%)
 rename {scripts => python/qemu}/qapi/error.py (100%)
 rename {scripts => python/qemu}/qapi/events.py (98%)
 rename {scripts => python/qemu}/qapi/expr.py (99%)
 rename {scripts => python/qemu}/qapi/gen.py (99%)
 rename {scripts => python/qemu}/qapi/introspect.py (99%)
 rename {scripts => python/qemu}/qapi/main.py (98%)
 rename {scripts => python/qemu}/qapi/parser.py (99%)
 rename {scripts => python/qemu}/qapi/schema.py (99%)
 rename {scripts => python/qemu}/qapi/source.py (98%)
 rename {scripts => python/qemu}/qapi/types.py (99%)
 rename {scripts => python/qemu}/qapi/visit.py (99%)
 delete mode 100644 scripts/qapi/.flake8
 delete mode 100644 scripts/qapi/.isort.cfg
 delete mode 100644 scripts/qapi/mypy.ini
 delete mode 100644 scripts/qapi/pylintrc

-- 
2.45.0

[PATCH 1/8] python/qapi: correct re.Match type hints for 3.13

[John Snow]

typing.Match was removed in Python 3.13, so we need to use re.Match
instead. However, Python 3.8 doesn't support using re.Match as a type
hint directly, so we need a conditional for now.

The import is written oddly so that "Match" is explicitly re-exported
for re-use by other modules. mypy will complain otherwise.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/common.py | 10 +++++++++-
 scripts/qapi/parser.py |  3 +--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 737b059e629..444b3acf53a 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -12,16 +12,24 @@
 # See the COPYING file in the top-level directory.
 
 import re
+import sys
 from typing import (
     Any,
     Dict,
-    Match,
     Optional,
     Sequence,
     Union,
 )
 
 
+if sys.version_info < (3, 9):
+    # typing.Match was removed in 3.13,
+    # but it's still a necessity in 3.8.
+    from typing import \
+        Match as Match  # pylint: disable=useless-import-alias
+else:
+    Match = re.Match
+
 #: Magic string that gets removed along with all space to its right.
 EATSPACE = '\033EATSPACE.'
 POINTER_SUFFIX = ' *' + EATSPACE
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index adc85b5b394..9a42b119131 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -23,13 +23,12 @@
     Dict,
     List,
     Mapping,
-    Match,
     Optional,
     Set,
     Union,
 )
 
-from .common import must_match
+from .common import Match, must_match
 from .error import QAPISemError, QAPISourceError
 from .source import QAPISourceInfo
 
-- 
2.45.0

Re: [PATCH 1/8] python/qapi: correct re.Match type hints for 3.13

[Philippe Mathieu-Daudé]

On 20/8/24 02:23, John Snow wrote:
> typing.Match was removed in Python 3.13, so we need to use re.Match
> instead. However, Python 3.8 doesn't support using re.Match as a type
> hint directly, so we need a conditional for now.
> 
> The import is written oddly so that "Match" is explicitly re-exported
> for re-use by other modules. mypy will complain otherwise.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   scripts/qapi/common.py | 10 +++++++++-
>   scripts/qapi/parser.py |  3 +--
>   2 files changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/8] python/qapi: change "FIXME" to "TODO"

[John Snow]

qemu.git/python/setup.cfg disallows checking in any code with "XXX",
"FIXME" or "TODO" in the comments. Soften the restriction to only
prohibit "FIXME", and change the two occurrences of "FIXME" in qapi to
read "TODO" instead.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/setup.cfg         | 5 +++++
 scripts/qapi/commands.py | 2 +-
 scripts/qapi/events.py   | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/python/setup.cfg b/python/setup.cfg
index 3b4e2cc5501..72b58c98c99 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -169,6 +169,11 @@ ignore-signatures=yes
 # TODO: Remove after we opt in to Pylint 2.8.3. See commit msg.
 min-similarity-lines=6
 
+[pylint.miscellaneous]
+
+# forbid FIXME/XXX comments, allow TODO.
+notes=FIXME,
+      XXX,
 
 [isort]
 force_grid_wrap=4
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 79951a841f5..cffed6cd3ba 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -385,7 +385,7 @@ def visit_command(self,
                       coroutine: bool) -> None:
         if not gen:
             return
-        # FIXME: If T is a user-defined type, the user is responsible
+        # TODO: If T is a user-defined type, the user is responsible
         # for making this work, i.e. to make T's condition the
         # conjunction of the T-returning commands' conditions.  If T
         # is a built-in type, this isn't possible: the
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index d1f639981a9..36dc0c50c78 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -84,7 +84,7 @@ def gen_event_send(name: str,
                    boxed: bool,
                    event_enum_name: str,
                    event_emit: str) -> str:
-    # FIXME: Our declaration of local variables (and of 'errp' in the
+    # TODO: Our declaration of local variables (and of 'errp' in the
     # parameter list) can collide with exploded members of the event's
     # data type passed in as parameters.  If this collision ever hits in
     # practice, we can rename our local variables with a leading _ prefix,
-- 
2.45.0

Re: [PATCH 2/8] python/qapi: change "FIXME" to "TODO"

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> qemu.git/python/setup.cfg disallows checking in any code with "XXX",
> "FIXME" or "TODO" in the comments. Soften the restriction to only
> prohibit "FIXME", and change the two occurrences of "FIXME" in qapi to
> read "TODO" instead.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

I don't like forbidding FIXME comments.  It's as futile as forbidding
known bugs.  All it can accomplish is making people use other, and
likely less clear ways to document them.

Perhaps projects exist that use FIXME comments only for known bugs in
uncommitted code.  To me, that feels *nuts*.  I commit all kinds of crap
in my tree.  I don't need silly "make check" failures while I develop,
the non-silly ones cause enough friction already.

In fact, we're quite happy to use FIXME comments even in merged code:

    $ git-grep FIXME | wc -l
    494

I can't see why python/ should be different.

> ---
>  python/setup.cfg         | 5 +++++
>  scripts/qapi/commands.py | 2 +-
>  scripts/qapi/events.py   | 2 +-
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 3b4e2cc5501..72b58c98c99 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -169,6 +169,11 @@ ignore-signatures=yes
>  # TODO: Remove after we opt in to Pylint 2.8.3. See commit msg.
>  min-similarity-lines=6
>  
> +[pylint.miscellaneous]
> +
> +# forbid FIXME/XXX comments, allow TODO.
> +notes=FIXME,
> +      XXX,
>  
>  [isort]
>  force_grid_wrap=4
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 79951a841f5..cffed6cd3ba 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -385,7 +385,7 @@ def visit_command(self,
>                        coroutine: bool) -> None:
>          if not gen:
>              return
> -        # FIXME: If T is a user-defined type, the user is responsible
> +        # TODO: If T is a user-defined type, the user is responsible
>          # for making this work, i.e. to make T's condition the
>          # conjunction of the T-returning commands' conditions.  If T
>          # is a built-in type, this isn't possible: the
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index d1f639981a9..36dc0c50c78 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -84,7 +84,7 @@ def gen_event_send(name: str,
>                     boxed: bool,
>                     event_enum_name: str,
>                     event_emit: str) -> str:
> -    # FIXME: Our declaration of local variables (and of 'errp' in the
> +    # TODO: Our declaration of local variables (and of 'errp' in the
>      # parameter list) can collide with exploded members of the event's
>      # data type passed in as parameters.  If this collision ever hits in
>      # practice, we can rename our local variables with a leading _ prefix,

Starting a comment with TODO tells me there's work to do.

Starting it with FIXME tells me there's a bug to fix.  That's more
specific.  Replacing FIXME by TODO loses information.

Re: [PATCH 2/8] python/qapi: change "FIXME" to "TODO"

[John Snow]

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

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

> John Snow <jsnow@redhat.com> writes:
>
> > qemu.git/python/setup.cfg disallows checking in any code with "XXX",
> > "FIXME" or "TODO" in the comments. Soften the restriction to only
> > prohibit "FIXME", and change the two occurrences of "FIXME" in qapi to
> > read "TODO" instead.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> I don't like forbidding FIXME comments.  It's as futile as forbidding
> known bugs.  All it can accomplish is making people use other, and
> likely less clear ways to document them.
>
> Perhaps projects exist that use FIXME comments only for known bugs in
> uncommitted code.  To me, that feels *nuts*.  I commit all kinds of crap
> in my tree.  I don't need silly "make check" failures while I develop,
> the non-silly ones cause enough friction already.
>
> In fact, we're quite happy to use FIXME comments even in merged code:
>
>     $ git-grep FIXME | wc -l
>     494
>
> I can't see why python/ should be different.
>
> > ---
> >  python/setup.cfg         | 5 +++++
> >  scripts/qapi/commands.py | 2 +-
> >  scripts/qapi/events.py   | 2 +-
> >  3 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/python/setup.cfg b/python/setup.cfg
> > index 3b4e2cc5501..72b58c98c99 100644
> > --- a/python/setup.cfg
> > +++ b/python/setup.cfg
> > @@ -169,6 +169,11 @@ ignore-signatures=yes
> >  # TODO: Remove after we opt in to Pylint 2.8.3. See commit msg.
> >  min-similarity-lines=6
> >
> > +[pylint.miscellaneous]
> > +
> > +# forbid FIXME/XXX comments, allow TODO.
> > +notes=FIXME,
> > +      XXX,
> >
> >  [isort]
> >  force_grid_wrap=4
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index 79951a841f5..cffed6cd3ba 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -385,7 +385,7 @@ def visit_command(self,
> >                        coroutine: bool) -> None:
> >          if not gen:
> >              return
> > -        # FIXME: If T is a user-defined type, the user is responsible
> > +        # TODO: If T is a user-defined type, the user is responsible
> >          # for making this work, i.e. to make T's condition the
> >          # conjunction of the T-returning commands' conditions.  If T
> >          # is a built-in type, this isn't possible: the
> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > index d1f639981a9..36dc0c50c78 100644
> > --- a/scripts/qapi/events.py
> > +++ b/scripts/qapi/events.py
> > @@ -84,7 +84,7 @@ def gen_event_send(name: str,
> >                     boxed: bool,
> >                     event_enum_name: str,
> >                     event_emit: str) -> str:
> > -    # FIXME: Our declaration of local variables (and of 'errp' in the
> > +    # TODO: Our declaration of local variables (and of 'errp' in the
> >      # parameter list) can collide with exploded members of the event's
> >      # data type passed in as parameters.  If this collision ever hits in
> >      # practice, we can rename our local variables with a leading _
> prefix,
>
> Starting a comment with TODO tells me there's work to do.
>
> Starting it with FIXME tells me there's a bug to fix.  That's more
> specific.  Replacing FIXME by TODO loses information.
>

meh. I do use the "prohibit fixme" personally because I've the memory of a
goldfish and I like setting up bombs for myself when I run tests, but
willing to cede if it gets me what I want otherwise. I could be coerced to
using "XXX" as my WIP testing bomb. Or maybe literally just adding "WIP" as
a new bomb. Is that a fair trade?

There are likely other standards differences between the two subtrees,
potentially things like documentation string length and so on -- I invite
you to take a look at the setup.cfg file and tweak things to your liking
and run "make check-minreqs" to see what barks, if anything.

After you run that command, you can type "source .dev-venv/bin/activate.sh"
(or .fish or whatever) and then "pylint --generate-rcfile | less" to get a
sample config and see all of the buttons, knobs and levers you could pull.
You can leave the environment when you're done with "deactivate".

Mentioning this only because there have been times in the past that my
formatting hasn't been to your liking, but there are avenues to
programmatically enforce it to make my qapi patches nicer for your tastes
in the future.

--js

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

Re: [PATCH 2/8] python/qapi: change "FIXME" to "TODO"

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Fri, Aug 30, 2024 at 7:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > qemu.git/python/setup.cfg disallows checking in any code with "XXX",
>> > "FIXME" or "TODO" in the comments. Soften the restriction to only
>> > prohibit "FIXME", and change the two occurrences of "FIXME" in qapi to
>> > read "TODO" instead.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> I don't like forbidding FIXME comments.  It's as futile as forbidding
>> known bugs.  All it can accomplish is making people use other, and
>> likely less clear ways to document them.
>>
>> Perhaps projects exist that use FIXME comments only for known bugs in
>> uncommitted code.  To me, that feels *nuts*.  I commit all kinds of crap
>> in my tree.  I don't need silly "make check" failures while I develop,
>> the non-silly ones cause enough friction already.
>>
>> In fact, we're quite happy to use FIXME comments even in merged code:
>>
>>     $ git-grep FIXME | wc -l
>>     494
>>
>> I can't see why python/ should be different.
>>
>> > ---
>> >  python/setup.cfg         | 5 +++++
>> >  scripts/qapi/commands.py | 2 +-
>> >  scripts/qapi/events.py   | 2 +-
>> >  3 files changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/python/setup.cfg b/python/setup.cfg
>> > index 3b4e2cc5501..72b58c98c99 100644
>> > --- a/python/setup.cfg
>> > +++ b/python/setup.cfg
>> > @@ -169,6 +169,11 @@ ignore-signatures=yes
>> >  # TODO: Remove after we opt in to Pylint 2.8.3. See commit msg.
>> >  min-similarity-lines=6
>> >
>> > +[pylint.miscellaneous]
>> > +
>> > +# forbid FIXME/XXX comments, allow TODO.
>> > +notes=FIXME,
>> > +      XXX,
>> >
>> >  [isort]
>> >  force_grid_wrap=4
>> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> > index 79951a841f5..cffed6cd3ba 100644
>> > --- a/scripts/qapi/commands.py
>> > +++ b/scripts/qapi/commands.py
>> > @@ -385,7 +385,7 @@ def visit_command(self,
>> >                        coroutine: bool) -> None:
>> >          if not gen:
>> >              return
>> > -        # FIXME: If T is a user-defined type, the user is responsible
>> > +        # TODO: If T is a user-defined type, the user is responsible
>> >          # for making this work, i.e. to make T's condition the
>> >          # conjunction of the T-returning commands' conditions.  If T
>> >          # is a built-in type, this isn't possible: the
>> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> > index d1f639981a9..36dc0c50c78 100644
>> > --- a/scripts/qapi/events.py
>> > +++ b/scripts/qapi/events.py
>> > @@ -84,7 +84,7 @@ def gen_event_send(name: str,
>> >                     boxed: bool,
>> >                     event_enum_name: str,
>> >                     event_emit: str) -> str:
>> > -    # FIXME: Our declaration of local variables (and of 'errp' in the
>> > +    # TODO: Our declaration of local variables (and of 'errp' in the
>> >      # parameter list) can collide with exploded members of the event's
>> >      # data type passed in as parameters.  If this collision ever hits in
>> >      # practice, we can rename our local variables with a leading prefix,
>>
>> Starting a comment with TODO tells me there's work to do.
>>
>> Starting it with FIXME tells me there's a bug to fix.  That's more
>> specific.  Replacing FIXME by TODO loses information.
>
> meh. I do use the "prohibit fixme" personally because I've the memory of a
> goldfish and I like setting up bombs for myself when I run tests, but
> willing to cede if it gets me what I want otherwise. I could be coerced to
> using "XXX" as my WIP testing bomb. Or maybe literally just adding "WIP" as
> a new bomb. Is that a fair trade?

I understand you'd like to have some kind of stylized comment that
automated testing will reject, to serve as a "do not post before
resolving this issue" reminder.  Fair?

If yes, whatever floats your boat and doesn't interfere with existing
practice.

    $ git-grep -w FIXME | wc -l
    493
    $ git-grep -w TODO | wc -l
    1249
    $ git-grep -w XXX | wc -l
    78288
    $ git-grep -w WIP
    Binary file pc-bios/skiboot.lid matches

> There are likely other standards differences between the two subtrees,
> potentially things like documentation string length and so on -- I invite
> you to take a look at the setup.cfg file and tweak things to your liking
> and run "make check-minreqs" to see what barks, if anything.
>
> After you run that command, you can type "source .dev-venv/bin/activate.sh"
> (or .fish or whatever) and then "pylint --generate-rcfile | less" to get a
> sample config and see all of the buttons, knobs and levers you could pull.
> You can leave the environment when you're done with "deactivate".
>
> Mentioning this only because there have been times in the past that my
> formatting hasn't been to your liking, but there are avenues to
> programmatically enforce it to make my qapi patches nicer for your tastes
> in the future.

Thanks!

Re: [PATCH 2/8] python/qapi: change "FIXME" to "TODO"

[John Snow]

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

On Sat, Aug 31, 2024, 2:02 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Fri, Aug 30, 2024 at 7:09 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > qemu.git/python/setup.cfg disallows checking in any code with "XXX",
> >> > "FIXME" or "TODO" in the comments. Soften the restriction to only
> >> > prohibit "FIXME", and change the two occurrences of "FIXME" in qapi to
> >> > read "TODO" instead.
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >>
> >> I don't like forbidding FIXME comments.  It's as futile as forbidding
> >> known bugs.  All it can accomplish is making people use other, and
> >> likely less clear ways to document them.
> >>
> >> Perhaps projects exist that use FIXME comments only for known bugs in
> >> uncommitted code.  To me, that feels *nuts*.  I commit all kinds of crap
> >> in my tree.  I don't need silly "make check" failures while I develop,
> >> the non-silly ones cause enough friction already.
> >>
> >> In fact, we're quite happy to use FIXME comments even in merged code:
> >>
> >>     $ git-grep FIXME | wc -l
> >>     494
> >>
> >> I can't see why python/ should be different.
> >>
> >> > ---
> >> >  python/setup.cfg         | 5 +++++
> >> >  scripts/qapi/commands.py | 2 +-
> >> >  scripts/qapi/events.py   | 2 +-
> >> >  3 files changed, 7 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/python/setup.cfg b/python/setup.cfg
> >> > index 3b4e2cc5501..72b58c98c99 100644
> >> > --- a/python/setup.cfg
> >> > +++ b/python/setup.cfg
> >> > @@ -169,6 +169,11 @@ ignore-signatures=yes
> >> >  # TODO: Remove after we opt in to Pylint 2.8.3. See commit msg.
> >> >  min-similarity-lines=6
> >> >
> >> > +[pylint.miscellaneous]
> >> > +
> >> > +# forbid FIXME/XXX comments, allow TODO.
> >> > +notes=FIXME,
> >> > +      XXX,
> >> >
> >> >  [isort]
> >> >  force_grid_wrap=4
> >> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> >> > index 79951a841f5..cffed6cd3ba 100644
> >> > --- a/scripts/qapi/commands.py
> >> > +++ b/scripts/qapi/commands.py
> >> > @@ -385,7 +385,7 @@ def visit_command(self,
> >> >                        coroutine: bool) -> None:
> >> >          if not gen:
> >> >              return
> >> > -        # FIXME: If T is a user-defined type, the user is responsible
> >> > +        # TODO: If T is a user-defined type, the user is responsible
> >> >          # for making this work, i.e. to make T's condition the
> >> >          # conjunction of the T-returning commands' conditions.  If T
> >> >          # is a built-in type, this isn't possible: the
> >> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> >> > index d1f639981a9..36dc0c50c78 100644
> >> > --- a/scripts/qapi/events.py
> >> > +++ b/scripts/qapi/events.py
> >> > @@ -84,7 +84,7 @@ def gen_event_send(name: str,
> >> >                     boxed: bool,
> >> >                     event_enum_name: str,
> >> >                     event_emit: str) -> str:
> >> > -    # FIXME: Our declaration of local variables (and of 'errp' in the
> >> > +    # TODO: Our declaration of local variables (and of 'errp' in the
> >> >      # parameter list) can collide with exploded members of the
> event's
> >> >      # data type passed in as parameters.  If this collision ever
> hits in
> >> >      # practice, we can rename our local variables with a leading
> prefix,
> >>
> >> Starting a comment with TODO tells me there's work to do.
> >>
> >> Starting it with FIXME tells me there's a bug to fix.  That's more
> >> specific.  Replacing FIXME by TODO loses information.
> >
> > meh. I do use the "prohibit fixme" personally because I've the memory of
> a
> > goldfish and I like setting up bombs for myself when I run tests, but
> > willing to cede if it gets me what I want otherwise. I could be coerced
> to
> > using "XXX" as my WIP testing bomb. Or maybe literally just adding "WIP"
> as
> > a new bomb. Is that a fair trade?
>
> I understand you'd like to have some kind of stylized comment that
> automated testing will reject, to serve as a "do not post before
> resolving this issue" reminder.  Fair?
>

Yep, that's it exactly.


> If yes, whatever floats your boat and doesn't interfere with existing
> practice.
>
>     $ git-grep -w FIXME | wc -l
>     493
>     $ git-grep -w TODO | wc -l
>     1249
>     $ git-grep -w XXX | wc -l
>     78288
>     $ git-grep -w WIP
>     Binary file pc-bios/skiboot.lid matches
>

"WIP" it is. I'll adjust the conf and nix this patch.


> > There are likely other standards differences between the two subtrees,
> > potentially things like documentation string length and so on -- I invite
> > you to take a look at the setup.cfg file and tweak things to your liking
> > and run "make check-minreqs" to see what barks, if anything.
> >
> > After you run that command, you can type "source
> .dev-venv/bin/activate.sh"
> > (or .fish or whatever) and then "pylint --generate-rcfile | less" to get
> a
> > sample config and see all of the buttons, knobs and levers you could
> pull.
> > You can leave the environment when you're done with "deactivate".
> >
> > Mentioning this only because there have been times in the past that my
> > formatting hasn't been to your liking, but there are avenues to
> > programmatically enforce it to make my qapi patches nicer for your tastes
> > in the future.
>
> Thanks!
>

No problem! I know our tastes don't always match but I figure if we can
agree on a config file, it'll make things nicer for the both of us in the
future.

We can discuss at KVM Forum too any of the little nits or preferences you'd
like to see automated and enforced to unify the Python style we use in
tree. I'd also like to discuss the usage of black formatting to automate
away the formatting guesswork. It'd be a lot of churn, but it would
drastically simplify things. (e.g. "make autoformat" in the python dir,
plus we can look into emacs/vim/vscode config for the repo)

I'll handle the config/maintenance/tooling but you can let me know your
preferences.

(This series should be a good example of which things I have configured for
python/ that we didn't tackle for qapi. It isn't very much. Some things you
prefer may now be *unhandled*, I think docstring length being confined to
72(?) or less might be one of them, I don't recall others at the moment.)

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

[PATCH 3/8] python/qapi: add pylint pragmas

[John Snow]

We are preparing to move the QAPI generator code into
qemu.git/python/qemu/qapi.

The qemu.git/python pylint configuration is stricter than the current
qapi generator configuration. These additional pragmas bridge the gap
without requiring us to loosen the requirements in the python directory.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py       | 1 +
 scripts/qapi/introspect.py | 1 +
 scripts/qapi/parser.py     | 4 ++++
 scripts/qapi/visit.py      | 1 +
 4 files changed, 7 insertions(+)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index cae0a083591..f60e580dd36 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -604,6 +604,7 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
     :raise QAPISemError: When any expression fails validation.
     :return: The same list of expressions (now modified).
     """
+    # pylint: disable=too-many-branches
     for expr in exprs:
         info = expr.info
         doc = expr.doc
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index ac14b20f308..9d499f90b7c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -106,6 +106,7 @@ def _tree_to_qlit(obj: JSONValue,
     :param dict_value: True when the value being processed belongs to a
                        dict key; which suppresses the output indent.
     """
+    # pylint: disable=too-many-branches
 
     def indent(level: int) -> str:
         return level * 4 * ' '
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 9a42b119131..9bb039fe8d3 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -118,6 +118,8 @@ def _parse(self) -> None:
 
         :return: None.  Results are stored in ``.exprs`` and ``.docs``.
         """
+        # pylint: disable=too-many-branches
+
         cur_doc = None
 
         # May raise OSError; allow the caller to handle it.
@@ -290,6 +292,7 @@ def accept(self, skip_comment: bool = True) -> None:
 
             ``.tok`` and ``.val`` will both be None at EOF.
         """
+        # pylint: disable=too-many-branches
         while True:
             self.tok = self.src[self.cursor]
             self.pos = self.cursor
@@ -478,6 +481,7 @@ def get_doc_paragraph(self, doc: 'QAPIDoc') -> Optional[str]:
             doc.append_line(line)
 
     def get_doc(self) -> 'QAPIDoc':
+        # pylint: disable=too-many-statements,too-many-branches
         if self.val != '##':
             raise QAPIParseError(
                 self, "junk after '##' at start of documentation comment")
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 12f92e429f6..20ce6be9978 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -65,6 +65,7 @@ def gen_visit_object_members(name: str,
                              base: Optional[QAPISchemaObjectType],
                              members: List[QAPISchemaObjectTypeMember],
                              branches: Optional[QAPISchemaBranches]) -> str:
+    # pylint: disable=too-many-branches
     ret = mcgen('''
 
 bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
-- 
2.45.0

[PATCH 4/8] python/qapi: remove outdated pragmas

[John Snow]

These pragmas are no longer neccessary under our current linter/static
analysis versions; they can be removed.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/gen.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 6a8abe00415..ce94aee8e70 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -62,11 +62,9 @@ def get_content(self) -> str:
         return self._top() + self._preamble + self._body + self._bottom()
 
     def _top(self) -> str:
-        # pylint: disable=no-self-use
         return ''
 
     def _bottom(self) -> str:
-        # pylint: disable=no-self-use
         return ''
 
     def write(self, output_dir: str) -> None:
-- 
2.45.0

[PATCH 5/8] python/qapi: ignore missing docstrings in pylint

[John Snow]

Maybe temporary, I am not sure. Instead of disabling docstring checking
*globally* for all of our python files, just disable it for QAPI
modules.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/commands.py   | 2 ++
 scripts/qapi/common.py     | 2 ++
 scripts/qapi/events.py     | 2 ++
 scripts/qapi/expr.py       | 2 ++
 scripts/qapi/gen.py        | 2 ++
 scripts/qapi/introspect.py | 2 ++
 scripts/qapi/main.py       | 2 ++
 scripts/qapi/parser.py     | 2 ++
 scripts/qapi/schema.py     | 2 +-
 scripts/qapi/source.py     | 2 ++
 scripts/qapi/types.py      | 2 ++
 scripts/qapi/visit.py      | 2 ++
 12 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index cffed6cd3ba..b01de93c965 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -13,6 +13,8 @@
 See the COPYING file in the top-level directory.
 """
 
+# pylint: disable=missing-docstring
+
 from typing import (
     Dict,
     List,
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 444b3acf53a..918a1ab728a 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -11,6 +11,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+# pylint: disable=missing-docstring
+
 import re
 import sys
 from typing import (
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 36dc0c50c78..9ab9ff4e695 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -12,6 +12,8 @@
 See the COPYING file in the top-level directory.
 """
 
+# pylint: disable=missing-docstring
+
 from typing import List, Optional
 
 from .common import c_enum_const, c_name, mcgen
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index f60e580dd36..c137dcf950a 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -13,6 +13,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+# pylint: disable=missing-docstring
+
 """
 Normalize and validate (context-free) QAPI schema expression structures.
 
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index ce94aee8e70..f869c751e53 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -11,6 +11,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+# pylint: disable=missing-docstring
+
 from contextlib import contextmanager
 import os
 import re
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 9d499f90b7c..44edc42d18b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -11,6 +11,8 @@
 See the COPYING file in the top-level directory.
 """
 
+# pylint: disable=missing-docstring
+
 from typing import (
     Any,
     Dict,
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 316736b6a29..24ffa15aa2c 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -7,6 +7,8 @@
 This is the main entry point for generating C code from the QAPI schema.
 """
 
+# pylint: disable=missing-docstring
+
 import argparse
 import sys
 from typing import Optional
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 9bb039fe8d3..9113c9d1506 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -14,6 +14,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+# pylint: disable=missing-docstring
+
 from collections import OrderedDict
 import os
 import re
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index d65c35f6ee6..a06b3e30ffd 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -12,7 +12,7 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
-# pylint: disable=too-many-lines
+# pylint: disable=too-many-lines, missing-docstring
 
 # TODO catching name collisions in generated code would be nice
 
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index 7b379fdc925..ad7f4e1a0e5 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -9,6 +9,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+# pylint: disable=missing-docstring
+
 import copy
 from typing import List, Optional, TypeVar
 
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 0dd0b00ada3..abba5983870 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -13,6 +13,8 @@
 # See the COPYING file in the top-level directory.
 """
 
+# pylint: disable=missing-docstring
+
 from typing import List, Optional
 
 from .common import c_enum_const, c_name, mcgen
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 20ce6be9978..851f3b01e97 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -13,6 +13,8 @@
 See the COPYING file in the top-level directory.
 """
 
+# pylint: disable=missing-docstring
+
 from typing import List, Optional
 
 from .common import (
-- 
2.45.0

[PATCH 6/8] python: allow short names for variables on older pylint

[John Snow]

Pylint >= 3.0.0 disabled this feature, but older pylint does not: allow
short names by default by using a regex to do so.

Incidentally, this removes the need for most of the allow list we had before, so
remove most of that, too.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/setup.cfg | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/python/setup.cfg b/python/setup.cfg
index 72b58c98c99..58dba90f815 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -148,17 +148,11 @@ disable=consider-using-f-string,
 
 [pylint.basic]
 # Good variable names which should always be accepted, separated by a comma.
-good-names=i,
-           j,
-           k,
-           ex,
-           Run,
-           _,   # By convention: Unused variable
-           fh,  # fh = open(...)
-           fd,  # fd = os.open(...)
-           c,   # for c in string: ...
-           T,   # for TypeVars. See pylint#3401
-           SocketAddrT,  # Not sure why this is invalid.
+good-names=SocketAddrT,  # Not sure why this is invalid.
+
+# pylint < 3.0 warns by default on short variable names.
+# Disable this for older versions.
+good-names-rgxs=^[_a-z][_a-z0-9]?$
 
 [pylint.similarities]
 # Ignore imports when computing similarities.
-- 
2.45.0

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

[John Snow]

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

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

[Markus Armbruster]

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.

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

[Daniel P. Berrangé]

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

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

[John Snow]

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

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

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

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

[John Snow]

[-- 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 --]

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

[Daniel P. Berrangé]

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

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

[Peter Maydell]

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

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

[John Snow]

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

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

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

[PATCH 8/8] python/qapi: remove redundant linter configuration

[John Snow]

Now that the qemu.qapi module is checked by the standard python tests,
we don't need separate configuration for it anymore.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/qapi/.flake8    |  3 --
 python/qemu/qapi/.isort.cfg |  7 -----
 python/qemu/qapi/mypy.ini   |  4 ---
 python/qemu/qapi/pylintrc   | 61 -------------------------------------
 4 files changed, 75 deletions(-)
 delete mode 100644 python/qemu/qapi/.flake8
 delete mode 100644 python/qemu/qapi/.isort.cfg
 delete mode 100644 python/qemu/qapi/mypy.ini
 delete mode 100644 python/qemu/qapi/pylintrc

diff --git a/python/qemu/qapi/.flake8 b/python/qemu/qapi/.flake8
deleted file mode 100644
index a873ff67309..00000000000
--- a/python/qemu/qapi/.flake8
+++ /dev/null
@@ -1,3 +0,0 @@
-[flake8]
-# Prefer pylint's bare-except checks to flake8's
-extend-ignore = E722
diff --git a/python/qemu/qapi/.isort.cfg b/python/qemu/qapi/.isort.cfg
deleted file mode 100644
index 643caa1fbd6..00000000000
--- a/python/qemu/qapi/.isort.cfg
+++ /dev/null
@@ -1,7 +0,0 @@
-[settings]
-force_grid_wrap=4
-force_sort_within_sections=True
-include_trailing_comma=True
-line_length=72
-lines_after_imports=2
-multi_line_output=3
diff --git a/python/qemu/qapi/mypy.ini b/python/qemu/qapi/mypy.ini
deleted file mode 100644
index 8109470a031..00000000000
--- a/python/qemu/qapi/mypy.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[mypy]
-strict = True
-disallow_untyped_calls = False
-python_version = 3.8
diff --git a/python/qemu/qapi/pylintrc b/python/qemu/qapi/pylintrc
deleted file mode 100644
index c028a1f9f51..00000000000
--- a/python/qemu/qapi/pylintrc
+++ /dev/null
@@ -1,61 +0,0 @@
-[MASTER]
-
-[MESSAGES CONTROL]
-
-# Disable the message, report, category or checker with the given id(s). You
-# can either give multiple identifiers separated by comma (,) or put this
-# option multiple times (only on the command line, not in the configuration
-# file where it should appear only once). You can also use "--disable=all" to
-# disable everything first and then reenable specific checks. For example, if
-# you want to run only the similarities checker, you can use "--disable=all
-# --enable=similarities". If you want to run only the classes checker, but have
-# no Warning level messages displayed, use "--disable=all --enable=classes
-# --disable=W".
-disable=consider-using-f-string,
-        fixme,
-        missing-docstring,
-        too-many-arguments,
-        too-many-branches,
-        too-many-instance-attributes,
-        too-many-statements,
-        useless-option-value,
-
-[REPORTS]
-
-[REFACTORING]
-
-[MISCELLANEOUS]
-
-[LOGGING]
-
-[BASIC]
-
-# Good variable names regexes, separated by a comma. If names match any regex,
-# they will always be accepted.
-#
-# Suppress complaints about short names.  PEP-8 is cool with them,
-# and so are we.
-good-names-rgxs=^[_a-z][_a-z0-9]?$
-
-[VARIABLES]
-
-[STRING]
-
-[SPELLING]
-
-[FORMAT]
-
-[SIMILARITIES]
-
-# Ignore import statements themselves when computing similarities.
-ignore-imports=yes
-
-[TYPECHECK]
-
-[CLASSES]
-
-[IMPORTS]
-
-[DESIGN]
-
-[EXCEPTIONS]
-- 
2.45.0