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

[Eric Blake]

On 02/03/2018 03:03 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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:
>>

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

Okay, so dropping the bit consistently makes sense; still, a mention in
the commit message wouldn't hurt.

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

7 files here,...

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

11 files here,...

> 
> Perhaps I can find a way to clarify the commit message.
> 

[1]


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

Yeah, my question was more about the conversion between multiline
'''...''' to single-line '...'; why not just use the single-line
construct earlier in the series when introducing the blurb variable.
You are right that creating blurb didn't need a separate patch, just
less churn over the series.

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

[1] and 9 more files.  So, the commit message only mentioned the 11 QMP
files, rather than all 27 (if I counted right) QAPI generated files.  My
comments were trying to point out that you simplified more than just the
QMP code generation into fewer script invocations, and the counts looked
off since out of the three spots converted, only one of the spots had 11
files.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature