Eric Blake <ebl...@redhat.com> writes:
> On 02/02/2018 07:03 AM, Markus Armbruster wrote:
>> Whenever qapi-schema.json changes, we run six programs eleven times to
>> update eleven files. This is silly. Replace the six programs by a
>> single program that spits out all eleven files.
>
> Yay, about time!
>
> One program, but still invoked multiple times:
>
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> Makefile | 86
>> ++++++++++------------
>> scripts/qapi-gen.py | 41 +++++++++++
>> scripts/qapi/__init__.py | 0
>> scripts/{qapi-commands.py => qapi/commands.py} | 23 ++----
>> scripts/{qapi.py => qapi/common.py} | 0
>> 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 ++-------
>
> Maybe the commit message should mention:
>
> This requires moving some files around for proper use in python.
Yes, I should mention that I wrap the QAPI modules in a Python package.
>> tests/Makefile.include | 56 +++++++-------
>> tests/qapi-schema/test-qapi.py | 2 +-
>> 12 files changed, 140 insertions(+), 220 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} (100%)
>> rename scripts/{qapi2texi.py => qapi/doc.py} (92%)
>> mode change 100755 => 100644
>
> Unintinentional? We're inconsistent on which of our *.py files are
> executable in git. Does it matter whether a file that is designed for
> use as a module is marked executable (if so, perhaps 5/21 should be
> adding the x attribute on other files, rather than this patch removing
> it on the doc generator).
qapi2texi.py is no longer runnable as a standalone program after this
patch.
So are qapi-{commands,events,introspect,types,visit}.py, but they never
had the executable bit set.
>> +++ b/Makefile
>
>> +qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h \
>> +qga/qapi-generated/qga-qapi-visit.c qga/qapi-generated/qga-qapi-visit.h \
>> +qga/qapi-generated/qga-qmp-commands.h qga/qapi-generated/qga-qmp-marshal.c \
>> +qga/qapi-generated/qga-qapi.texi: \
>> +qga/qapi-generated/qapi-gen-timestamp ;
>> +qga/qapi-generated/qapi-gen-timestamp: $(SRC_PATH)/qga/qapi-schema.json
>> $(qapi-py)
>> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-gen.py \
>> + -o qga/qapi-generated -p "qga-" $<, \
>> + "GEN","$(@:%-timestamp=%)")
>> + @>$@
>
> once for qga,
That's the QAPI/QGA schema. The commit message talks about the QAPI/QMP
schema. I wish the two weren't named the same.
Modularization might make fusing them possible. Whether that's a good
idea I don't know.
>> +qapi-types.c qapi-types.h \
>> +qapi-visit.c qapi-visit.h \
>> +qmp-commands.h qmp-marshal.c \
>> +qapi-event.c qapi-event.h \
>> +qmp-introspect.h qmp-introspect.c \
>> +qapi.texi: \
>> +qapi-gen-timestamp ;
>> +qapi-gen-timestamp: $(qapi-modules) $(qapi-py)
>> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-gen.py \
>> + -o "." -b $<, \
>> + "GEN","$(@:%-timestamp=%)")
>> + @>$@
>
> and again for the main level. Still, a definite improvement!
Perhaps I can find a way to clarify the commit message.
>> +++ b/scripts/qapi-gen.py
>
>> +def main(argv):
>> + (input_file, output_dir, do_c, do_h, prefix, opts) = \
>> + parse_command_line('bu', ['builtins', 'unmask-non-abi-names'])
>> +
>> + opt_builtins = False
>> + opt_unmask = False
>> +
>> + for o, a in opts:
>> + if o in ('-b', '--builtins'):
>> + opt_builtins = True
>> + if o in ('-u', '--unmask-non-abi-names'):
>> + opt_unmask = True
>> +
>> + schema = QAPISchema(input_file)
>> +
>> + gen_types(schema, output_dir, prefix, opt_builtins)
>> + gen_visit(schema, output_dir, prefix, opt_builtins)
>> + gen_commands(schema, output_dir, prefix)
>> + gen_events(schema, output_dir, prefix)
>> + gen_introspect(schema, output_dir, prefix, opt_unmask)
>> + gen_doc(schema, output_dir, prefix)
>
> Rather simple, but definitely nicer all in one python file than as a
> series of make rules.
>
>> @@ -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'
>
> Some churn on the definition of blurb; worth tidying this in the
> introduction earlier in the series?
Doesn't seem worth a separate patch, but I can try to reshuffle things
to reduce churn.
>> rename to scripts/qapi/introspect.py
>> index 09e7d1f140..2153060f2c 100644
>> --- a/scripts/qapi-introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -10,7 +10,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 *
>>
>>
>> # Caveman's json.dumps() replacement (we're stuck at Python 2.4)
>> @@ -168,22 +168,8 @@ const char %(c_name)s[] = %(c_string)s;
>> self._gen_json(name, 'event', {'arg-type':
>> self._use_type(arg_type)})
>>
>>
>> -def main(argv):
>> - # Debugging aid: unmask QAPI schema's type names
>> - # We normally mask them, because they're not QMP wire ABI
>> - opt_unmask = False
>> -
>> - (input_file, output_dir, do_c, do_h, prefix, opts) = \
>> - parse_command_line('u', ['unmask-non-abi-names'])
>
> Hmm - we didn't have any docs about this hidden command line option; I
> see you still preserved it, but it may be worth mentioning in your
> pending doc updates for the series respin.
Maybe.
Providing --help would probably be more useful. We should convert
qapi-gen.py to argparse.
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -23,7 +23,16 @@ check-help:
>> ifneq ($(wildcard config-host.mak),)
>> export SRC_PATH
>>
>> -qapi-py = $(SRC_PATH)/scripts/qapi.py $(SRC_PATH)/scripts/ordereddict.py
>> +# TODO don't duplicate $(SRC_PATH)/Makefile's qapi-py here
>> +qapi-py = $(SRC_PATH)/scripts/qapi/commands.py \
>> +$(SRC_PATH)/scripts/qapi/events.py \
>> +$(SRC_PATH)/scripts/qapi/introspect.py \
>> +$(SRC_PATH)/scripts/qapi/types.py \
>> +$(SRC_PATH)/scripts/qapi/visit.py \
>> +$(SRC_PATH)/scripts/qapi/common.py \
>> +$(SRC_PATH)/scripts/qapi/doc.py \
>> +$(SRC_PATH)/scripts/ordereddict.py \
>> +$(SRC_PATH)/scripts/qapi-gen.py
>
> So, were you counting these in the 11 generated files mentioned in the
> commit message? :)
I'm not sure I understand what you mean here...
