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

[Markus Armbruster <armbru@redhat.com>]

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.