Re: [PATCH v5 03/36] qapi-gen: Separate arg-parsing from generation

From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>,
	Cleber Rosa <crosa@redhat.com>
Subject: Re: [PATCH v5 03/36] qapi-gen: Separate arg-parsing from generation
Date: Thu, 08 Oct 2020 07:56:44 +0200	[thread overview]
Message-ID: <87h7r545wj.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <65c2a120-e29e-b572-680a-ee6d141ff9c8@redhat.com> (John Snow's message of "Wed, 7 Oct 2020 10:52:54 -0400")

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.

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