Re: [PATCH v3 4/4] qapi: expose all schema features to code

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On Fri, Jan 31, 2025 at 8:18 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Cc: John Snow for Python typing expertise.
>>
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > This replaces use of the constants from the QapiSpecialFeatures
>> > enum, with constants from the auto-generate QapiFeatures enum
>> > in qapi-features.h
>> >
>> > The 'deprecated' and 'unstable' features still have a little bit of
>> > special handling, being force defined to be the 1st + 2nd features
>> > in the enum, regardless of whether they're used in the schema. This
>> > retains compatibility with common code that references the features
>> > via the QapiSpecialFeatures constants.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

[...]

>> > diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
>> > new file mode 100644
>> > index 0000000000..f32f9fe5f4
>> > --- /dev/null
>> > +++ b/scripts/qapi/features.py
>> > @@ -0,0 +1,51 @@
>> > +"""
>> > +QAPI features generator
>> > +
>> > +Copyright 2024 Red Hat
>> > +
>> > +This work is licensed under the terms of the GNU GPL, version 2.
>> > +# See the COPYING file in the top-level directory.
>> > +"""
>> > +
>> > +from typing import Dict
>> > +
>> > +from .common import c_enum_const, c_name
>> > +from .gen import QAPISchemaMonolithicCVisitor
>> > +from .schema import (
>> > +    QAPISchema,
>> > +    QAPISchemaFeature,
>> > +)
>> > +
>> > +
>> > +class QAPISchemaGenFeatureVisitor(QAPISchemaMonolithicCVisitor):
>> > +
>> > +    def __init__(self, prefix: str):
>> > +        super().__init__(
>> > +            prefix, 'qapi-features',
>> > +            ' * Schema-defined QAPI features',
>> > +            __doc__)
>> > +
>> > +        self.features: Dict[str, QAPISchemaFeature] = {}
>> > +
>> > +    def visit_begin(self, schema: QAPISchema) -> None:
>> > +        self.features = schema.features()
>>
>> Inconsistent type hints:
>>
>>     $ mypy --config-file scripts/qapi/mypy.ini scripts/qapi-gen.py
>>     scripts/qapi/schema.py:1164: error: Incompatible return value type
>> (got "dict_values[str, QAPISchemaFeature]", expected
>> "List[QAPISchemaFeature]")  [return-value]
>>     scripts/qapi/features.py:31: error: Incompatible types in assignment
>> (expression has type "List[QAPISchemaFeature]", variable has type
>> "Dict[str, QAPISchemaFeature]")  [assignment]
>>
>> We've been working towards having the build run mypy, but we're not
>> there, yet.  Sorry for the inconvenience!
>>
>> schema.features() returns .values(), i.e. a view object.
>>
>> I guess the type hint should be ValuesView[QAPISchemaFeature], both for
>> type type of attribute .features above, and for the return type of
>> method .features() below.  John?
>>
>
> It's probably easiest to just use list(...) in the return and then use
> List[T] anywhere it matters. The values view type is "kind of, but not
> actually a list" because it isn't mutable. It is, however, an
> Iterable/Sequence. You can either convert it to a list or make the typing
> more abstract.
>
> (Rule of thumb: return types should be as specific as possible, input types
> should be as abstract as possible.)

Converting a view to a list makes a copy, right?

I'm not asking because that would be terrible.  I just like to
understand things.

I'd like to move further discussion to Daniel's v4.

> I apologize for this format of relaying patches as it is against the blood
> oath I swore as a maintainer, but it's late in my day, forgive me:
> https://gitlab.com/jsnow/qemu/-/commits/dan-fixup
>
> That branch has two things in it:
>
> (1) patches to make the python/ tests check the qapi module. This means the
> "make check-minreqs" test you can run from python/ will be run by the
> GitLab pipelines. You can also run "make check-tox" manually, or run the
> optional python-tox test from the pipeline dashboard.

These are:

    dd9e47f0a8 qapi: update pylintrc config
    dfc6344f32 python: add qapi static analysis tests
    1f89bf53ed qapi: delete un-needed static analysis configs

Will you post them for review & merging?

> (2) two fixups for linting problems with this series with my s-o-b; feel
> free to steal them if they're good enough for you.

These are:

    b36a412162 fixup
    fa6c90ea76 fixup

I'll post them in reply to Daniel's v4 so they get recorded in the list
archive.

> Thank you for your patience,
> --js

Thanks for your help!

[...]