Re: [PATCH v2] qapi: pluggable backend code generators

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@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é <berrange@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 <armbru@redhat.com>

No need to respin, I can make the tweaks I suggested myself.  Feel free
to challenge my suggestions, of course.