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

Commit Message

John Snow Oct. 5, 2020, 7:51 p.m. UTC

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 <[e-mail]>
Reviewed-by: Eduardo Habkost <[e-mail]>
Reviewed-by: Cleber Rosa <[e-mail]>
Tested-by: Cleber Rosa <[e-mail]>
---
 scripts/qapi-gen.py | 85 +++++++++++++++++++++++++++++++++------------
 1 file changed, 62 insertions(+), 23 deletions(-)

Comments

Markus Armbruster Oct. 6, 2020, 11:51 a.m. UTC | #1

John Snow <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Eduardo Habkost <[e-mail]>
> Reviewed-by: Cleber Rosa <[e-mail]>
> Tested-by: Cleber Rosa <[e-mail]>
> ---
>  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).

Is it really a good idea to do this in generate?  It's not about
generating code, it's about validating a CLI option.

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

2. They split the definition of the CLI: most of it is here, except for
defaults, which are defined elsewhere.

3. The defaults will not change, and nothing else uses the constants.

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

>  
>  
>  if __name__ == '__main__':
> -    main(sys.argv)
> +    sys.exit(main())

"Python was designed to be easy to understand and fun to use."
Ha ha ha.

John Snow Oct. 6, 2020, 3:59 p.m. UTC | #2

On 10/6/20 7:51 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> 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 <[e-mail]>
>> Reviewed-by: Eduardo Habkost <[e-mail]>
>> Reviewed-by: Cleber Rosa <[e-mail]>
>> Tested-by: Cleber Rosa <[e-mail]>
>> ---
>>   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?

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.

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?

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.

Wouldn't make sense to allow garbage to flow in from one interface but 
not the other; so the check is here.

>> +
>> +    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?

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

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

--js

John Snow Oct. 6, 2020, 4:46 p.m. UTC | #3

On 10/6/20 7:51 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> 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 <[e-mail]>
>> Reviewed-by: Eduardo Habkost <[e-mail]>
>> Reviewed-by: Cleber Rosa <[e-mail]>
>> Tested-by: Cleber Rosa <[e-mail]>
>> ---
>>   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).
> 
> Is it really a good idea to do this in generate?  It's not about
> generating code, it's about validating a CLI option.
> 
>> +
>> +    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.
> 
> 2. They split the definition of the CLI: most of it is here, except for
> defaults, which are defined elsewhere.
> 
> 3. The defaults will not change, and nothing else uses the constants.
> 
>>       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.
> 
>>   
>>   
>>   if __name__ == '__main__':
>> -    main(sys.argv)
>> +    sys.exit(main())
> 
> "Python was designed to be easy to understand and fun to use."
> Ha ha ha.
> 
Here's a Fixup that requires more (but obvious) Fixups to later patches 
in this series. (The next patch needs the same changes re-applied to the 
now-moved file.)

A note: I add a FIXME, but when I respin my error series, it will take 
care of this piece. This is just the quickest, dumbest way to victory in 
the interim. It is not worth engineering a better solution, because 
there is one waiting in part 4 already.

(Future knowledge: I add a QAPIError that does not define its arguments 
that inherits Exception and is used as the basis for all error classes 
in QAPI; and the contextual error that requires "info" is changed to 
inherit from QAPIError. This is useful as a "package-level" error, one 
in which that *all* errors in this package inherit from. We already 
achieve that, but we needed a more general form to serve as the root of 
that inheritance tree.)


diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
index 117b396a595..c62c4e93323 100644
--- a/scripts/qapi-gen.py
+++ b/scripts/qapi-gen.py
@@ -22,10 +22,6 @@
  from qapi.visit import gen_visit


-DEFAULT_OUTPUT_DIR = ''
-DEFAULT_PREFIX = ''
-
-
  def generate(schema_file: str,
               output_dir: str,
               prefix: str,
@@ -68,10 +64,10 @@ def main() -> int:
      parser.add_argument('-b', '--builtins', action='store_true',
                          help="generate code for built-in types")
      parser.add_argument('-o', '--output-dir', action='store',
-                        default=DEFAULT_OUTPUT_DIR,
+                        default='',
                          help="write output to directory OUTPUT_DIR")
      parser.add_argument('-p', '--prefix', action='store',
-                        default=DEFAULT_PREFIX,
+                        default='',
                          help="prefix for symbols")
      parser.add_argument('-u', '--unmask-non-abi-names', 
action='store_true',
                          dest='unmask',
diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index ae60d9e2fe8..f5a818ae35f 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -21,6 +21,9 @@ def __init__(self, info, col, msg):

      def __str__(self):
+        # FIXME: We need a general-purpose no-context error class.
+        if not self.info:
+            return self.msg
          loc = str(self.info)
          if self.col is not None:
              assert self.info.line is not None
              loc += ':%s' % self.col

Markus Armbruster Oct. 7, 2020, 7:54 a.m. UTC | #4

John Snow <[e-mail]> writes:

> On 10/6/20 7:51 AM, Markus Armbruster wrote:
>> John Snow <[e-mail]> 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 <[e-mail]>
>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>> Tested-by: Cleber Rosa <[e-mail]>
>>> ---
>>>   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.

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

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

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

Markus Armbruster Oct. 7, 2020, 8:07 a.m. UTC | #5

John Snow <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Eduardo Habkost <[e-mail]>
> Reviewed-by: Cleber Rosa <[e-mail]>
> Tested-by: Cleber Rosa <[e-mail]>
> ---
>  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

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

Markus Armbruster Oct. 7, 2020, 8:12 a.m. UTC | #6

I keep stumbling over things in later patches that turn out to go back
to this one.

John Snow <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Eduardo Habkost <[e-mail]>
> Reviewed-by: Cleber Rosa <[e-mail]>
> Tested-by: Cleber Rosa <[e-mail]>
> ---
>  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.

PEP 8: For flowing long blocks of text with fewer structural
restrictions (docstrings or comments), the line length should be limited
to 72 characters.

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

PEP 257: The docstring is a phrase ending in a period.  It prescribes
the function or method's effect as a command ("Do this", "Return that"),
not as a description; e.g. don't write "Returns the pathname ...".

Suggest

       Generate C code for the given schema into 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.
> +    """
[...]

John Snow Oct. 7, 2020, 2:36 p.m. UTC | #7

On 10/7/20 4:07 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> 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 <[e-mail]>
>> Reviewed-by: Eduardo Habkost <[e-mail]>
>> Reviewed-by: Cleber Rosa <[e-mail]>
>> Tested-by: Cleber Rosa <[e-mail]>
>> ---
>>   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

John Snow Oct. 7, 2020, 2:41 p.m. UTC | #8

On 10/7/20 4:12 AM, Markus Armbruster wrote:
> I keep stumbling over things in later patches that turn out to go back
> to this one.
> 
> John Snow <[e-mail]> 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 <[e-mail]>
>> Reviewed-by: Eduardo Habkost <[e-mail]>
>> Reviewed-by: Cleber Rosa <[e-mail]>
>> Tested-by: Cleber Rosa <[e-mail]>
>> ---
>>   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.
> 
> PEP 8: For flowing long blocks of text with fewer structural
> restrictions (docstrings or comments), the line length should be limited
> to 72 characters.
> 

Eugh. OK, but I don't have a good way to check or enforce this, 
admittedly. I have to change my emacs settings to understand this when I 
hit the reflow key. I don't know if the python mode has a context-aware 
reflow length.

("I don't disagree, but I'm not immediately sure right now how I will 
make sure I, or anyone else, complies with this. Low priority as a result?")

>> +"""
>>   
>>   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.
> 
> PEP 257: The docstring is a phrase ending in a period.  It prescribes
> the function or method's effect as a command ("Do this", "Return that"),
> not as a description; e.g. don't write "Returns the pathname ...".
> 
> Suggest
> 
>         Generate C code for the given schema into the target directory.
> 

OK. I don't mind trying to foster a consistent tone. I clearly didn't. I 
will add a note to my style guide todo.

I give you permission to change the voice in any of my docstrings, or to 
adjust the phrasing to be more technically accurate as you see fit. You 
are the primary maintainer of this code, of course, and you will know best.

It will be quicker to give you full permission to just change any of the 
docstrings as you see fit than it will be to play review-respin ping-pong.

>> +
>> +    :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.
>> +    """
> [...]
>

John Snow Oct. 7, 2020, 2:52 p.m. UTC | #9

On 10/7/20 3:54 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> writes:
> 
>> On 10/6/20 7:51 AM, Markus Armbruster wrote:
>>> John Snow <[e-mail]> 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 <[e-mail]>
>>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>>> Tested-by: Cleber Rosa <[e-mail]>
>>>> ---
>>>>    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?
>

Markus Armbruster Oct. 8, 2020, 5:56 a.m. UTC | #10

John Snow <[e-mail]> writes:

> On 10/7/20 3:54 AM, Markus Armbruster wrote:
>> John Snow <[e-mail]> writes:
>> 
>>> On 10/6/20 7:51 AM, Markus Armbruster wrote:
>>>> John Snow <[e-mail]> 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 <[e-mail]>
>>>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>>>> Tested-by: Cleber Rosa <[e-mail]>
>>>>> ---
>>>>>    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/.

You asked why the class has the init it has, and I answered :)

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

If you have a non-trivial error message format convention, you have a
use for a function formatting error messages.

If you have a separation between diagnose and report of errors, you you
have a use for a transport from diagnose to report.  In Python, that's
raise.

The existing error message in main() has neither.

The existing error class QAPIError caters for the existing users.

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

For testing and debugging, treating "prefix is sane" as a precondition
is fine.  I wouldn't even bother checking it.  A check would catch
accidents, and these accidents seem vanishingly unlikely to me.

Evidence: we did without *any* prefix checking for *years*.  I added it
in commit 1cf47a15f18 just for completeness.

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

If you genuinely think a check is needed there, that's the way to go.

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

Markus Armbruster Oct. 8, 2020, 6:51 a.m. UTC | #11

John Snow <[e-mail]> writes:

> On 10/7/20 4:07 AM, Markus Armbruster wrote:
>> John Snow <[e-mail]> 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 <[e-mail]>
>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>> Tested-by: Cleber Rosa <[e-mail]>
>>> ---
>>>   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.

Markus Armbruster Oct. 8, 2020, 7:15 a.m. UTC | #12

John Snow <[e-mail]> writes:

> On 10/7/20 4:12 AM, Markus Armbruster wrote:
>> I keep stumbling over things in later patches that turn out to go back
>> to this one.
>> John Snow <[e-mail]> 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 <[e-mail]>
>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>> Tested-by: Cleber Rosa <[e-mail]>
>>> ---
>>>   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.
>> PEP 8: For flowing long blocks of text with fewer structural
>> restrictions (docstrings or comments), the line length should be limited
>> to 72 characters.
>> 
>
> Eugh. OK, but I don't have a good way to check or enforce this,
> admittedly. I have to change my emacs settings to understand this when
> I hit the reflow key. I don't know if the python mode has a
> context-aware reflow length.
>
> ("I don't disagree, but I'm not immediately sure right now how I will
> make sure I, or anyone else, complies with this. Low priority as a
> result?")

Emacs Python mode is close enough by default: fill-paragraph (bound to
M-q) uses variable fill-column, which defaults to 70.  If you want the
extra two columns PEP 8 grants you, I can show you how to bump it to 72
just for Python mode.

You can use fill-paragraph for code, too.  I don't myself, because I
disagree with its line breaking decisions too often (and so does PEP 8).
A better Python mode would break code lines more neatly, and with the
width defaulting to 79.

>>> +"""
>>>     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.
>> PEP 257: The docstring is a phrase ending in a period.  It
>> prescribes
>> the function or method's effect as a command ("Do this", "Return that"),
>> not as a description; e.g. don't write "Returns the pathname ...".
>> Suggest
>>         Generate C code for the given schema into the target
>> directory.
>> 
>
> OK. I don't mind trying to foster a consistent tone. I clearly
> didn't. I will add a note to my style guide todo.
>
> I give you permission to change the voice in any of my docstrings, or
> to adjust the phrasing to be more technically accurate as you see
> fit. You are the primary maintainer of this code, of course, and you
> will know best.
>
> It will be quicker to give you full permission to just change any of
> the docstrings as you see fit than it will be to play review-respin
> ping-pong.

Me rewriting your commits without your consent is putting words in your
mouth, which I don't want to do.

We can still reduce ping-pong: whenever I can, I don't just say "this
needs improvement", I propose improvements.  If you disagree, we talk.
Else, if you have to respin, you make a reasonable effort to take them.
Else, the remaining improvements are trivial (because no respin), and
I'll make them in my tree.

>>> +
>>> +    :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.
>>> +    """
>> [...]
>>

John Snow Oct. 8, 2020, 4:37 p.m. UTC | #13

On 10/8/20 2:51 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> writes:
> 
>> On 10/7/20 4:07 AM, Markus Armbruster wrote:
>>> John Snow <[e-mail]> 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 <[e-mail]>
>>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>>> Tested-by: Cleber Rosa <[e-mail]>
>>>> ---
>>>>    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

John Snow Oct. 8, 2020, 4:50 p.m. UTC | #14

On 10/8/20 2:51 AM, Markus Armbruster wrote:
> 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.

You know, I don't think I was actually explicitly aware that the #! 
shebang was not something the shell actually processed itself. Always 
learning new things.

(No, I don't think I have ever execve'd something that wasn't a binary.)

"entry point" is a little vague, an entry point for what? by whom? I was 
trying to call attention to the idea specifically that main() was 
intended as python's "console script entry point", but used the word 
"shell" instead.

"console script entrypoint" is also a lot of jargon. What I really want 
to communicate is: "When you run `qapi-gen` on your command-line, this 
is the function that runs!"

So I guess something like:

"qapi-gen executable entry point." will suffice. Please further adjust 
to your liking when staging.

--js

John Snow Oct. 8, 2020, 5:14 p.m. UTC | #15

On 10/8/20 3:15 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> writes:
> 
>> On 10/7/20 4:12 AM, Markus Armbruster wrote:
>>> I keep stumbling over things in later patches that turn out to go back
>>> to this one.
>>> John Snow <[e-mail]> 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 <[e-mail]>
>>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>>> Tested-by: Cleber Rosa <[e-mail]>
>>>> ---
>>>>    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.
>>> PEP 8: For flowing long blocks of text with fewer structural
>>> restrictions (docstrings or comments), the line length should be limited
>>> to 72 characters.
>>>
>>
>> Eugh. OK, but I don't have a good way to check or enforce this,
>> admittedly. I have to change my emacs settings to understand this when
>> I hit the reflow key. I don't know if the python mode has a
>> context-aware reflow length.
>>
>> ("I don't disagree, but I'm not immediately sure right now how I will
>> make sure I, or anyone else, complies with this. Low priority as a
>> result?")
> 
> Emacs Python mode is close enough by default: fill-paragraph (bound to
> M-q) uses variable fill-column, which defaults to 70.  If you want the
> extra two columns PEP 8 grants you, I can show you how to bump it to 72
> just for Python mode.
> 
> You can use fill-paragraph for code, too.  I don't myself, because I
> disagree with its line breaking decisions too often (and so does PEP 8).
> A better Python mode would break code lines more neatly, and with the
> width defaulting to 79.
> 

Yeah, how do I set the reflow to 72 for specific modes?

I tend to do a lot of refactoring and "prototyping" in Pycharm, but when 
it comes to bread and butter edits I still prefer emacs. I kinda bounce 
between 'em a lot. Having emacs DTRT is still useful to me.

>>>> +"""
>>>>      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.
>>> PEP 257: The docstring is a phrase ending in a period.  It
>>> prescribes
>>> the function or method's effect as a command ("Do this", "Return that"),
>>> not as a description; e.g. don't write "Returns the pathname ...".
>>> Suggest
>>>          Generate C code for the given schema into the target
>>> directory.
>>>
>>
>> OK. I don't mind trying to foster a consistent tone. I clearly
>> didn't. I will add a note to my style guide todo.
>>
>> I give you permission to change the voice in any of my docstrings, or
>> to adjust the phrasing to be more technically accurate as you see
>> fit. You are the primary maintainer of this code, of course, and you
>> will know best.
>>
>> It will be quicker to give you full permission to just change any of
>> the docstrings as you see fit than it will be to play review-respin
>> ping-pong.
> 
> Me rewriting your commits without your consent is putting words in your
> mouth, which I don't want to do.
> 
> We can still reduce ping-pong: whenever I can, I don't just say "this
> needs improvement", I propose improvements.  If you disagree, we talk.
> Else, if you have to respin, you make a reasonable effort to take them.
> Else, the remaining improvements are trivial (because no respin), and
> I'll make them in my tree.
> 

I appreciate the consideration. They're not just "my" words, though, 
they are "our" words!

I do give you permission to touch up things like punctuation, voice, 
etc. If I dislike the changes I can always yelp at you post-hoc when you 
post the PR email.

So I'll reiterate that I am definitely happy with such changes -- the 
precision of the technical writing here is not my strong suit as I do 
not know QAPI as intimately as you, and it's not the focus of this 
series. I don't think of it as a personal offense that someone would 
copy-edit these things.

If you're not comfortable doing it, that's OK too, but I'm definitely 
okay with things like comment and docstring edits in particular. It's 
just something that feels hard to predict.

(Anyway, I made this change. There are likely other voice and line 
length changes needed, but I think I will try to address those 
systematically when I lift missing-docstring for pylint. Probably I will 
craft a series that creates a style guide, enables sphinx, lifts 
missing-docstring, and performs this same style of cleanup, going 
module-by-module.)

>>>> +
>>>> +    :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.
>>>> +    """
>>> [...]
>>>

John Snow Oct. 8, 2020, 5:33 p.m. UTC | #16

On 10/8/20 1:56 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> writes:
> 
>> On 10/7/20 3:54 AM, Markus Armbruster wrote:
>>> John Snow <[e-mail]> writes:
>>>
>>>> On 10/6/20 7:51 AM, Markus Armbruster wrote:
>>>>> John Snow <[e-mail]> 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 <[e-mail]>
>>>>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>>>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>>>>> Tested-by: Cleber Rosa <[e-mail]>
>>>>>> ---
>>>>>>     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/.
> 
> You asked why the class has the init it has, and I answered :)
> 
>>>> 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.
> 
> If you have a non-trivial error message format convention, you have a
> use for a function formatting error messages.
> 
> If you have a separation between diagnose and report of errors, you you
> have a use for a transport from diagnose to report.  In Python, that's
> raise.
> 
> The existing error message in main() has neither.
> 
> The existing error class QAPIError caters for the existing users.
> 
>>>> 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.
> 
> For testing and debugging, treating "prefix is sane" as a precondition
> is fine.  I wouldn't even bother checking it.  A check would catch
> accidents, and these accidents seem vanishingly unlikely to me.
> 
> Evidence: we did without *any* prefix checking for *years*.  I added it
> in commit 1cf47a15f18 just for completeness.
> 
>> 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.
> 
> If you genuinely think a check is needed there, that's the way to go.
> 

A goal of this patch in particular is the clean separation of the CLI 
frontend from the implementation logic behind it, such that you have:

(1) The CLI parsing, and
(2) A function implementing a task the package is capable of performing

The CLI parsing method converts sys.argv arguments into function 
parameter data that can be acted upon by another function in the package.

If checks are exclusive to the process of converting flags to argument 
data, they should stay in the CLI parsing function.

If checks prevent you from passing bad data lower into the program, I 
prefer pushing that check lower into the stack instead, such that the 
module is equivalently useful without the CLI frontend.

This conceptualization might be different from how you perceive QAPI. In 
my case, I consider the public functions and modules themselves to be 
interface that should behave reasonably and with some defensiveness. You 
might only count the CLI script itself in that category.

In this case, the prefix is passed to many different generators, so it 
seems good to check it before we hand it off to all those different places.

You don't like the loss of precision that causes for the CLI frontend 
error, which is reasonable.

I consider the function interface and the CLI frontend "equivalent", and 
so I found it odd to reject bad data in one, but not the other. IOW, I 
do not consider the CLI frontend the only user of generate().

I think it's minor in this case, sure. I am fine with saying "A valid 
prefix is a precondition", but then I want to document and enforce that 
precondition -- so I did wind up using an assert in my pending respin.

--js

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

Markus Armbruster Oct. 9, 2020, 7:12 a.m. UTC | #17

John Snow <[e-mail]> writes:

> On 10/8/20 2:51 AM, Markus Armbruster wrote:
>> 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.
>
> You know, I don't think I was actually explicitly aware that the #!
> shebang was not something the shell actually processed itself. Always 
> learning new things.
>
> (No, I don't think I have ever execve'd something that wasn't a binary.)

I'm sure you've done it countless times, just not knowingly :)

> "entry point" is a little vague, an entry point for what? by whom? I
> was trying to call attention to the idea specifically that main() was 
> intended as python's "console script entry point", but used the word
> "shell" instead.
>
> "console script entrypoint" is also a lot of jargon. What I really

It is.

If I didn't find "console script" so thoroughly misguided, I'd advie you
to stick to it just because it's what we found in Python's docs.  It's
misguided, because this entry point is the one and only entry point for
*any* kind of Python executable program, be it console, GUI, or
whatever.

> want to communicate is: "When you run `qapi-gen` on your command-line,
> this is the function that runs!"
>
> So I guess something like:
>
> "qapi-gen executable entry point." will suffice. Please further adjust
> to your liking when staging.

Works for me.

Markus Armbruster Oct. 9, 2020, 7:19 a.m. UTC | #18

John Snow <[e-mail]> writes:

> On 10/8/20 3:15 AM, Markus Armbruster wrote:
>> John Snow <[e-mail]> writes:
>> 
>>> On 10/7/20 4:12 AM, Markus Armbruster wrote:
>>>> I keep stumbling over things in later patches that turn out to go back
>>>> to this one.
>>>> John Snow <[e-mail]> 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 <[e-mail]>
>>>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>>>> Tested-by: Cleber Rosa <[e-mail]>
>>>>> ---
>>>>>    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.
>>>> PEP 8: For flowing long blocks of text with fewer structural
>>>> restrictions (docstrings or comments), the line length should be limited
>>>> to 72 characters.
>>>>
>>>
>>> Eugh. OK, but I don't have a good way to check or enforce this,
>>> admittedly. I have to change my emacs settings to understand this when
>>> I hit the reflow key. I don't know if the python mode has a
>>> context-aware reflow length.
>>>
>>> ("I don't disagree, but I'm not immediately sure right now how I will
>>> make sure I, or anyone else, complies with this. Low priority as a
>>> result?")
>> 
>> Emacs Python mode is close enough by default: fill-paragraph (bound to
>> M-q) uses variable fill-column, which defaults to 70.  If you want the
>> extra two columns PEP 8 grants you, I can show you how to bump it to 72
>> just for Python mode.
>> 
>> You can use fill-paragraph for code, too.  I don't myself, because I
>> disagree with its line breaking decisions too often (and so does PEP 8).
>> A better Python mode would break code lines more neatly, and with the
>> width defaulting to 79.
>
> Yeah, how do I set the reflow to 72 for specific modes?
>
> I tend to do a lot of refactoring and "prototyping" in Pycharm, but
> when it comes to bread and butter edits I still prefer emacs. I kinda
> bounce between 'em a lot. Having emacs DTRT is still useful to me.

In your .emacs:

    (add-hook 'python-mode-hook
              (lambda () (setq fill-column 72)))


Happy to explain this in detail if you're curious.

[...]

Patch

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.
+    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())