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

[John Snow]

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.

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.

--js