Daniel P. Berrangé <berra...@redhat.com> writes:
> The 'qapi.backend.QAPIBackend' class defines an API contract for code
> generators. The current generator is put into a new class
> 'qapi.backend.QAPICBackend' and made to be the default impl.
>
> A custom generator can be requested using the '-k' arg which takes a
Missed an instance of -k. Can fix this myself.
> fully qualified python class name
>
> qapi-gen.py -B the.python.module.QAPIMyBackend
>
> This allows out of tree code to use the QAPI generator infrastructure
> to create new language bindings for QAPI schemas. This has the caveat
> that the QAPI generator APIs are not guaranteed stable, so consumers
> of this feature may have to update their code to be compatible with
> future QEMU releases.
>
> Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> ---
>
> In v2:
>
> - Create QAPISchema object ahead of calling backend
> - Use -B instead of -k
> - Fix mypy annotations
> - Add error checking when loading backend
> - Hardcode import of QAPICBackend to guarantee mypy coverage
>
> scripts/qapi/backend.py | 63 ++++++++++++++++++++++++++++++++++
> scripts/qapi/main.py | 75 ++++++++++++++++++++++-------------------
> 2 files changed, 103 insertions(+), 35 deletions(-)
> create mode 100644 scripts/qapi/backend.py
>
> diff --git a/scripts/qapi/backend.py b/scripts/qapi/backend.py
> new file mode 100644
> index 0000000000..14e60aa67a
> --- /dev/null
> +++ b/scripts/qapi/backend.py
> @@ -0,0 +1,63 @@
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +
> +from abc import ABC, abstractmethod
> +
> +from .commands import gen_commands
> +from .events import gen_events
> +from .features import gen_features
> +from .introspect import gen_introspect
> +from .schema import QAPISchema
> +from .types import gen_types
> +from .visit import gen_visit
> +
> +
> +class QAPIBackend(ABC):
> +
> + @abstractmethod
> + def generate(self,
> + schema: QAPISchema,
> + output_dir: str,
> + prefix: str,
> + unmask: bool,
> + builtins: bool,
> + gen_tracing: bool) -> None:
> + """
> + Generate code for the given schema into the target directory.
> +
> + :param schema: The primary QAPI schema object.
> + :param output_dir: The output directory to store generated code.
> + :param prefix: Optional C-code prefix for symbol names.
> + :param unmask: Expose non-ABI names through introspection?
> + :param builtins: Generate code for built-in types?
> +
> + :raise QAPIError: On failures.
> + """
> +
> +
> +class QAPICBackend(QAPIBackend):
> +
> + def generate(self,
> + schema: QAPISchema,
> + output_dir: str,
> + prefix: str,
> + unmask: bool,
> + builtins: bool,
> + gen_tracing: bool) -> None:
> + """
> + Generate C code for the given schema into the target directory.
> +
> + :param schema_file: The primary QAPI schema file.
> + :param output_dir: The output directory to store generated code.
> + :param prefix: Optional C-code prefix for symbol names.
> + :param unmask: Expose non-ABI names through introspection?
> + :param builtins: Generate code for built-in types?
> +
> + :raise QAPIError: On failures.
> + """
> + gen_types(schema, output_dir, prefix, builtins)
> + gen_features(schema, output_dir, prefix)
> + gen_visit(schema, output_dir, prefix, builtins)
> + gen_commands(schema, output_dir, prefix, gen_tracing)
> + gen_events(schema, output_dir, prefix)
> + gen_introspect(schema, output_dir, prefix, unmask)
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index 324081b9fc..8a8b1e0121 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -8,18 +8,14 @@
> """
>
> import argparse
> +from importlib import import_module
> import sys
> from typing import Optional
>
> -from .commands import gen_commands
> +from .backend import QAPIBackend, QAPICBackend
> from .common import must_match
> from .error import QAPIError
> -from .events import gen_events
> -from .features import gen_features
> -from .introspect import gen_introspect
> from .schema import QAPISchema
> -from .types import gen_types
> -from .visit import gen_visit
>
>
> def invalid_prefix_char(prefix: str) -> Optional[str]:
> @@ -29,32 +25,37 @@ def invalid_prefix_char(prefix: str) -> Optional[str]:
> return None
>
>
> -def generate(schema_file: str,
> - output_dir: str,
> - prefix: str,
> - unmask: bool = False,
> - builtins: bool = False,
> - gen_tracing: bool = False) -> None:
> - """
> - Generate C code for the given schema into the target directory.
> +def create_backend(path: str) -> QAPIBackend:
> + if path is None:
> + return QAPICBackend()
>
> - :param schema_file: The primary QAPI schema file.
> - :param output_dir: The output directory to store generated code.
> - :param prefix: Optional C-code prefix for symbol names.
> - :param unmask: Expose non-ABI names through introspection?
> - :param builtins: Generate code for built-in types?
> + if "." not in path:
> + print(f"Missing qualified module path in '{path}'", file=sys.stderr)
> + sys.exit(1)
>
> - :raise QAPIError: On failures.
> - """
> - assert invalid_prefix_char(prefix) is None
> + module_path, _, class_name = path.rpartition('.')
I'd like to tweak this to
module_path, dot, class_name = path.rpartition('.')
if not dot:
print(f"argument of -B must be of the form MODULE.CLASS",
file=sys.stderr)
sys.exit(1)
> + try:
> + mod = import_module(module_path)
> + except Exception as ex:
pylint complains:
scripts/qapi/main.py:39:11: W0718: Catching too general exception Exception
(broad-exception-caught)
I can't see offhand what exception(s) we're supposed to catch here, so
let's not worry about this now.
> + print(f"Unable to import '{module_path}': {ex}", file=sys.stderr)
> + sys.exit(1)
> +
> + if not hasattr(mod, class_name):
> + print(f"Module '{module_path}' has no class '{class_name}'",
> file=sys.stderr)
pycodestyle-3 complains:
scripts/qapi/main.py:44:80: E501 line too long (85 > 79 characters)
Let's break the line after the comma, and also start the error message
in lower case for consistency with error messages elsewhere.
> + sys.exit(1)
> + klass = getattr(mod, class_name)
Alternatively
try:
klass = getattr(mod, class_name)
except AttributeError:
print(f"module '{module_path}' has no class '{class_name}'",
file=sys.stderr)
sys.exit(1)
Admittedly a matter of taste. I tend to avoid the
if frobnicate would fail:
error out
frobnicate
pattern when practical.
> +
> + try:
> + backend = klass()
> +
> + if not isinstance(backend, QAPIBackend):
> + print(f"Backend '{path}' must be an instance of QAPIBackend",
> file=sys.stderr)
Likewise.
> + sys.exit(1)
>
> - schema = QAPISchema(schema_file)
> - gen_types(schema, output_dir, prefix, builtins)
> - gen_features(schema, output_dir, prefix)
> - gen_visit(schema, output_dir, prefix, builtins)
> - gen_commands(schema, output_dir, prefix, gen_tracing)
> - gen_events(schema, output_dir, prefix)
> - gen_introspect(schema, output_dir, prefix, unmask)
> + return backend
> + except Exception as ex:
Likewise too general exception.
I'd like to shrink the try block and reduce the nesting:
try:
backend = klass()
except Exception as ex:
print(f"Backend '{path}' cannot be instantiated: {ex}",
file=sys.stderr)
sys.exit(1)
if not isinstance(backend, QAPIBackend):
print(f"Backend '{path}' must be an instance of QAPIBackend",
file=sys.stderr)
sys.exit(1)
return backend
> + print(f"Backend '{path}' cannot be instantiated: {ex}",
> file=sys.stderr)
Likewise line too long.
> + sys.exit(1)
>
>
> def main() -> int:
> @@ -77,6 +78,8 @@ def main() -> int:
> parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
> dest='unmask',
> help="expose non-ABI names in introspection")
> + parser.add_argument('-B', '--backend', default=None,
> + help="Python module name for code generator")
>
> # Option --suppress-tracing exists so we can avoid solving build system
> # problems. TODO Drop it when we no longer need it.
> @@ -93,12 +96,14 @@ def main() -> int:
> return 1
>
> try:
> - generate(args.schema,
> - output_dir=args.output_dir,
> - prefix=args.prefix,
> - unmask=args.unmask,
> - builtins=args.builtins,
> - gen_tracing=not args.suppress_tracing)
> + schema = QAPISchema(args.schema)
> + backend = create_backend(args.backend)
> + backend.generate(schema,
> + output_dir=args.output_dir,
> + prefix=args.prefix,
> + unmask=args.unmask,
> + builtins=args.builtins,
> + gen_tracing=not args.suppress_tracing)
> except QAPIError as err:
> print(err, file=sys.stderr)
> return 1
Error reporting test cases:
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B nonexistent
qapi/qapi-schema.json
argument of -B must be of the form MODULE.CLASS
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B qapi.backend.foo
qapi/qapi-schema.json
module 'qapi.backend' has no class 'foo'
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B nonexistent
qapi/qapi-schema.json
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B nonexistent
qapi/qapi-schema.json
argument of -B must be of the form MODULE.CLASS
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B nonexistent.foo
qapi/qapi-schema.json
unable to import 'nonexistent': No module named 'nonexistent'
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B qapi.backend.foo
qapi/qapi-schema.json
module 'qapi.backend' has no class 'foo'
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B qapi.backend.QAPIBackend
qapi/qapi-schema.json
backend 'qapi.backend.QAPIBackend' cannot be instantiated: Can't instantiate
abstract class QAPIBackend without an implementation for abstract method
'generate'
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B qapi.common.Indentation
qapi/qapi-schema.json
backend 'qapi.common.Indentation' must be an instance of QAPIBackend
Good enough.
Reviewed-by: Markus Armbruster <arm...@redhat.com>
No need to respin, I can make the tweaks I suggested myself. Feel free
to challenge my suggestions, of course.
