Hi, On Mon, Oct 02, 2023 at 04:09:29PM -0400, John Snow wrote: > On Mon, Oct 2, 2023 at 3:07 PM John Snow <js...@redhat.com> wrote: > > > > On Wed, Sep 27, 2023 at 7:25 AM Victor Toso <victort...@redhat.com> wrote: > > > > > > This patch handles QAPI enum types and generates its equivalent in Go. > > > > > > Basically, Enums are being handled as strings in Golang. > > > > > > 1. For each QAPI enum, we will define a string type in Go to be the > > > assigned type of this specific enum. > > > > > > 2. Naming: CamelCase will be used in any identifier that we want to > > > export [0], which is everything. > > > > > > [0] https://go.dev/ref/spec#Exported_identifiers > > > > > > Example: > > > > > > qapi: > > > | { 'enum': 'DisplayProtocol', > > > | 'data': [ 'vnc', 'spice' ] } > > > > > > go: > > > | type DisplayProtocol string > > > | > > > | const ( > > > | DisplayProtocolVnc DisplayProtocol = "vnc" > > > | DisplayProtocolSpice DisplayProtocol = "spice" > > > | ) > > > > > > Signed-off-by: Victor Toso <victort...@redhat.com> > > > --- > > > scripts/qapi/golang.py | 140 +++++++++++++++++++++++++++++++++++++++++ > > > scripts/qapi/main.py | 2 + > > > 2 files changed, 142 insertions(+) > > > create mode 100644 scripts/qapi/golang.py > > > > > > diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py > > > new file mode 100644 > > > index 0000000000..87081cdd05 > > > --- /dev/null > > > +++ b/scripts/qapi/golang.py > > > @@ -0,0 +1,140 @@ > > > +""" > > > +Golang QAPI generator > > > +""" > > > +# Copyright (c) 2023 Red Hat Inc. > > > +# > > > +# Authors: > > > +# Victor Toso <victort...@redhat.com> > > > +# > > > +# This work is licensed under the terms of the GNU GPL, version 2. > > > +# See the COPYING file in the top-level directory. > > > + > > > +# due QAPISchemaVisitor interface > > > +# pylint: disable=too-many-arguments > > "due to" - also, you could more selectively disable this warning by > putting this comment in the body of the QAPISchemaVisitor class which > would make your exemption from the linter more locally obvious. > > > > + > > > +# Just for type hint on self > > > +from __future__ import annotations > > Oh, you know - it's been so long since I worked on QAPI I didn't > realize we had access to this now. That's awesome! > > (It was introduced in Python 3.7+) > > > > + > > > +import os > > > +from typing import List, Optional > > > + > > > +from .schema import ( > > > + QAPISchema, > > > + QAPISchemaType, > > > + QAPISchemaVisitor, > > > + QAPISchemaEnumMember, > > > + QAPISchemaFeature, > > > + QAPISchemaIfCond, > > > + QAPISchemaObjectType, > > > + QAPISchemaObjectTypeMember, > > > + QAPISchemaVariants, > > > +) > > > +from .source import QAPISourceInfo > > > + > > Try running isort here: > > > cd ~/src/qemu/scripts > > isort -c qapi/golang.py > > ERROR: /home/jsnow/src/qemu/scripts/qapi/golang.py Imports are > incorrectly sorted and/or formatted. > > you can have it fix the import order for you: > > > isort qapi/golang.py > > It's very pedantic stuff, but luckily there's a tool to just handle it for > you.
Thanks! Also fixed for the next iteration.
> > > +TEMPLATE_ENUM = '''
> > > +type {name} string
> > > +const (
> > > +{fields}
> > > +)
> > > +'''
> > > +
> > > +
> > > +def gen_golang(schema: QAPISchema,
> > > + output_dir: str,
> > > + prefix: str) -> None:
> > > + vis = QAPISchemaGenGolangVisitor(prefix)
> > > + schema.visit(vis)
> > > + vis.write(output_dir)
> > > +
> > > +
> > > +def qapi_to_field_name_enum(name: str) -> str:
> > > + return name.title().replace("-", "")
> > > +
> > > +
> > > +class QAPISchemaGenGolangVisitor(QAPISchemaVisitor):
> > > +
> > > + def __init__(self, _: str):
> > > + super().__init__()
> > > + types = ["enum"]
> > > + self.target = {name: "" for name in types}
>
> you *could* say:
>
> types = ("enum",)
> self.target = dict.fromkeys(types, "")
>
> 1. We don't need a list because we won't be modifying it, so a tuple suffices
> 2. There's an idiom for doing what you want that reads a little better
> 3. None of it really matters, though.
No complains with moving it to a tuple.
> Also keep in mind you don't *need* to initialize a dict in this way,
> you can just arbitrarily assign into it whenever you'd like.
>
> sellf.target['enum'] = foo
I think it is a problem with += operator when not initialized.
self.target['enum'] = foo
At least I recall having errors around dict not being
initialized.
> I don't know if that makes things easier or not with however the
> subsequent patches are written.
>
> > > + self.schema = None
> > > + self.golang_package_name = "qapi"
> > > +
> > > + def visit_begin(self, schema):
> > > + self.schema = schema
> > > +
> > > + # Every Go file needs to reference its package name
> > > + for target in self.target:
> > > + self.target[target] = f"package {self.golang_package_name}\n"
> > > +
> > > + def visit_end(self):
> > > + self.schema = None
> > > +
> > > + def visit_object_type(self: QAPISchemaGenGolangVisitor,
> > > + name: str,
> > > + info: Optional[QAPISourceInfo],
> > > + ifcond: QAPISchemaIfCond,
> > > + features: List[QAPISchemaFeature],
> > > + base: Optional[QAPISchemaObjectType],
> > > + members: List[QAPISchemaObjectTypeMember],
> > > + variants: Optional[QAPISchemaVariants]
> > > + ) -> None:
> > > + pass
> > > +
> > > + def visit_alternate_type(self: QAPISchemaGenGolangVisitor,
> > > + name: str,
> > > + info: Optional[QAPISourceInfo],
> > > + ifcond: QAPISchemaIfCond,
> > > + features: List[QAPISchemaFeature],
> > > + variants: QAPISchemaVariants
> > > + ) -> None:
> > > + pass
> > > +
> > > + def visit_enum_type(self: QAPISchemaGenGolangVisitor,
>
> Was there a problem when you omitted the type for 'self'?
> Usually that can be inferred. As of this patch, at least, I
> think this can be safely dropped. (Maybe it becomes important
> later.)
I don't think I tried removing the type for self. I actually
tried to keep all types expressed, just for the sake of knowing
what types they were.
Yes, it can be easily inferred and removed.
> > > + name: str,
> > > + info: Optional[QAPISourceInfo],
> > > + ifcond: QAPISchemaIfCond,
> > > + features: List[QAPISchemaFeature],
> > > + members: List[QAPISchemaEnumMember],
> > > + prefix: Optional[str]
> > > + ) -> None:
> > > +
> > > + value = qapi_to_field_name_enum(members[0].name)
> >
> > Unsure if this was addressed on the mailing list yet, but in our call
> > we discussed how this call was vestigial and was causing the QAPI
> > tests to fail. Actually, I can't quite run "make check-qapi-schema"
> > and see the failure, I'm seeing it when I run "make check" and I'm not
> > sure how to find the failure more efficiently/quickly:
> >
> > jsnow@scv ~/s/q/build (review)> make
> > [1/60] Generating subprojects/dtc/version_gen.h with a custom command
> > [2/60] Generating qemu-version.h with a custom command (wrapped by
> > meson to capture output)
> > [3/44] Generating tests/Test QAPI files with a custom command
> > FAILED: tests/qapi-builtin-types.c tests/qapi-builtin-types.h
> > tests/qapi-builtin-visit.c tests/qapi-builtin-visit.h
> > tests/test-qapi-commands-sub-sub-module.c
> > tests/test-qapi-commands-sub-sub-module.h tests/test-qapi-commands.c
> > tests/test-qapi-commands.h tests/test-qapi-emit-events.c
> > tests/test-qapi-emit-events.h tests/test-qapi-events-sub-sub-module.c
> > tests/test-qapi-events-sub-sub-module.h tests/test-qapi-events.c
> > tests/test-qapi-events.h tests/test-qapi-init-commands.c
> > tests/test-qapi-init-commands.h tests/test-qapi-introspect.c
> > tests/test-qapi-introspect.h tests/test-qapi-types-sub-sub-module.c
> > tests/test-qapi-types-sub-sub-module.h tests/test-qapi-types.c
> > tests/test-qapi-types.h tests/test-qapi-visit-sub-sub-module.c
> > tests/test-qapi-visit-sub-sub-module.h tests/test-qapi-visit.c
> > tests/test-qapi-visit.h
> > /home/jsnow/src/qemu/build/pyvenv/bin/python3
> > /home/jsnow/src/qemu/scripts/qapi-gen.py -o
> > /home/jsnow/src/qemu/build/tests -b -p test-
> > ../tests/qapi-schema/qapi-schema-test.json --suppress-tracing
> > Traceback (most recent call last):
> > File "/home/jsnow/src/qemu/scripts/qapi-gen.py", line 19, in <module>
> > sys.exit(main.main())
> > ^^^^^^^^^^^
> > File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 96, in main
> > generate(args.schema,
> > File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 58, in generate
> > gen_golang(schema, output_dir, prefix)
> > File "/home/jsnow/src/qemu/scripts/qapi/golang.py", line 46, in gen_golang
> > schema.visit(vis)
> > File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 1227, in visit
> > mod.visit(visitor)
> > File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 209, in visit
> > entity.visit(visitor)
> > File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 346, in visit
> > visitor.visit_enum_type(
> > File "/home/jsnow/src/qemu/scripts/qapi/golang.py", line 102, in
> > visit_enum_type
> > value = qapi_to_field_name_enum(members[0].name)
> > ~~~~~~~^^^
> > IndexError: list index out of range
> > ninja: build stopped: subcommand failed.
> > make: *** [Makefile:162: run-ninja] Error 1
> >
> >
> > For the rest of my review, I commented this line out and continued on.
> >
> > > + fields = ""
> > > + for member in members:
> > > + value = qapi_to_field_name_enum(member.name)
> > > + fields += f'''\t{name}{value} {name} = "{member.name}"\n'''
> > > +
> > > + self.target["enum"] += TEMPLATE_ENUM.format(name=name,
> > > fields=fields[:-1])
>
> This line is a little too long. (sorry)
>
> try:
>
> cd ~/src/qemu/scripts
> flake8 qapi/
toso@tapioca ~/s/qemu > flake8 scripts/qapi | wc
89 734 6260
Yep, I'll fix them.
> jsnow@scv ~/s/q/scripts (review)> flake8 qapi/
> qapi/main.py:60:1: E302 expected 2 blank lines, found 1
> qapi/golang.py:106:80: E501 line too long (82 > 79 characters)
Cheers,
Victor
>
> > > +
> > > + def visit_array_type(self, name, info, ifcond, element_type):
> > > + pass
> > > +
> > > + def visit_command(self,
> > > + name: str,
> > > + info: Optional[QAPISourceInfo],
> > > + ifcond: QAPISchemaIfCond,
> > > + features: List[QAPISchemaFeature],
> > > + arg_type: Optional[QAPISchemaObjectType],
> > > + ret_type: Optional[QAPISchemaType],
> > > + gen: bool,
> > > + success_response: bool,
> > > + boxed: bool,
> > > + allow_oob: bool,
> > > + allow_preconfig: bool,
> > > + coroutine: bool) -> None:
> > > + pass
> > > +
> > > + def visit_event(self, name, info, ifcond, features, arg_type, boxed):
> > > + pass
> > > +
> > > + def write(self, output_dir: str) -> None:
> > > + for module_name, content in self.target.items():
> > > + go_module = module_name + "s.go"
> > > + go_dir = "go"
> > > + pathname = os.path.join(output_dir, go_dir, go_module)
> > > + odir = os.path.dirname(pathname)
> > > + os.makedirs(odir, exist_ok=True)
> > > +
> > > + with open(pathname, "w", encoding="ascii") as outfile:
> > > + outfile.write(content)
> > > diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> > > index 316736b6a2..cdbb3690fd 100644
> > > --- a/scripts/qapi/main.py
> > > +++ b/scripts/qapi/main.py
> > > @@ -15,6 +15,7 @@
> > > from .common import must_match
> > > from .error import QAPIError
> > > from .events import gen_events
> > > +from .golang import gen_golang
> > > from .introspect import gen_introspect
> > > from .schema import QAPISchema
> > > from .types import gen_types
> > > @@ -54,6 +55,7 @@ def generate(schema_file: str,
> > > gen_events(schema, output_dir, prefix)
> > > gen_introspect(schema, output_dir, prefix, unmask)
> > >
> > > + gen_golang(schema, output_dir, prefix)
> > >
> > > def main() -> int:
> > > """
> > > --
> > > 2.41.0
> > >
>
signature.asc
Description: PGP signature
