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

[John Snow <jsnow@redhat.com>]

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/.

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

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

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.

>> 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.
> 
>>>> +
>>>> +    schema = QAPISchema(schema_file)
>>>> +    gen_types(schema, output_dir, prefix, builtins)
>>>> +    gen_visit(schema, output_dir, prefix, builtins)
>>>> +    gen_commands(schema, output_dir, prefix)
>>>> +    gen_events(schema, output_dir, prefix)
>>>> +    gen_introspect(schema, output_dir, prefix, unmask)
>>>> +
>>>> +
>>>> +def main() -> int:
>>>> +    """
>>>> +    gapi-gen shell script entrypoint.
>>>> +    Expects arguments via sys.argv, see --help for details.
>>>> +
>>>> +    :return: int, 0 on success, 1 on failure.
>>>> +    """
>>>>        parser = argparse.ArgumentParser(
>>>>            description='Generate code from a QAPI schema')
>>>>        parser.add_argument('-b', '--builtins', action='store_true',
>>>>                            help="generate code for built-in types")
>>>> -    parser.add_argument('-o', '--output-dir', action='store', default='',
>>>> +    parser.add_argument('-o', '--output-dir', action='store',
>>>> +                        default=DEFAULT_OUTPUT_DIR,
>>>>                            help="write output to directory OUTPUT_DIR")
>>>> -    parser.add_argument('-p', '--prefix', action='store', default='',
>>>> +    parser.add_argument('-p', '--prefix', action='store',
>>>> +                        default=DEFAULT_PREFIX,
>>>>                            help="prefix for symbols")
>>> I don't like the changes to default=, because:
>>> 1. They are only losely related to the patch's purpose.
>>>
>>
>> Subjective, but OK.
>>
>>> 2. They split the definition of the CLI: most of it is here, except for
>>> defaults, which are defined elsewhere.
>>>
>>
>> All of it is in main.py, though! If you were to, say, move generate()
>> elsewhere, it'd look pretty compact as just the CLI frontend, no?
> 
> Same statement is more compact than same screenful is more compact than
> same file :)
> 
>>> 3. The defaults will not change, and nothing else uses the constants.
>>>
>>
>> But, fine. Cleber had the same comment but I wasn't fully on-board,
>> but two folks saying the same thing ...
>>
>>>>        parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
>>>>                            dest='unmask',
>>>> @@ -32,25 +79,17 @@ def main(argv):
>>>>        parser.add_argument('schema', action='store')
>>>>        args = parser.parse_args()
>>>>    -    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?',
>>>> args.prefix)
>>>> -    if match.end() != len(args.prefix):
>>>> -        print("%s: 'funny character '%s' in argument of --prefix"
>>>> -              % (sys.argv[0], args.prefix[match.end()]),
>>>> -              file=sys.stderr)
>>>> -        sys.exit(1)
>>>> -
>>>>        try:
>>>> -        schema = QAPISchema(args.schema)
>>>> +        generate(args.schema,
>>>> +                 output_dir=args.output_dir,
>>>> +                 prefix=args.prefix,
>>>> +                 unmask=args.unmask,
>>>> +                 builtins=args.builtins)
>>>>        except QAPIError as err:
>>>> -        print(err, file=sys.stderr)
>>>> -        exit(1)
>>>> -
>>>> -    gen_types(schema, args.output_dir, args.prefix, args.builtins)
>>>> -    gen_visit(schema, args.output_dir, args.prefix, args.builtins)
>>>> -    gen_commands(schema, args.output_dir, args.prefix)
>>>> -    gen_events(schema, args.output_dir, args.prefix)
>>>> -    gen_introspect(schema, args.output_dir, args.prefix, args.unmask)
>>>> +        print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
>>>> +        return 1
>>>> +    return 0
>>> Subtle change: you move the gen_FOO() into the try ... except.
>>> Okay;
>>> they don't raise QAPIError, but perhaps worth a mention in the commit
>>> message.
>>>
>>
>> Forbidden future knowledge; I intend them to.
> 
> I don't mind the move.
> 
>>>>      
>>>>    if __name__ == '__main__':
>>>> -    main(sys.argv)
>>>> +    sys.exit(main())
>>> "Python was designed to be easy to understand and fun to use."
>>> Ha ha ha.
>>>
>>
>> I mean, I'm having fun, aren't you?
> 
> So many kinds of fun!  The fun I'm having with this patch hunk is
> mocking "easy and fun" Python for requiring such an elaborate menuett
> just to express "can run as program".
> 
> This emperor has no clothes, either.  And that's funny, isn't it?
>