Re: [Qemu-devel] [PATCH v2 08/29] qapi-gen: New common driver for code and doc generators

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/11/2018 03:35 AM, Markus Armbruster wrote:
>> Whenever qapi-schema.json changes, we run six programs eleven times to
>> update eleven files.  Similar for qga/qapi-schema.json.  This is
>> silly.  Replace the six programs by a single program that spits out
>> all eleven files.
>>
>> The programs become modules in new Python package qapi, along with the
>> helper library.  This requires moving them to scripts/qapi/.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Marc-André Lureau <address@hidden>
>> ---
>>   .gitignore                                         |  2 +
>>   Makefile                                           | 86 +++++++++----------
>>   docs/devel/qapi-code-gen.txt                       | 97 
>> ++++++++++------------
>>   monitor.c                                          |  2 +-
>>   qapi-schema.json                                   |  2 +-
>>   scripts/qapi-gen.py                                | 41 +++++++++
>>   scripts/qapi/__init__.py                           |  0
>>   scripts/{qapi-commands.py => qapi/commands.py}     | 23 ++---
>>   scripts/{qapi.py => qapi/common.py}                | 18 +---
>>   scripts/{qapi2texi.py => qapi/doc.py}              | 29 ++-----
>>   scripts/{qapi-event.py => qapi/events.py}          | 23 ++---
>>   scripts/{qapi-introspect.py => qapi/introspect.py} | 32 ++-----
>>   scripts/{qapi-types.py => qapi/types.py}           | 34 ++------
>>   scripts/{qapi-visit.py => qapi/visit.py}           | 34 ++------
>>   tests/Makefile.include                             | 56 ++++++-------
>>   tests/qapi-schema/test-qapi.py                     |  4 +-
>>   16 files changed, 193 insertions(+), 290 deletions(-)
>>   create mode 100755 scripts/qapi-gen.py
>>   create mode 100644 scripts/qapi/__init__.py
>>   rename scripts/{qapi-commands.py => qapi/commands.py} (94%)
>>   rename scripts/{qapi.py => qapi/common.py} (99%)
>>   rename scripts/{qapi2texi.py => qapi/doc.py} (92%)
>>   mode change 100755 => 100644
>
> Still forgot mention that the mode bit change was intentional, but not
> worth a respin for just that.
>
>>   rename scripts/{qapi-event.py => qapi/events.py} (92%)
>>   rename scripts/{qapi-introspect.py => qapi/introspect.py} (90%)
>>   rename scripts/{qapi-types.py => qapi/types.py} (90%)
>>   rename scripts/{qapi-visit.py => qapi/visit.py} (92%)
>
> Reviewed-by: Eric Blake <address@hidden>
>
>> +++ b/docs/devel/qapi-code-gen.txt
>
>> -    $ python scripts/qapi-event.py --output-dir="qapi-generated"
>> -    --prefix="example-" example-schema.json
>>       $ cat qapi-generated/example-qapi-event.h
>>   [Uninteresting stuff omitted...]
>>   @@ -1302,23 +1296,22 @@ Example:
>>       }
>>         const char *const example_QAPIEvent_lookup[] = {
>> -        [EXAMPLE_QAPI_EVENT_MY_EVENT] = "MY_EVENT",
>> +
>> +[EXAMPLE_QAPI_EVENT_MY_EVENT] = "MY_EVENT",
>>           [EXAMPLE_QAPI_EVENT__MAX] = NULL,
>>       };
>
> Looks like our generated code indentation has slightly regressed from
> what we would write by hand, but it's still okay for generated code
> (and the commit message in 5/29 did call that out)

I'd prefer to have this tidied up.

>> +++ b/scripts/qapi-gen.py
>
>> +++ b/scripts/qapi/commands.py
>> @@ -13,7 +13,7 @@ This work is licensed under the terms of the GNU GPL, 
>> version 2.
>>   See the COPYING file in the top-level directory.
>>   """
>>   -from qapi import *
>> +from qapi.common import *
>>       def gen_command_decl(name, arg_type, boxed, ret_type):
>> @@ -255,13 +255,8 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>>           self._regy += gen_register_command(name, success_response)
>>     -def main(argv):
>> -    (input_file, output_dir, do_c, do_h, prefix, opts) = 
>> parse_command_line()
>> -
>> -    blurb = '''
>> - * Schema-defined QAPI/QMP commands
>> -'''
>> -
>> +def gen_commands(schema, output_dir, prefix):
>> +    blurb = ' * Schema-defined QAPI/QMP commands'
>
> We discussed whether to make the assignment to blurb be a one-liner in
> an earlier patch, but I'm also fine with the churn you have over the
> course of the series.

I ran out of time, and decided to leave this one as is.