[PATCH] qapi: pluggable backend code generators

[PATCH] qapi: pluggable backend code generators

[Daniel P. Berrangé]

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 fully qualified python class name

   qapi-gen.py -k the.python.module.QAPIMyBackend

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---

This is an impl of the idea I mentioned at:

   https://lists.nongnu.org/archive/html/qemu-devel/2025-02/msg03475.html

With this change, it is possible for the Go generator code to live
outside of qemu.git, invoked using:

  $ PYTHONPATH=/path/to/qemu.git/scripts \
    python /path/to/qemu.git/scripts/qapi-gen.py \
      -o somedir \
      -k qapi.golang.golang.QAPIGoBackend \
      /path/to/qemu.git/qga/qapi-schema.json

The external app could just expect qemu.git to be checkedout somewhere
convenient, or could use a git submodule to reference it.

 scripts/qapi/backend.py | 96 +++++++++++++++++++++++++++++++++++++++++
 scripts/qapi/main.py    | 65 ++++++++--------------------
 2 files changed, 113 insertions(+), 48 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..b6873fd2e3
--- /dev/null
+++ b/scripts/qapi/backend.py
@@ -0,0 +1,96 @@
+# 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 typing import Optional
+
+from .commands import gen_commands
+from .common import must_match
+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]:
+    match = must_match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+    if match.end() != len(prefix):
+        return prefix[match.end()]
+    return None
+
+
+class QAPIBackend(ABC):
+
+    def run(self,
+            schema_file: str,
+            output_dir: str,
+            prefix: str,
+            unmask: bool = False,
+            builtins: bool = False,
+            gen_tracing: bool = False) -> None:
+        """
+        Run the code generator 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.
+        """
+        assert invalid_prefix_char(prefix) is None
+
+        schema = QAPISchema(schema_file)
+        self.generate(schema, output_dir, prefix, unmask, builtins, gen_tracing)
+
+    @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.
+        """
+        pass
+
+
+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..35552dffce 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -8,53 +8,18 @@
 """
 
 import argparse
+from importlib import import_module
 import sys
-from typing import Optional
 
-from .commands import gen_commands
-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
+from .backend import invalid_prefix_char
 
 
-def invalid_prefix_char(prefix: str) -> Optional[str]:
-    match = must_match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
-    if match.end() != len(prefix):
-        return prefix[match.end()]
-    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.
-
-    :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.
-    """
-    assert invalid_prefix_char(prefix) is None
-
-    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)
+def import_class_from_string(path):
+    module_path, _, class_name = path.rpartition('.')
+    mod = import_module(module_path)
+    klass = getattr(mod, class_name)
+    return klass
 
 
 def main() -> int:
@@ -77,6 +42,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('-k', '--backend', default="qapi.backend.QAPICBackend",
+                        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.
@@ -92,13 +59,15 @@ def main() -> int:
         print(f"{sys.argv[0]}: {msg}", file=sys.stderr)
         return 1
 
+    backendclass = import_class_from_string(args.backend)
     try:
-        generate(args.schema,
-                 output_dir=args.output_dir,
-                 prefix=args.prefix,
-                 unmask=args.unmask,
-                 builtins=args.builtins,
-                 gen_tracing=not args.suppress_tracing)
+        backend = backendclass()
+        backend.run(args.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
-- 
2.47.1

Re: [PATCH] qapi: pluggable backend code generators

[Philippe Mathieu-Daudé]

On 17/2/25 14:49, Daniel P. Berrangé wrote:
> 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 fully qualified python class name
> 
>     qapi-gen.py -k the.python.module.QAPIMyBackend
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> 
> This is an impl of the idea I mentioned at:
> 
>     https://lists.nongnu.org/archive/html/qemu-devel/2025-02/msg03475.html
> 
> With this change, it is possible for the Go generator code to live
> outside of qemu.git, invoked using:
> 
>    $ PYTHONPATH=/path/to/qemu.git/scripts \
>      python /path/to/qemu.git/scripts/qapi-gen.py \
>        -o somedir \
>        -k qapi.golang.golang.QAPIGoBackend \
>        /path/to/qemu.git/qga/qapi-schema.json
> 
> The external app could just expect qemu.git to be checkedout somewhere
> convenient, or could use a git submodule to reference it.
> 
>   scripts/qapi/backend.py | 96 +++++++++++++++++++++++++++++++++++++++++
>   scripts/qapi/main.py    | 65 ++++++++--------------------
>   2 files changed, 113 insertions(+), 48 deletions(-)
>   create mode 100644 scripts/qapi/backend.py

Good idea.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] qapi: pluggable backend code generators

[Markus Armbruster]

Cc: John for advice on my somewhat nebulous mypy worries at the end.

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 fully qualified python class name
>
>    qapi-gen.py -k the.python.module.QAPIMyBackend

I understand why that will be useful, but explaining it briefly in the
commit message wouldn't hurt.

>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>
> This is an impl of the idea I mentioned at:
>
>    https://lists.nongnu.org/archive/html/qemu-devel/2025-02/msg03475.html
>
> With this change, it is possible for the Go generator code to live
> outside of qemu.git, invoked using:
>
>   $ PYTHONPATH=/path/to/qemu.git/scripts \
>     python /path/to/qemu.git/scripts/qapi-gen.py \
>       -o somedir \
>       -k qapi.golang.golang.QAPIGoBackend \
>       /path/to/qemu.git/qga/qapi-schema.json
>
> The external app could just expect qemu.git to be checkedout somewhere
> convenient, or could use a git submodule to reference it.
>
>  scripts/qapi/backend.py | 96 +++++++++++++++++++++++++++++++++++++++++
>  scripts/qapi/main.py    | 65 ++++++++--------------------
>  2 files changed, 113 insertions(+), 48 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..b6873fd2e3
> --- /dev/null
> +++ b/scripts/qapi/backend.py
> @@ -0,0 +1,96 @@
> +# 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 typing import Optional
> +
> +from .commands import gen_commands
> +from .common import must_match
> +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]:
> +    match = must_match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
> +    if match.end() != len(prefix):
> +        return prefix[match.end()]
> +    return None
> +
> +
> +class QAPIBackend(ABC):
> +
> +    def run(self,
> +            schema_file: str,
> +            output_dir: str,
> +            prefix: str,
> +            unmask: bool = False,
> +            builtins: bool = False,
> +            gen_tracing: bool = False) -> None:
> +        """
> +        Run the code generator 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.
> +        """
> +        assert invalid_prefix_char(prefix) is None
> +
> +        schema = QAPISchema(schema_file)

Hmm.  This makes the backend run the frontend.  Could we keep this in
main.py instead?

> +        self.generate(schema, output_dir, prefix, unmask, builtins, gen_tracing)
> +
> +    @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.
> +        """
> +        pass

pylint prefers not to pass:

    scripts/qapi/backend.py:68:8: W0107: Unnecessary pass statement (unnecessary-pass)

> +
> +
> +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..35552dffce 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -8,53 +8,18 @@
>  """
>  
>  import argparse
> +from importlib import import_module
>  import sys
> -from typing import Optional
>  
> -from .commands import gen_commands
> -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
> +from .backend import invalid_prefix_char

isort wants you to put this above from .error, like this:

   from .backend import invalid_prefix_char
   from .error import QAPIError

>  
>  
> -def invalid_prefix_char(prefix: str) -> Optional[str]:
> -    match = must_match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
> -    if match.end() != len(prefix):
> -        return prefix[match.end()]
> -    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.
> -
> -    :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.
> -    """
> -    assert invalid_prefix_char(prefix) is None
> -
> -    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)
> +def import_class_from_string(path):
> +    module_path, _, class_name = path.rpartition('.')
> +    mod = import_module(module_path)
> +    klass = getattr(mod, class_name)
> +    return klass

Lacks error handling, see appended test cases.

Moreover, mypy points out

    scripts/qapi/main.py:18: error: Function is missing a type annotation  [no-untyped-def]

The argument is str, but what is returned?

The function name suggests it returns a class.

As coded, the function could return pretty much anything.

The caller actually needs a QAPIBackend class.

We need a checked cast to QAPIBackend class somewhere.  If you put it in
this function, it returns a QAPIBackend class, and its name should be
adjusted accordingly.  import_backend() maybe?

>  
>  
>  def main() -> int:
> @@ -77,6 +42,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('-k', '--backend', default="qapi.backend.QAPICBackend",

Any particular reason for picking -k for --backend?

-b is taken, but -B would be available.

Break the line before default=, please.

> +                        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.
> @@ -92,13 +59,15 @@ def main() -> int:
>          print(f"{sys.argv[0]}: {msg}", file=sys.stderr)
>          return 1
>  
> +    backendclass = import_class_from_string(args.backend)
>      try:
> -        generate(args.schema,
> -                 output_dir=args.output_dir,
> -                 prefix=args.prefix,
> -                 unmask=args.unmask,
> -                 builtins=args.builtins,
> -                 gen_tracing=not args.suppress_tracing)
> +        backend = backendclass()
> +        backend.run(args.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

The connection to the backend moves to run-time: static import
statements get replaced by a dynamic import_module().  Fine, it's what
it takes to support pluggable backends.

However, it might hide the bundled backend from tools like mypy.  Would
that be bad?  I'm not sure.

If it is, we could avoid it pretty easily: instead of defaulting the
module name, so we dynamically load the bundled backend module by
default, default the class, so we create the bundled backend class by
default.


The promised test cases:

$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -k noexistent qapi/qapi-schema.json 
Traceback (most recent call last):
  File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
    sys.exit(main.main())
             ~~~~~~~~~^^
  File "/work/armbru/qemu/scripts/qapi/main.py", line 62, in main
    backendclass = import_class_from_string(args.backend)
  File "/work/armbru/qemu/scripts/qapi/main.py", line 20, in import_class_from_string
    mod = import_module(module_path)
  File "/usr/lib64/python3.13/importlib/__init__.py", line 88, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1384, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1298, in _sanity_check
ValueError: Empty module name

 python3 scripts/qapi-gen.py -o /tmp/$$ -b -k noexistent.foo qapi/qapi-schema.json 
Traceback (most recent call last):
  File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
    sys.exit(main.main())
             ~~~~~~~~~^^
  File "/work/armbru/qemu/scripts/qapi/main.py", line 62, in main
    backendclass = import_class_from_string(args.backend)
  File "/work/armbru/qemu/scripts/qapi/main.py", line 20, in import_class_from_string
    mod = import_module(module_path)
  File "/usr/lib64/python3.13/importlib/__init__.py", line 88, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1324, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'noexistent'

$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -k qapi.backend.foo qapi/qapi-schema.json 
Traceback (most recent call last):
  File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
    sys.exit(main.main())
             ~~~~~~~~~^^
  File "/work/armbru/qemu/scripts/qapi/main.py", line 62, in main
    backendclass = import_class_from_string(args.backend)
  File "/work/armbru/qemu/scripts/qapi/main.py", line 21, in import_class_from_string
    klass = getattr(mod, class_name)
AttributeError: module 'qapi.backend' has no attribute 'foo'

$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -k qapi.backend.QAPIBackend qapi/qapi-schema.json 
Traceback (most recent call last):
  File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
    sys.exit(main.main())
             ~~~~~~~~~^^
  File "/work/armbru/qemu/scripts/qapi/main.py", line 64, in main
    backend = backendclass()
TypeError: Can't instantiate abstract class QAPIBackend without an implementation for abstract method 'generate'

 $ python3 scripts/qapi-gen.py -o /tmp/$$ -b -k qapi.common.Indentation qapi/qapi-schema.json 
Traceback (most recent call last):
  File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
    sys.exit(main.main())
             ~~~~~~~~~^^
  File "/work/armbru/qemu/scripts/qapi/main.py", line 65, in main
    backend.run(args.schema,
    ^^^^^^^^^^^
AttributeError: 'Indentation' object has no attribute 'run'

Re: [PATCH] qapi: pluggable backend code generators

[Daniel P. Berrangé]

On Thu, Feb 20, 2025 at 08:58:17AM +0100, Markus Armbruster wrote:
> Cc: John for advice on my somewhat nebulous mypy worries at the end.
> 
> 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 fully qualified python class name
> >
> >    qapi-gen.py -k the.python.module.QAPIMyBackend
> 
> I understand why that will be useful, but explaining it briefly in the
> commit message wouldn't hurt.

I expanded on this.


> > +class QAPIBackend(ABC):
> > +
> > +    def run(self,
> > +            schema_file: str,
> > +            output_dir: str,
> > +            prefix: str,
> > +            unmask: bool = False,
> > +            builtins: bool = False,
> > +            gen_tracing: bool = False) -> None:
> > +        """
> > +        Run the code generator 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.
> > +        """
> > +        assert invalid_prefix_char(prefix) is None
> > +
> > +        schema = QAPISchema(schema_file)
> 
> Hmm.  This makes the backend run the frontend.  Could we keep this in
> main.py instead?

Yes, that is better, and eliminates the need for the extra
'run' method to call 'generate'.

> > +        self.generate(schema, output_dir, prefix, unmask, builtins, gen_tracing)
> > +
> > +    @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.
> > +        """
> > +        pass
> 
> pylint prefers not to pass:
> 
>     scripts/qapi/backend.py:68:8: W0107: Unnecessary pass statement (unnecessary-pass)

Fun, I expected it to be a syntax error to not have a statement in the
method body, but pylint is right.


> > diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> > index 324081b9fc..35552dffce 100644
> > --- a/scripts/qapi/main.py
> > +++ b/scripts/qapi/main.py
> > @@ -8,53 +8,18 @@
> >  """
> >  
> >  import argparse
> > +from importlib import import_module
> >  import sys
> > -from typing import Optional
> >  
> > -from .commands import gen_commands
> > -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
> > +from .backend import invalid_prefix_char
> 
> isort wants you to put this above from .error, like this:
> 
>    from .backend import invalid_prefix_char
>    from .error import QAPIError

Yep, I forgot we sorted imports


> > +def import_class_from_string(path):
> > +    module_path, _, class_name = path.rpartition('.')
> > +    mod = import_module(module_path)
> > +    klass = getattr(mod, class_name)
> > +    return klass
> 
> Lacks error handling, see appended test cases.
> 
> Moreover, mypy points out
> 
>     scripts/qapi/main.py:18: error: Function is missing a type annotation  [no-untyped-def]
> 
> The argument is str, but what is returned?
> 
> The function name suggests it returns a class.
> 
> As coded, the function could return pretty much anything.
> 
> The caller actually needs a QAPIBackend class.
> 
> We need a checked cast to QAPIBackend class somewhere.  If you put it in
> this function, it returns a QAPIBackend class, and its name should be
> adjusted accordingly.  import_backend() maybe?

Yeah, with all the error checking needed for the scenarios you
illustrate below, this method was better turned into a more
specialized "create_backend" method.

> 
> >  
> >  
> >  def main() -> int:
> > @@ -77,6 +42,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('-k', '--backend', default="qapi.backend.QAPICBackend",
> 
> Any particular reason for picking -k for --backend?

There is a 'k' in 'backend' was the extent of my decision making:-)

> -b is taken, but -B would be available.

I've used -B now.


> > @@ -92,13 +59,15 @@ def main() -> int:
> >          print(f"{sys.argv[0]}: {msg}", file=sys.stderr)
> >          return 1
> >  
> > +    backendclass = import_class_from_string(args.backend)
> >      try:
> > -        generate(args.schema,
> > -                 output_dir=args.output_dir,
> > -                 prefix=args.prefix,
> > -                 unmask=args.unmask,
> > -                 builtins=args.builtins,
> > -                 gen_tracing=not args.suppress_tracing)
> > +        backend = backendclass()
> > +        backend.run(args.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
> 
> The connection to the backend moves to run-time: static import
> statements get replaced by a dynamic import_module().  Fine, it's what
> it takes to support pluggable backends.
> 
> However, it might hide the bundled backend from tools like mypy.  Would
> that be bad?  I'm not sure.
> 
> If it is, we could avoid it pretty easily: instead of defaulting the
> module name, so we dynamically load the bundled backend module by
> default, default the class, so we create the bundled backend class by
> default.

Yes, in v2 I've kept QAPICBackend as an explicit import so we
avoid any questions about hiding from mypy.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|