| Message ID | 20230927112544.85011-2-victortoso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi-go: add generator for Golang interface | expand |
On Wed, Sep 27, 2023 at 01:25:36PM +0200, Victor Toso 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 <victortoso@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 <victortoso@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 > + > +# Just for type hint on self > +from __future__ import annotations > + > +import os > +from typing import List, Optional > + > +from .schema import ( > + QAPISchema, > + QAPISchemaType, > + QAPISchemaVisitor, > + QAPISchemaEnumMember, > + QAPISchemaFeature, > + QAPISchemaIfCond, > + QAPISchemaObjectType, > + QAPISchemaObjectTypeMember, > + QAPISchemaVariants, > +) > +from .source import QAPISourceInfo > + > +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} > + 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, > + 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) > + 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]) Here you are formatting the enums as you visit them, appending to the output buffer. The resulting enums appear in whatever order we visited them with, which is pretty arbitrary. Browsing the generated Go code to understand it, I find myself wishing that it was emitted in alphabetical order. This could be done if we worked in two phase. In the visit phase, we collect the bits of data we need, and then add a format phase then generates the formatted output, having first sorted by enum name. Same thought for the other types/commands. > + > + 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: IIUC, we defacto consider the .qapi json files to be UTF-8, and thus in theory we could have non-ascii characters in there somewhere. I'd suggest we using utf8 encoding when outputting to avoid surprises. > + outfile.write(content) With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Sep 27, 2023 at 01:25:36PM +0200, Victor Toso 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 <victortoso@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 <victortoso@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 >> + >> +# Just for type hint on self >> +from __future__ import annotations >> + >> +import os >> +from typing import List, Optional >> + >> +from .schema import ( >> + QAPISchema, >> + QAPISchemaType, >> + QAPISchemaVisitor, >> + QAPISchemaEnumMember, >> + QAPISchemaFeature, >> + QAPISchemaIfCond, >> + QAPISchemaObjectType, >> + QAPISchemaObjectTypeMember, >> + QAPISchemaVariants, >> +) >> +from .source import QAPISourceInfo >> + >> +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} >> + 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, >> + 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) >> + 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]) > > Here you are formatting the enums as you visit them, appending to > the output buffer. The resulting enums appear in whatever order we > visited them with, which is pretty arbitrary. We visit in source order, not in arbitrary order. > Browsing the generated Go code to understand it, I find myself > wishing that it was emitted in alphabetical order. If that's easier to read in generated Go, then I suspect it would also be easier to read in the QAPI schema and in generated C. > This could be done if we worked in two phase. In the visit phase, > we collect the bits of data we need, and then add a format phase > then generates the formatted output, having first sorted by enum > name. > > Same thought for the other types/commands. > >> + >> + 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: > > IIUC, we defacto consider the .qapi json files to be UTF-8, and thus > in theory we could have non-ascii characters in there somewhere. I'd > suggest we using utf8 encoding when outputting to avoid surprises. Seconded. QAPIGen.write() already uses encoding='utf-8' for writing generated files. >> + outfile.write(content) > > > With regards, > Daniel
On Thu, Sep 28, 2023 at 04:20:55PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Wed, Sep 27, 2023 at 01:25:36PM +0200, Victor Toso 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 <victortoso@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 <victortoso@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 > >> + > >> +# Just for type hint on self > >> +from __future__ import annotations > >> + > >> +import os > >> +from typing import List, Optional > >> + > >> +from .schema import ( > >> + QAPISchema, > >> + QAPISchemaType, > >> + QAPISchemaVisitor, > >> + QAPISchemaEnumMember, > >> + QAPISchemaFeature, > >> + QAPISchemaIfCond, > >> + QAPISchemaObjectType, > >> + QAPISchemaObjectTypeMember, > >> + QAPISchemaVariants, > >> +) > >> +from .source import QAPISourceInfo > >> + > >> +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} > >> + 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, > >> + 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) > >> + 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]) > > > > Here you are formatting the enums as you visit them, appending to > > the output buffer. The resulting enums appear in whatever order we > > visited them with, which is pretty arbitrary. > > We visit in source order, not in arbitrary order. I meant arbitrary in the sense that us developers just add new QAPI types pretty much anywhere we feel like it in the .qapi files. > > > Browsing the generated Go code to understand it, I find myself > > wishing that it was emitted in alphabetical order. > > If that's easier to read in generated Go, then I suspect it would also > be easier to read in the QAPI schema and in generated C. Yes, although C has some ordering constraints on things being declared, so it would be a bit harder to do this in C. With regards, Daniel
Hi, On Thu, Sep 28, 2023 at 02:52:08PM +0100, Daniel P. Berrangé wrote: > On Wed, Sep 27, 2023 at 01:25:36PM +0200, Victor Toso 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 <victortoso@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 <victortoso@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 > > + > > +# Just for type hint on self > > +from __future__ import annotations > > + > > +import os > > +from typing import List, Optional > > + > > +from .schema import ( > > + QAPISchema, > > + QAPISchemaType, > > + QAPISchemaVisitor, > > + QAPISchemaEnumMember, > > + QAPISchemaFeature, > > + QAPISchemaIfCond, > > + QAPISchemaObjectType, > > + QAPISchemaObjectTypeMember, > > + QAPISchemaVariants, > > +) > > +from .source import QAPISourceInfo > > + > > +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} > > + 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, > > + 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) > > + 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]) > > Here you are formatting the enums as you visit them, appending to > the output buffer. The resulting enums appear in whatever order we > visited them with, which is pretty arbitrary. > > Browsing the generated Go code to understand it, I find myself > wishing that it was emitted in alphabetical order. > > This could be done if we worked in two phase. In the visit phase, > we collect the bits of data we need, and then add a format phase > then generates the formatted output, having first sorted by enum > name. > > Same thought for the other types/commands. I cared for sorted in some places [0] but not all of them indeed. I'll include your request/suggestion in the next version. [0] https://gitlab.com/victortoso/qemu/-/blob/qapi-golang-v1/scripts/qapi/golang.py?ref_type=heads#L804 > > > + > > + 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: > > IIUC, we defacto consider the .qapi json files to be UTF-8, and thus > in theory we could have non-ascii characters in there somewhere. I'd > suggest we using utf8 encoding when outputting to avoid surprises. Sure thing. Cheers, Victor
On Wed, Sep 27, 2023 at 7:25 AM Victor Toso <victortoso@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 <victortoso@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 <victortoso@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 > + > +# Just for type hint on self > +from __future__ import annotations > + > +import os > +from typing import List, Optional > + > +from .schema import ( > + QAPISchema, > + QAPISchemaType, > + QAPISchemaVisitor, > + QAPISchemaEnumMember, > + QAPISchemaFeature, > + QAPISchemaIfCond, > + QAPISchemaObjectType, > + QAPISchemaObjectTypeMember, > + QAPISchemaVariants, > +) > +from .source import QAPISourceInfo > + > +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} > + 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, > + 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]) > + > + 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 >
On Mon, Oct 2, 2023 at 3:07 PM John Snow <jsnow@redhat.com> wrote: > > On Wed, Sep 27, 2023 at 7:25 AM Victor Toso <victortoso@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 <victortoso@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 <victortoso@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. > > +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. 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 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.) > > + 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/ 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) > > + > > + 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 > >
Hi, On Mon, Oct 02, 2023 at 03:07:49PM -0400, John Snow wrote: > On Wed, Sep 27, 2023 at 7:25 AM Victor Toso <victortoso@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 <victortoso@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 <victortoso@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 > > + > > +# Just for type hint on self > > +from __future__ import annotations > > + > > +import os > > +from typing import List, Optional > > + > > +from .schema import ( > > + QAPISchema, > > + QAPISchemaType, > > + QAPISchemaVisitor, > > + QAPISchemaEnumMember, > > + QAPISchemaFeature, > > + QAPISchemaIfCond, > > + QAPISchemaObjectType, > > + QAPISchemaObjectTypeMember, > > + QAPISchemaVariants, > > +) > > +from .source import QAPISourceInfo > > + > > +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} > > + 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, > > + 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. Yes, it was a leftover when I was reorganizing the patches. You can see this value is not actually used. I removed it in my branch for v2. Cheers, Victor > > + 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]) > > + > > + 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 > > >
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 <jsnow@redhat.com> wrote: > > > > On Wed, Sep 27, 2023 at 7:25 AM Victor Toso <victortoso@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 <victortoso@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 <victortoso@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 > > > >
On Wed, Oct 4, 2023, 8:43 AM Victor Toso <victortoso@redhat.com> wrote: > 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 <jsnow@redhat.com> wrote: > > > > > > On Wed, Sep 27, 2023 at 7:25 AM Victor Toso <victortoso@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 <victortoso@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 <victortoso@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. > ah, okay. You can also do: self.target.setdefault("enum", "") += "blah" but it's also fine to initialize up front. just teaching you a trick in case it helps. > > 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. > Normally I'm also in favor of being explicit, but where python is concerned this may interfere with inheritance. Usually it's best to leave self untyped because it avoids cyclical references and it also behaves correctly in the type tree. There are idioms for how to express a return type of "self" if that becomes needed. > > > > + 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 > > > > > > >
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 <victortoso@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 + +# Just for type hint on self +from __future__ import annotations + +import os +from typing import List, Optional + +from .schema import ( + QAPISchema, + QAPISchemaType, + QAPISchemaVisitor, + QAPISchemaEnumMember, + QAPISchemaFeature, + QAPISchemaIfCond, + QAPISchemaObjectType, + QAPISchemaObjectTypeMember, + QAPISchemaVariants, +) +from .source import QAPISourceInfo + +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} + 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, + 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) + 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]) + + 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: """
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 <victortoso@redhat.com> --- scripts/qapi/golang.py | 140 +++++++++++++++++++++++++++++++++++++++++ scripts/qapi/main.py | 2 + 2 files changed, 142 insertions(+) create mode 100644 scripts/qapi/golang.py