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 :|