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