Re: [Qemu-devel] [PATCH RFC 06/21] qapi-gen: New common driver for code and doc generators

[Eric Blake]

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 <address@hidden>
> ---
>  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