On 10/8/20 1:56 AM, Markus Armbruster wrote:
John Snow <js...@redhat.com> writes:
On 10/7/20 3:54 AM, Markus Armbruster wrote:
John Snow <js...@redhat.com> writes:
On 10/6/20 7:51 AM, Markus Armbruster wrote:
John Snow <js...@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 <js...@redhat.com>
Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
Reviewed-by: Cleber Rosa <cr...@redhat.com>
Tested-by: Cleber Rosa <cr...@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.
A goal of this patch in particular is the clean separation of the CLI
frontend from the implementation logic behind it, such that you have:
(1) The CLI parsing, and
(2) A function implementing a task the package is capable of performing
The CLI parsing method converts sys.argv arguments into function
parameter data that can be acted upon by another function in the package.
If checks are exclusive to the process of converting flags to argument
data, they should stay in the CLI parsing function.
If checks prevent you from passing bad data lower into the program, I
prefer pushing that check lower into the stack instead, such that the
module is equivalently useful without the CLI frontend.
This conceptualization might be different from how you perceive QAPI. In
my case, I consider the public functions and modules themselves to be
interface that should behave reasonably and with some defensiveness. You
might only count the CLI script itself in that category.
In this case, the prefix is passed to many different generators, so it
seems good to check it before we hand it off to all those different places.
You don't like the loss of precision that causes for the CLI frontend
error, which is reasonable.
I consider the function interface and the CLI frontend "equivalent", and
so I found it odd to reject bad data in one, but not the other. IOW, I
do not consider the CLI frontend the only user of generate().
I think it's minor in this case, sure. I am fine with saying "A valid
prefix is a precondition", but then I want to document and enforce that
precondition -- so I did wind up using an assert in my pending respin.
--js
Wouldn't make sense to allow garbage to flow in from one interface but
not the other; so the check is here.
"@prefix is sane" is a precondition of generate().
When there's a real risk of preconditions getting violated, or
readers
getting confused about preconditions, check them with assert.
[...]
