Daniel P. Berrangé <berra...@redhat.com> writes:
> On Tue, Feb 25, 2025 at 01:31:56PM +0100, Markus Armbruster wrote:
>> 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>
>> > ---
>
>> > -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)
>
> Yep, sure thing.
>
>>
>> > + 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.
>
> Yeah, I was concerned that by putting specialized exception
> classes here, we would miss some possible scenarios, and IMHO
> the completeness of catching Exception is better than the
> technical purity of pylint's complaint.
Cleaner code would require a stronger contract.
We'll be just fine.
>> > + 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.
>
> I guess I tend to avoid using exception catching for such flow
> control, but I don't mind that much either way.
I'm not a fan of using exceptions for mundane failures, but it's how
Python rolls.
>> > - 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
>
> Sure, fine with me.
>
>
>> 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.
>
> Thank you, it is all fine.
Cool, expect it in my next pull request. Thanks!
