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

[John Snow <jsnow@redhat.com>]

On 10/8/20 2:51 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 10/7/20 4:07 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
>>>>      
>>>> -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)
>>>> +
>>>> +    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.
>>> What's a "shell script entrypoint"?
>>> Python docs talk about "when [...] run as a script":
>>> https://docs.python.org/3/library/__main__.html
>>> Similar:
>>> https://docs.python.org/3/tutorial/modules.html#executing-modules-as-scripts
>>>
>>
>> "entrypoint" is Python garble for a function that can be registered as
>> a callable from the command line.
>>
>> So in a theoretical setup.py, you'd do something like:
>>
>> 'entry_points': {
>>    'console_scripts': [
>>      'qapi-gen = qapi.main:main',
>>    ]
>> }
>>
>> so when I say "shell script entrypoint", I am referring to a shell
>> script (I mean: it has a shebang and can be executed by an interactive
>> shell process) that calls the entrypoint.
> 
> It can be executed by any process.  See execve(2):
> 
>         pathname must be either a binary executable, or a script starting  with
>         a line of the form:
> 
>             #!interpreter [optional-arg]
> 
>         For details of the latter case, see "Interpreter scripts" below.
> 
> "Entry point" makes sense in Python context, "script entry point" also
> makes sense (since every Python program is a script, script is
> redundant, but not wrong).  "Shell script entry point" is misleading.
> 
>> Once (if) QAPI migrates to ./python/qemu/qapi, it will be possible to
>> just generate that script.
>>
>> (i.e. doing `pip install qemu.qapi` will install a 'qapi-gen' CLI
>> script for you. this is how packages like sphinx create the
>> 'sphinx-build' script, etc.)
>>
>>>> +    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")
>>>>        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
>>>>      
>>>>    if __name__ == '__main__':
>>>> -    main(sys.argv)
>>>> +    sys.exit(main())
>>> What does sys.exit() really buy us here?  I'm asking because both
>>> spots
>>> in the Python docs I referenced above do without.
>>>
>>
>> It just pushes the sys.exit out of the main function so it can be
>> invoked by other machinery. (And takes the return code from main and
>> turns it into the return code for the process.)
>>
>> I don't think it winds up mattering for simple "console_script" entry
>> points, but you don't want the called function to exit and deny the
>> caller the chance to do their own tidying post-call.
>>
>> You've already offered a "YAGNI", but it's just the convention I tend
>> to stick to for how to structure entry points.
> 
> I'm not questioning the conventional if __name__ == '__main__' menuett.
> I wonder why *we* need sys.exit() where the examples in the Python docs
> don't.
> 

I assume they don't care about setting that return code manually. By 
default it's going to be 0 on normal exit, and non-zero if it raises or 
aborts.

We catch the error to pretty-print it to console instead, and want to 
avoid the stack trace so we return a status code instead.

I assume that's the only difference, really.

--js