Re: [PATCH v2 02/38] qapi-gen: Separate arg-parsing from generation

[Markus Armbruster]

Cleber Rosa <crosa@redhat.com> writes:

> On Tue, Sep 22, 2020 at 05:00:25PM -0400, John Snow wrote:
>> 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>
>> ---
>>  scripts/qapi-gen.py | 87 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 63 insertions(+), 24 deletions(-)
>> 
>> diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
>> index 4b03f7d53b..59becba3e1 100644
>> --- a/scripts/qapi-gen.py
>> +++ b/scripts/qapi-gen.py
>> @@ -1,9 +1,13 @@
>>  #!/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
>> @@ -11,21 +15,65 @@
>>  
>>  from qapi.commands import gen_commands
>>  from qapi.doc import gen_doc
>> +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 = ''
>
> I did not understand the purpose of these.  If they're used only as
> the default value for the command line option parsing, I'd suggest
> dropping them.
>
>> +
>> +
>> +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 and match.end() != len(prefix):
>
> Nice catch with the extra check here.  Maybe worth mentioning and/or
> splitting the change?

Please do not sneak additional checking into patches advertized as pure
refactoring.  It makes me look for more sneakery with a microscope.

This re.match() cannot possibly fail.  Three cases:

* First character is funny

  The regexp matches the empty string.  There's a reason the regexp ends
  with '?'.

* Non-first character is funny

  The regexp matches the non-funny prefix.

* No character is funny

  The regexp matches the complete string.

Checking impossible conditions as if they were possible is confusing.
Please drop the additional check.

We can talk about checking this impossible condition with

        assert(match)

if you believe it makes the code easier to understand (it does not
improve its behavior).

>> +        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)
>> +    gen_doc(schema, output_dir, prefix)
>> +
>> +
>> +def main() -> int:
>
> One extra Pythonic touch would be to use a bool here, and then:
>
>   sys.exit(0 if main() else 1)
>
> But that's probably overkill.

Yuck :)

>> +    """
>> +    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")
>>      parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
>>                          dest='unmask',
>> @@ -33,26 +81,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)
>> -
>
> Glad to see that this "quitter" is gone in favor of one and only
> sys.exit().

I'm totally fine with having multiple sys.exit() in main().  They become
icky only when buried too deeply.

I'm fine with John's patch regardless.

[...]