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>
---
meson.build | 1 +
scripts/qapi/commands.py | 1 +
scripts/qapi/features.py | 51 ++++++++++++++++++++++++
scripts/qapi/gen.py | 6 +--
scripts/qapi/main.py | 2 +
scripts/qapi/schema.py | 30 +++++++++++++-
scripts/qapi/types.py | 7 +++-
scripts/qapi/visit.py | 3 +-
tests/meson.build | 2 +
tests/qapi-schema/features-too-many.err | 2 +
tests/qapi-schema/features-too-many.json | 13 ++++++
tests/qapi-schema/features-too-many.out | 0
tests/qapi-schema/meson.build | 1 +
13 files changed, 112 insertions(+), 7 deletions(-)
create mode 100644 scripts/qapi/features.py
create mode 100644 tests/qapi-schema/features-too-many.err
create mode 100644 tests/qapi-schema/features-too-many.json
create mode 100644 tests/qapi-schema/features-too-many.out
diff --git a/meson.build b/meson.build
index 147097c652..3815878b23 100644
--- a/meson.build
+++ b/meson.build
@@ -3444,6 +3444,7 @@ qapi_gen_depends = [ meson.current_source_dir() / 'scripts/qapi/__init__.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/features.py',
meson.current_source_dir() / 'scripts/qapi/visit.py',
meson.current_source_dir() / 'scripts/qapi-gen.py'
]
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index d629d2d97e..bf88bfc442 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -355,6 +355,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
#include "qemu/osdep.h"
#include "%(prefix)sqapi-commands.h"
#include "%(prefix)sqapi-init-commands.h"
+#include "%(prefix)sqapi-features.h"
void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
{
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()
+ self._genh.add("#include \"qapi/util.h\"\n\n")
+
+ def visit_end(self) -> None:
+ self._genh.add("typedef enum {\n")
+ for f in self.features:
+ self._genh.add(f" {c_enum_const('qapi_feature', f.name)}")
+ if f.name in QAPISchemaFeature.SPECIAL_NAMES:
+ self._genh.add(f" = {c_enum_const('qapi', f.name)},\n")
+ else:
+ self._genh.add(",\n")
+
+ self._genh.add("} " + c_name('QapiFeature') + ";\n")
+
+
+def gen_features(schema: QAPISchema,
+ output_dir: str,
+ prefix: str) -> None:
+ vis = QAPISchemaGenFeatureVisitor(prefix)
+ schema.visit(vis)
+ vis.write(output_dir)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b51f8d955e..d3c56d45c8 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -42,9 +42,9 @@
def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
- featenum = [f"1u << {c_enum_const('qapi', feat.name)}"
- for feat in features if feat.is_special()]
- return ' | '.join(featenum) or '0'
+ feats = [f"1u << {c_enum_const('qapi_feature', feat.name)}"
+ for feat in features]
+ return ' | '.join(feats) or '0'
class QAPIGen:
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 316736b6a2..2b9a2c0c02 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -18,6 +18,7 @@
from .introspect import gen_introspect
from .schema import QAPISchema
from .types import gen_types
+from .features import gen_features
from .visit import gen_visit
@@ -49,6 +50,7 @@ def generate(schema_file: str,
schema = QAPISchema(schema_file)
gen_types(schema, output_dir, prefix, builtins)
+ gen_features(schema, output_dir, prefix)
gen_visit(schema, output_dir, prefix, builtins)
gen_commands(schema, output_dir, prefix, gen_tracing)
gen_events(schema, output_dir, prefix)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index e97c978d38..39c91af245 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -933,8 +933,11 @@ def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
class QAPISchemaFeature(QAPISchemaMember):
role = 'feature'
+ # Features which are standardized across all schemas
+ SPECIAL_NAMES = ['deprecated', 'unstable']
+
def is_special(self) -> bool:
- return self.name in ('deprecated', 'unstable')
+ return self.name in QAPISchemaFeature.SPECIAL_NAMES
class QAPISchemaObjectTypeMember(QAPISchemaMember):
@@ -1138,6 +1141,16 @@ def __init__(self, fname: str):
self._entity_list: List[QAPISchemaEntity] = []
self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
+ # NB, values in the dict will identify the first encountered
+ # usage of a named feature only
+ self._feature_dict: Dict[str, QAPISchemaFeature] = OrderedDict()
+
+ # All schemas get the names defined in the QapiSpecialFeature enum.
+ # Use of OrderedDict ensures they are emitted first when generating
+ # the enum definition, thus matching QapiSpecialFeature.
+ for f in QAPISchemaFeature.SPECIAL_NAMES:
+ self._feature_dict[f] = QAPISchemaFeature(f, None)
+
self._schema_dir = os.path.dirname(fname)
self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
self._make_module(fname)
@@ -1147,6 +1160,9 @@ def __init__(self, fname: str):
self._def_exprs(exprs)
self.check()
+ def features(self) -> List[QAPISchemaFeature]:
+ return self._feature_dict.values()
+
def _def_entity(self, ent: QAPISchemaEntity) -> None:
self._entity_list.append(ent)
@@ -1258,6 +1274,12 @@ def _make_features(
) -> List[QAPISchemaFeature]:
if features is None:
return []
+
+ for f in features:
+ feat = QAPISchemaFeature(f['name'], info)
+ if feat.name not in self._feature_dict:
+ self._feature_dict[feat.name] = feat
+
return [QAPISchemaFeature(f['name'], info,
QAPISchemaIfCond(f.get('if')))
for f in features]
@@ -1485,6 +1507,12 @@ def check(self) -> None:
for doc in self.docs:
doc.check()
+ features = list(self._feature_dict.values())
+ if len(features) > 64:
+ raise QAPISemError(
+ features[64].info,
+ "Maximum of 64 schema features is permitted")
+
def visit(self, visitor: QAPISchemaVisitor) -> None:
visitor.visit_begin(self)
for mod in self._module_dict.values():
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index ade6b7a3d7..5294e5ea3b 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -308,11 +308,14 @@ def _begin_user_module(self, name: str) -> None:
#include "qapi/dealloc-visitor.h"
#include "%(types)s.h"
#include "%(visit)s.h"
+#include "%(prefix)sqapi-features.h"
''',
- types=types, visit=visit))
+ types=types, visit=visit,
+ prefix=self._prefix))
self._genh.preamble_add(mcgen('''
#include "qapi/qapi-builtin-types.h"
-'''))
+''',
+ prefix=self._prefix))
def visit_begin(self, schema: QAPISchema) -> None:
# gen_object() is recursive, ensure it doesn't visit the empty type
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 8dbf4ef1c3..2d678c281d 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -360,8 +360,9 @@ def _begin_user_module(self, name: str) -> None:
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "%(visit)s.h"
+#include "%(prefix)sqapi-features.h"
''',
- visit=visit))
+ visit=visit, prefix=self._prefix))
self._genh.preamble_add(mcgen('''
#include "qapi/qapi-builtin-visit.h"
#include "%(types)s.h"
diff --git a/tests/meson.build b/tests/meson.build
index 907a4c1c98..a4ede66d0d 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -16,6 +16,8 @@ test_qapi_outputs = [
'test-qapi-events-sub-sub-module.h',
'test-qapi-events.c',
'test-qapi-events.h',
+ 'test-qapi-features.c',
+ 'test-qapi-features.h',
'test-qapi-init-commands.c',
'test-qapi-init-commands.h',
'test-qapi-introspect.c',
diff --git a/tests/qapi-schema/features-too-many.err b/tests/qapi-schema/features-too-many.err
new file mode 100644
index 0000000000..bbbd6e5202
--- /dev/null
+++ b/tests/qapi-schema/features-too-many.err
@@ -0,0 +1,2 @@
+features-too-many.json: In command 'go-fish':
+features-too-many.json:2: Maximum of 64 schema features is permitted
diff --git a/tests/qapi-schema/features-too-many.json b/tests/qapi-schema/features-too-many.json
new file mode 100644
index 0000000000..aab0a0b5f1
--- /dev/null
+++ b/tests/qapi-schema/features-too-many.json
@@ -0,0 +1,13 @@
+# Max 64 features, with 2 specials, so 63rd custom is invalid
+{ 'command': 'go-fish',
+ 'features': [
+ 'f00', 'f01', 'f02', 'f03', 'f04', 'f05', 'f06', 'f07',
+ 'f08', 'f09', 'f0a', 'f0b', 'f0c', 'f0d', 'f0e', 'f0f',
+ 'f10', 'f11', 'f12', 'f13', 'f14', 'f15', 'f16', 'f17',
+ 'f18', 'f19', 'f1a', 'f1b', 'f1c', 'f1d', 'f1e', 'f1f',
+ 'f20', 'f21', 'f22', 'f23', 'f24', 'f25', 'f26', 'f27',
+ 'f28', 'f29', 'f2a', 'f2b', 'f2c', 'f2d', 'f2e', 'f2f',
+ 'f30', 'f31', 'f32', 'f33', 'f34', 'f35', 'f36', 'f37',
+ 'f38', 'f39', 'f3a', 'f3b', 'f3c', 'f3d', 'f3e'
+ ]
+}
diff --git a/tests/qapi-schema/features-too-many.out b/tests/qapi-schema/features-too-many.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index 0f479d9317..9577178b6f 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -105,6 +105,7 @@ schemas = [
'event-case.json',
'event-member-invalid-dict.json',
'event-nest-struct.json',
+ 'features-too-many.json',
'features-bad-type.json',
'features-deprecated-type.json',
'features-duplicate-name.json',
--
2.46.0
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/schema.py b/scripts/qapi/schema.py
> index e97c978d38..39c91af245 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -933,8 +933,11 @@ def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
> class QAPISchemaFeature(QAPISchemaMember):
> role = 'feature'
>
> + # Features which are standardized across all schemas
> + SPECIAL_NAMES = ['deprecated', 'unstable']
> +
> def is_special(self) -> bool:
> - return self.name in ('deprecated', 'unstable')
> + return self.name in QAPISchemaFeature.SPECIAL_NAMES
>
>
> class QAPISchemaObjectTypeMember(QAPISchemaMember):
> @@ -1138,6 +1141,16 @@ def __init__(self, fname: str):
> self._entity_list: List[QAPISchemaEntity] = []
> self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
> self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
> + # NB, values in the dict will identify the first encountered
> + # usage of a named feature only
Unusual indentation within the comment. Can tidy up in my tree.
> + self._feature_dict: Dict[str, QAPISchemaFeature] = OrderedDict()
> +
> + # All schemas get the names defined in the QapiSpecialFeature enum.
> + # Use of OrderedDict ensures they are emitted first when generating
> + # the enum definition, thus matching QapiSpecialFeature.
Actually, even plain dict ensures that since 3.6 de facto, and since 3.7
de jure. OrderedDict additionally permits changing the order, but we
don't use that. I have a patch to get rid of OrderedDict as part of a
not quite finishes series of simplifications for Python 3.8.
Perhaps the easiest way forward is not to worry about this here and now,
and instead clean it up with along with the other uses of OrderedDict.
> + for f in QAPISchemaFeature.SPECIAL_NAMES:
> + self._feature_dict[f] = QAPISchemaFeature(f, None)
> +
> self._schema_dir = os.path.dirname(fname)
> self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
> self._make_module(fname)
[...]
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>
> ---
> meson.build | 1 +
> scripts/qapi/commands.py | 1 +
> scripts/qapi/features.py | 51 ++++++++++++++++++++++++
> scripts/qapi/gen.py | 6 +--
> scripts/qapi/main.py | 2 +
> scripts/qapi/schema.py | 30 +++++++++++++-
> scripts/qapi/types.py | 7 +++-
> scripts/qapi/visit.py | 3 +-
> tests/meson.build | 2 +
> tests/qapi-schema/features-too-many.err | 2 +
> tests/qapi-schema/features-too-many.json | 13 ++++++
> tests/qapi-schema/features-too-many.out | 0
> tests/qapi-schema/meson.build | 1 +
> 13 files changed, 112 insertions(+), 7 deletions(-)
> create mode 100644 scripts/qapi/features.py
> create mode 100644 tests/qapi-schema/features-too-many.err
> create mode 100644 tests/qapi-schema/features-too-many.json
> create mode 100644 tests/qapi-schema/features-too-many.out
>
> diff --git a/meson.build b/meson.build
> index 147097c652..3815878b23 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3444,6 +3444,7 @@ qapi_gen_depends = [ meson.current_source_dir() / 'scripts/qapi/__init__.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/features.py',
> meson.current_source_dir() / 'scripts/qapi/visit.py',
> meson.current_source_dir() / 'scripts/qapi-gen.py'
> ]
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index d629d2d97e..bf88bfc442 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -355,6 +355,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
> #include "qemu/osdep.h"
> #include "%(prefix)sqapi-commands.h"
> #include "%(prefix)sqapi-init-commands.h"
> +#include "%(prefix)sqapi-features.h"
>
> void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
> {
> 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?
Tentative fixup appended.
> + self._genh.add("#include \"qapi/util.h\"\n\n")
> +
> + def visit_end(self) -> None:
> + self._genh.add("typedef enum {\n")
> + for f in self.features:
> + self._genh.add(f" {c_enum_const('qapi_feature', f.name)}")
> + if f.name in QAPISchemaFeature.SPECIAL_NAMES:
> + self._genh.add(f" = {c_enum_const('qapi', f.name)},\n")
More type confusion here:
scripts/qapi/features.py:37: error: "str" has no attribute "name" [attr-defined]
scripts/qapi/features.py:38: error: "str" has no attribute "name" [attr-defined]
scripts/qapi/features.py:39: error: "str" has no attribute "name" [attr-defined]
My fixup takes care of these, too.
> + else:
> + self._genh.add(",\n")
> +
> + self._genh.add("} " + c_name('QapiFeature') + ";\n")
> +
> +
> +def gen_features(schema: QAPISchema,
> + output_dir: str,
> + prefix: str) -> None:
> + vis = QAPISchemaGenFeatureVisitor(prefix)
> + schema.visit(vis)
> + vis.write(output_dir)
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index b51f8d955e..d3c56d45c8 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -42,9 +42,9 @@
>
>
> def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
> - featenum = [f"1u << {c_enum_const('qapi', feat.name)}"
> - for feat in features if feat.is_special()]
> - return ' | '.join(featenum) or '0'
> + feats = [f"1u << {c_enum_const('qapi_feature', feat.name)}"
> + for feat in features]
> + return ' | '.join(feats) or '0'
>
>
> class QAPIGen:
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index 316736b6a2..2b9a2c0c02 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -18,6 +18,7 @@
> from .introspect import gen_introspect
> from .schema import QAPISchema
> from .types import gen_types
> +from .features import gen_features
> from .visit import gen_visit
>
>
> @@ -49,6 +50,7 @@ def generate(schema_file: str,
>
> schema = QAPISchema(schema_file)
> gen_types(schema, output_dir, prefix, builtins)
> + gen_features(schema, output_dir, prefix)
> gen_visit(schema, output_dir, prefix, builtins)
> gen_commands(schema, output_dir, prefix, gen_tracing)
> gen_events(schema, output_dir, prefix)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index e97c978d38..39c91af245 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -933,8 +933,11 @@ def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
> class QAPISchemaFeature(QAPISchemaMember):
> role = 'feature'
>
> + # Features which are standardized across all schemas
> + SPECIAL_NAMES = ['deprecated', 'unstable']
> +
> def is_special(self) -> bool:
> - return self.name in ('deprecated', 'unstable')
> + return self.name in QAPISchemaFeature.SPECIAL_NAMES
>
>
> class QAPISchemaObjectTypeMember(QAPISchemaMember):
> @@ -1138,6 +1141,16 @@ def __init__(self, fname: str):
> self._entity_list: List[QAPISchemaEntity] = []
> self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
> self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
> + # NB, values in the dict will identify the first encountered
> + # usage of a named feature only
> + self._feature_dict: Dict[str, QAPISchemaFeature] = OrderedDict()
> +
> + # All schemas get the names defined in the QapiSpecialFeature enum.
> + # Use of OrderedDict ensures they are emitted first when generating
> + # the enum definition, thus matching QapiSpecialFeature.
> + for f in QAPISchemaFeature.SPECIAL_NAMES:
> + self._feature_dict[f] = QAPISchemaFeature(f, None)
> +
> self._schema_dir = os.path.dirname(fname)
> self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
> self._make_module(fname)
> @@ -1147,6 +1160,9 @@ def __init__(self, fname: str):
> self._def_exprs(exprs)
> self.check()
>
> + def features(self) -> List[QAPISchemaFeature]:
> + return self._feature_dict.values()
See typing trouble above.
> +
> def _def_entity(self, ent: QAPISchemaEntity) -> None:
> self._entity_list.append(ent)
>
> @@ -1258,6 +1274,12 @@ def _make_features(
> ) -> List[QAPISchemaFeature]:
> if features is None:
> return []
> +
> + for f in features:
> + feat = QAPISchemaFeature(f['name'], info)
> + if feat.name not in self._feature_dict:
> + self._feature_dict[feat.name] = feat
> +
> return [QAPISchemaFeature(f['name'], info,
> QAPISchemaIfCond(f.get('if')))
> for f in features]
> @@ -1485,6 +1507,12 @@ def check(self) -> None:
> for doc in self.docs:
> doc.check()
>
> + features = list(self._feature_dict.values())
> + if len(features) > 64:
> + raise QAPISemError(
> + features[64].info,
> + "Maximum of 64 schema features is permitted")
> +
> def visit(self, visitor: QAPISchemaVisitor) -> None:
> visitor.visit_begin(self)
> for mod in self._module_dict.values():
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index ade6b7a3d7..5294e5ea3b 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -308,11 +308,14 @@ def _begin_user_module(self, name: str) -> None:
> #include "qapi/dealloc-visitor.h"
> #include "%(types)s.h"
> #include "%(visit)s.h"
> +#include "%(prefix)sqapi-features.h"
> ''',
> - types=types, visit=visit))
> + types=types, visit=visit,
> + prefix=self._prefix))
> self._genh.preamble_add(mcgen('''
> #include "qapi/qapi-builtin-types.h"
> -'''))
> +''',
> + prefix=self._prefix))
>
> def visit_begin(self, schema: QAPISchema) -> None:
> # gen_object() is recursive, ensure it doesn't visit the empty type
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 8dbf4ef1c3..2d678c281d 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -360,8 +360,9 @@ def _begin_user_module(self, name: str) -> None:
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "%(visit)s.h"
> +#include "%(prefix)sqapi-features.h"
> ''',
> - visit=visit))
> + visit=visit, prefix=self._prefix))
> self._genh.preamble_add(mcgen('''
> #include "qapi/qapi-builtin-visit.h"
> #include "%(types)s.h"
> diff --git a/tests/meson.build b/tests/meson.build
> index 907a4c1c98..a4ede66d0d 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -16,6 +16,8 @@ test_qapi_outputs = [
> 'test-qapi-events-sub-sub-module.h',
> 'test-qapi-events.c',
> 'test-qapi-events.h',
> + 'test-qapi-features.c',
> + 'test-qapi-features.h',
> 'test-qapi-init-commands.c',
> 'test-qapi-init-commands.h',
> 'test-qapi-introspect.c',
> diff --git a/tests/qapi-schema/features-too-many.err b/tests/qapi-schema/features-too-many.err
> new file mode 100644
> index 0000000000..bbbd6e5202
> --- /dev/null
> +++ b/tests/qapi-schema/features-too-many.err
> @@ -0,0 +1,2 @@
> +features-too-many.json: In command 'go-fish':
> +features-too-many.json:2: Maximum of 64 schema features is permitted
> diff --git a/tests/qapi-schema/features-too-many.json b/tests/qapi-schema/features-too-many.json
> new file mode 100644
> index 0000000000..aab0a0b5f1
> --- /dev/null
> +++ b/tests/qapi-schema/features-too-many.json
> @@ -0,0 +1,13 @@
> +# Max 64 features, with 2 specials, so 63rd custom is invalid
> +{ 'command': 'go-fish',
> + 'features': [
> + 'f00', 'f01', 'f02', 'f03', 'f04', 'f05', 'f06', 'f07',
> + 'f08', 'f09', 'f0a', 'f0b', 'f0c', 'f0d', 'f0e', 'f0f',
> + 'f10', 'f11', 'f12', 'f13', 'f14', 'f15', 'f16', 'f17',
> + 'f18', 'f19', 'f1a', 'f1b', 'f1c', 'f1d', 'f1e', 'f1f',
> + 'f20', 'f21', 'f22', 'f23', 'f24', 'f25', 'f26', 'f27',
> + 'f28', 'f29', 'f2a', 'f2b', 'f2c', 'f2d', 'f2e', 'f2f',
> + 'f30', 'f31', 'f32', 'f33', 'f34', 'f35', 'f36', 'f37',
> + 'f38', 'f39', 'f3a', 'f3b', 'f3c', 'f3d', 'f3e'
> + ]
> +}
> diff --git a/tests/qapi-schema/features-too-many.out b/tests/qapi-schema/features-too-many.out
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
> index 0f479d9317..9577178b6f 100644
> --- a/tests/qapi-schema/meson.build
> +++ b/tests/qapi-schema/meson.build
> @@ -105,6 +105,7 @@ schemas = [
> 'event-case.json',
> 'event-member-invalid-dict.json',
> 'event-nest-struct.json',
> + 'features-too-many.json',
> 'features-bad-type.json',
> 'features-deprecated-type.json',
> 'features-duplicate-name.json',
diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
index f32f9fe5f4..be3e5d03ff 100644
--- a/scripts/qapi/features.py
+++ b/scripts/qapi/features.py
@@ -7,7 +7,7 @@
# See the COPYING file in the top-level directory.
"""
-from typing import Dict
+from typing import Dict, ValuesView
from .common import c_enum_const, c_name
from .gen import QAPISchemaMonolithicCVisitor
@@ -25,7 +25,7 @@ def __init__(self, prefix: str):
' * Schema-defined QAPI features',
__doc__)
- self.features: Dict[str, QAPISchemaFeature] = {}
+ self.features: ValuesView[QAPISchemaFeature]
def visit_begin(self, schema: QAPISchema) -> None:
self.features = schema.features()
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 39c91af245..f27933d244 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -29,6 +29,7 @@
List,
Optional,
Union,
+ ValuesView,
cast,
)
@@ -1160,7 +1161,7 @@ def __init__(self, fname: str):
self._def_exprs(exprs)
self.check()
- def features(self) -> List[QAPISchemaFeature]:
+ def features(self) -> ValuesView[QAPISchemaFeature]:
return self._feature_dict.values()
def _def_entity(self, ent: QAPISchemaEntity) -> None:
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>
> > ---
> > meson.build | 1 +
> > scripts/qapi/commands.py | 1 +
> > scripts/qapi/features.py | 51 ++++++++++++++++++++++++
> > scripts/qapi/gen.py | 6 +--
> > scripts/qapi/main.py | 2 +
> > scripts/qapi/schema.py | 30 +++++++++++++-
> > scripts/qapi/types.py | 7 +++-
> > scripts/qapi/visit.py | 3 +-
> > tests/meson.build | 2 +
> > tests/qapi-schema/features-too-many.err | 2 +
> > tests/qapi-schema/features-too-many.json | 13 ++++++
> > tests/qapi-schema/features-too-many.out | 0
> > tests/qapi-schema/meson.build | 1 +
> > 13 files changed, 112 insertions(+), 7 deletions(-)
> > create mode 100644 scripts/qapi/features.py
> > create mode 100644 tests/qapi-schema/features-too-many.err
> > create mode 100644 tests/qapi-schema/features-too-many.json
> > create mode 100644 tests/qapi-schema/features-too-many.out
> >
> > diff --git a/meson.build b/meson.build
> > index 147097c652..3815878b23 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -3444,6 +3444,7 @@ qapi_gen_depends = [ meson.current_source_dir() /
> 'scripts/qapi/__init__.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/features.py',
> > meson.current_source_dir() /
> 'scripts/qapi/visit.py',
> > meson.current_source_dir() / 'scripts/qapi-gen.py'
> > ]
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index d629d2d97e..bf88bfc442 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -355,6 +355,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
> > #include "qemu/osdep.h"
> > #include "%(prefix)sqapi-commands.h"
> > #include "%(prefix)sqapi-init-commands.h"
> > +#include "%(prefix)sqapi-features.h"
> >
> > void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
> > {
> > 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.)
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.
(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.
Thank you for your patience,
--js
>
> Tentative fixup appended.
>
> > + self._genh.add("#include \"qapi/util.h\"\n\n")
> > +
> > + def visit_end(self) -> None:
> > + self._genh.add("typedef enum {\n")
> > + for f in self.features:
> > + self._genh.add(f" {c_enum_const('qapi_feature', f.name
> )}")
> > + if f.name in QAPISchemaFeature.SPECIAL_NAMES:
> > + self._genh.add(f" = {c_enum_const('qapi', f.name)},\n")
>
> More type confusion here:
>
> scripts/qapi/features.py:37: error: "str" has no attribute "name"
> [attr-defined]
> scripts/qapi/features.py:38: error: "str" has no attribute "name"
> [attr-defined]
> scripts/qapi/features.py:39: error: "str" has no attribute "name"
> [attr-defined]
>
> My fixup takes care of these, too.
>
> > + else:
> > + self._genh.add(",\n")
> > +
> > + self._genh.add("} " + c_name('QapiFeature') + ";\n")
> > +
> > +
> > +def gen_features(schema: QAPISchema,
> > + output_dir: str,
> > + prefix: str) -> None:
> > + vis = QAPISchemaGenFeatureVisitor(prefix)
> > + schema.visit(vis)
> > + vis.write(output_dir)
> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > index b51f8d955e..d3c56d45c8 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -42,9 +42,9 @@
> >
> >
> > def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
> > - featenum = [f"1u << {c_enum_const('qapi', feat.name)}"
> > - for feat in features if feat.is_special()]
> > - return ' | '.join(featenum) or '0'
> > + feats = [f"1u << {c_enum_const('qapi_feature', feat.name)}"
> > + for feat in features]
> > + return ' | '.join(feats) or '0'
> >
> >
> > class QAPIGen:
> > diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> > index 316736b6a2..2b9a2c0c02 100644
> > --- a/scripts/qapi/main.py
> > +++ b/scripts/qapi/main.py
> > @@ -18,6 +18,7 @@
> > from .introspect import gen_introspect
> > from .schema import QAPISchema
> > from .types import gen_types
> > +from .features import gen_features
> > from .visit import gen_visit
> >
> >
> > @@ -49,6 +50,7 @@ def generate(schema_file: str,
> >
> > schema = QAPISchema(schema_file)
> > gen_types(schema, output_dir, prefix, builtins)
> > + gen_features(schema, output_dir, prefix)
> > gen_visit(schema, output_dir, prefix, builtins)
> > gen_commands(schema, output_dir, prefix, gen_tracing)
> > gen_events(schema, output_dir, prefix)
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index e97c978d38..39c91af245 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -933,8 +933,11 @@ def connect_doc(self, doc: Optional[QAPIDoc]) ->
> None:
> > class QAPISchemaFeature(QAPISchemaMember):
> > role = 'feature'
> >
> > + # Features which are standardized across all schemas
> > + SPECIAL_NAMES = ['deprecated', 'unstable']
> > +
> > def is_special(self) -> bool:
> > - return self.name in ('deprecated', 'unstable')
> > + return self.name in QAPISchemaFeature.SPECIAL_NAMES
> >
> >
> > class QAPISchemaObjectTypeMember(QAPISchemaMember):
> > @@ -1138,6 +1141,16 @@ def __init__(self, fname: str):
> > self._entity_list: List[QAPISchemaEntity] = []
> > self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
> > self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
> > + # NB, values in the dict will identify the first encountered
> > + # usage of a named feature only
> > + self._feature_dict: Dict[str, QAPISchemaFeature] = OrderedDict()
> > +
> > + # All schemas get the names defined in the QapiSpecialFeature
> enum.
> > + # Use of OrderedDict ensures they are emitted first when
> generating
> > + # the enum definition, thus matching QapiSpecialFeature.
> > + for f in QAPISchemaFeature.SPECIAL_NAMES:
> > + self._feature_dict[f] = QAPISchemaFeature(f, None)
> > +
> > self._schema_dir = os.path.dirname(fname)
> > self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
> > self._make_module(fname)
> > @@ -1147,6 +1160,9 @@ def __init__(self, fname: str):
> > self._def_exprs(exprs)
> > self.check()
> >
> > + def features(self) -> List[QAPISchemaFeature]:
> > + return self._feature_dict.values()
>
> See typing trouble above.
>
> > +
> > def _def_entity(self, ent: QAPISchemaEntity) -> None:
> > self._entity_list.append(ent)
> >
> > @@ -1258,6 +1274,12 @@ def _make_features(
> > ) -> List[QAPISchemaFeature]:
> > if features is None:
> > return []
> > +
> > + for f in features:
> > + feat = QAPISchemaFeature(f['name'], info)
> > + if feat.name not in self._feature_dict:
> > + self._feature_dict[feat.name] = feat
> > +
> > return [QAPISchemaFeature(f['name'], info,
> > QAPISchemaIfCond(f.get('if')))
> > for f in features]
> > @@ -1485,6 +1507,12 @@ def check(self) -> None:
> > for doc in self.docs:
> > doc.check()
> >
> > + features = list(self._feature_dict.values())
> > + if len(features) > 64:
> > + raise QAPISemError(
> > + features[64].info,
> > + "Maximum of 64 schema features is permitted")
> > +
> > def visit(self, visitor: QAPISchemaVisitor) -> None:
> > visitor.visit_begin(self)
> > for mod in self._module_dict.values():
> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > index ade6b7a3d7..5294e5ea3b 100644
> > --- a/scripts/qapi/types.py
> > +++ b/scripts/qapi/types.py
> > @@ -308,11 +308,14 @@ def _begin_user_module(self, name: str) -> None:
> > #include "qapi/dealloc-visitor.h"
> > #include "%(types)s.h"
> > #include "%(visit)s.h"
> > +#include "%(prefix)sqapi-features.h"
> > ''',
> > - types=types, visit=visit))
> > + types=types, visit=visit,
> > + prefix=self._prefix))
> > self._genh.preamble_add(mcgen('''
> > #include "qapi/qapi-builtin-types.h"
> > -'''))
> > +''',
> > + prefix=self._prefix))
> >
> > def visit_begin(self, schema: QAPISchema) -> None:
> > # gen_object() is recursive, ensure it doesn't visit the empty
> type
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 8dbf4ef1c3..2d678c281d 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -360,8 +360,9 @@ def _begin_user_module(self, name: str) -> None:
> > #include "qemu/osdep.h"
> > #include "qapi/error.h"
> > #include "%(visit)s.h"
> > +#include "%(prefix)sqapi-features.h"
> > ''',
> > - visit=visit))
> > + visit=visit, prefix=self._prefix))
> > self._genh.preamble_add(mcgen('''
> > #include "qapi/qapi-builtin-visit.h"
> > #include "%(types)s.h"
> > diff --git a/tests/meson.build b/tests/meson.build
> > index 907a4c1c98..a4ede66d0d 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -16,6 +16,8 @@ test_qapi_outputs = [
> > 'test-qapi-events-sub-sub-module.h',
> > 'test-qapi-events.c',
> > 'test-qapi-events.h',
> > + 'test-qapi-features.c',
> > + 'test-qapi-features.h',
> > 'test-qapi-init-commands.c',
> > 'test-qapi-init-commands.h',
> > 'test-qapi-introspect.c',
> > diff --git a/tests/qapi-schema/features-too-many.err
> b/tests/qapi-schema/features-too-many.err
> > new file mode 100644
> > index 0000000000..bbbd6e5202
> > --- /dev/null
> > +++ b/tests/qapi-schema/features-too-many.err
> > @@ -0,0 +1,2 @@
> > +features-too-many.json: In command 'go-fish':
> > +features-too-many.json:2: Maximum of 64 schema features is permitted
> > diff --git a/tests/qapi-schema/features-too-many.json
> b/tests/qapi-schema/features-too-many.json
> > new file mode 100644
> > index 0000000000..aab0a0b5f1
> > --- /dev/null
> > +++ b/tests/qapi-schema/features-too-many.json
> > @@ -0,0 +1,13 @@
> > +# Max 64 features, with 2 specials, so 63rd custom is invalid
> > +{ 'command': 'go-fish',
> > + 'features': [
> > + 'f00', 'f01', 'f02', 'f03', 'f04', 'f05', 'f06', 'f07',
> > + 'f08', 'f09', 'f0a', 'f0b', 'f0c', 'f0d', 'f0e', 'f0f',
> > + 'f10', 'f11', 'f12', 'f13', 'f14', 'f15', 'f16', 'f17',
> > + 'f18', 'f19', 'f1a', 'f1b', 'f1c', 'f1d', 'f1e', 'f1f',
> > + 'f20', 'f21', 'f22', 'f23', 'f24', 'f25', 'f26', 'f27',
> > + 'f28', 'f29', 'f2a', 'f2b', 'f2c', 'f2d', 'f2e', 'f2f',
> > + 'f30', 'f31', 'f32', 'f33', 'f34', 'f35', 'f36', 'f37',
> > + 'f38', 'f39', 'f3a', 'f3b', 'f3c', 'f3d', 'f3e'
> > + ]
> > +}
> > diff --git a/tests/qapi-schema/features-too-many.out
> b/tests/qapi-schema/features-too-many.out
> > new file mode 100644
> > index 0000000000..e69de29bb2
> > diff --git a/tests/qapi-schema/meson.build
> b/tests/qapi-schema/meson.build
> > index 0f479d9317..9577178b6f 100644
> > --- a/tests/qapi-schema/meson.build
> > +++ b/tests/qapi-schema/meson.build
> > @@ -105,6 +105,7 @@ schemas = [
> > 'event-case.json',
> > 'event-member-invalid-dict.json',
> > 'event-nest-struct.json',
> > + 'features-too-many.json',
> > 'features-bad-type.json',
> > 'features-deprecated-type.json',
> > 'features-duplicate-name.json',
>
>
> diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
> index f32f9fe5f4..be3e5d03ff 100644
> --- a/scripts/qapi/features.py
> +++ b/scripts/qapi/features.py
> @@ -7,7 +7,7 @@
> # See the COPYING file in the top-level directory.
> """
>
> -from typing import Dict
> +from typing import Dict, ValuesView
>
> from .common import c_enum_const, c_name
> from .gen import QAPISchemaMonolithicCVisitor
> @@ -25,7 +25,7 @@ def __init__(self, prefix: str):
> ' * Schema-defined QAPI features',
> __doc__)
>
> - self.features: Dict[str, QAPISchemaFeature] = {}
> + self.features: ValuesView[QAPISchemaFeature]
>
> def visit_begin(self, schema: QAPISchema) -> None:
> self.features = schema.features()
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 39c91af245..f27933d244 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -29,6 +29,7 @@
> List,
> Optional,
> Union,
> + ValuesView,
> cast,
> )
>
> @@ -1160,7 +1161,7 @@ def __init__(self, fname: str):
> self._def_exprs(exprs)
> self.check()
>
> - def features(self) -> List[QAPISchemaFeature]:
> + def features(self) -> ValuesView[QAPISchemaFeature]:
> return self._feature_dict.values()
>
> def _def_entity(self, ent: QAPISchemaEntity) -> None:
>
>
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!
[...]
On Fri, Feb 7, 2025, 5:30 AM Markus Armbruster <armbru@redhat.com> wrote:
> 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.
'Kay, but let me answer your direct questions here, sorry. I'll switch to
v4 afterwards.
Yeah, list(iterable) just builds a list from the iterable, so it uses the
iterable, immutable "view" into the dict keys to build a list. The dict
view is a "live" object attached to the dict, the list is a static mutable
list fixed at time of copy.
(you could type it more accurately, but that can be annoying, so you can
just convert it to something "normal" like a list or tuple.)
> > 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?
>
Yep! Just wanted to offer them as part of this fixup/review to make it easy
for the both of you to run tests consistently. I know it's been a real PITA
to lint qapi, and I found a really simple way to ease that pain and wanted
to share right away.
> > (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.
>
Thanks, didn't realize there was a v4 since I'm not in the CC list there.
Add me to the replies if you have further questions or requests for advice.
> > Thank you for your patience,
> > --js
>
> Thanks for your help!
>
> [...]
>
>
© 2016 - 2025 Red Hat, Inc.