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.
> 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).
> +++ 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,
> +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!
> +++ 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?
> 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.
> --- 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? :)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
