John Snow <jsnow@redhat.com> writes:
On 10/7/20 3:54 AM, Markus Armbruster wrote:
John Snow <jsnow@redhat.com> writes:
On 10/6/20 7:51 AM, Markus Armbruster wrote:
John Snow <jsnow@redhat.com> writes:
This is a minor re-work of the entrypoint script. It isolates a
generate() method from the actual command-line mechanism.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
---
scripts/qapi-gen.py | 85 +++++++++++++++++++++++++++++++++------------
1 file changed, 62 insertions(+), 23 deletions(-)
diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
index 541e8c1f55d..117b396a595 100644
--- a/scripts/qapi-gen.py
+++ b/scripts/qapi-gen.py
@@ -1,30 +1,77 @@
#!/usr/bin/env python3
-# QAPI generator
-#
+
# This work is licensed under the terms of the GNU GPL, version 2 or later.
# See the COPYING file in the top-level directory.
+"""
+QAPI Generator
+
+This script is the main entry point for generating C code from the QAPI schema.
+"""
import argparse
import re
import sys
from qapi.commands import gen_commands
+from qapi.error import QAPIError
from qapi.events import gen_events
from qapi.introspect import gen_introspect
-from qapi.schema import QAPIError, QAPISchema
+from qapi.schema import QAPISchema
from qapi.types import gen_types
from qapi.visit import gen_visit
Unrelated cleanup. Okay.
-def main(argv):
+DEFAULT_OUTPUT_DIR = ''
+DEFAULT_PREFIX = ''
+
+
+def generate(schema_file: str,
+ output_dir: str,
+ prefix: str,
+ unmask: bool = False,
+ builtins: bool = False) -> None:
+ """
+ generate uses a given schema to produce C code in 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.
+ """
+ match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+ if match.end() != len(prefix):
+ msg = "funny character '{:s}' in prefix '{:s}'".format(
+ prefix[match.end()], prefix)
+ raise QAPIError('', None, msg)
Uh...
$ python3 scripts/qapi-gen.py --prefix=@ x
scripts/qapi-gen.py: : funny character '@' in prefix '@'
Unwanted " :".
This is due to a hack: you pass '' for info (*quack*). Everything
else
passes QAPISourceInfo (I believe).
Quack indeed - why does our base error class require so much
information from a specific part of the generation process?
Because it's not "a base error class", it's a base error class for
the
QAPI schema compiler frontend.
Well. It's the base for every error we /had/.
You asked why the class has the init it has, and I answered :)
Ah, someone changes this in part 4 so that we have a more generic
error class to use as a base when we are missing such information.
Evolving it to satisfy a need for a more widely usable error class
is
okay.
Yep. It's helpful to keep a very generic form on which we grow other
errors from, so that things like the entry point can be written
legibly.
If you have a non-trivial error message format convention, you have a
use for a function formatting error messages.
If you have a separation between diagnose and report of errors, you you
have a use for a transport from diagnose to report. In Python, that's
raise.
The existing error message in main() has neither.
The existing error class QAPIError caters for the existing users.
You are witnessing some more future-bleed.
Is it really a good idea to do this in generate? It's not about
generating code, it's about validating a CLI option.
One might also ask: Is it a good idea to only validate this on a
frontend, and not in the implementation?
Yes, because that's where you can emit the better error message more
easily.
$ python3 scripts/qapi-gen.py --prefix=@ x
scripts/qapi-gen.py: 'funny character '@' in argument of --prefix
is better than
$ python3 scripts/qapi-gen.py --prefix=@ x
scripts/qapi-gen.py: funny character '@' in prefix '@'
In generate(), the knowledge where the offending prefix value comes
from
is no longer available.
To emit this error message, you'd have to raise a sufficiently
distinct
error in generate, catch it in main(), then put the error message
together somehow. Bah.
Aside: there's a stray ' in the old error message.
The idea here was to create a function that could be used in a script
(for tests, debugging interfaces, other python packages) to do all of
the same things that the CLI tool did, just sans the actual CLI.
YAGNI.
It's useful for testing and debugging to be able to just call it
outside of the CLI, though. Maybe you won't use it, but I will.
For testing and debugging, treating "prefix is sane" as a precondition
is fine. I wouldn't even bother checking it. A check would catch
accidents, and these accidents seem vanishingly unlikely to me.
Evidence: we did without *any* prefix checking for *years*. I added it
in commit 1cf47a15f18 just for completeness.
I could always add the prefix check into a tiny function and give the
good error message in main(), and just assert in generate() if you
insist on the slightly more specific error message from the CLI script.
If you genuinely think a check is needed there, that's the way to go.