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.
>>> 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.
[...]
