Re: [Qemu-devel] [PATCH for-2.9 02/47] qapi: Make doc comments optional where we don't need them

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 03/13/2017 01:18 AM, Markus Armbruster wrote:
>> Since we added the documentation generator in commit 3313b61, doc
>> comments are mandatory.  That's a very good idea for a schema that
>> needs to be documented, but has proven to be annoying for testing.
>
> As I've found out while rebasing my JSON output visitor as well as
> patches to allow anonymous bases to flat unions.  Thanks, this will help me!
>
>> 
>> Make doc comments optional again, but add a new directive
>> 
>>     { 'pragma': { 'doc-required': true } }
>> 
>> to let a QAPI schema require them.
>
> I like it.  It's extensible to other pragmas, as well; and reading
> ahead, it looks like you did a good job of flagging unknown pragmas.
>
>> 
>> Require documentation in the schemas we actually want documented:
>> qapi-schema.json and qga/qapi-schema.json.
>> 
>> We could probably make qapi2texi.py cope with incomplete
>> documentation, but for now, simply make it refuse to run unless the
>> schema has 'doc-required': true.
>
> I'd rather fail early on our non-documented stuff then risk broken
> corner cases; so I think you made the right decision.

I figure making qapi2texi.py robust wouldn't be hard, but decided not to
try for 2.9.

>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/qapi-code-gen.txt             | 40 +++++++++++++++++++++++++-------------
>
>> @@ -277,6 +280,11 @@ class QAPISchemaParser(object):
>>                                         "Value of 'include' must be a string")
>>                  self._include(include, info, os.path.dirname(abs_fname),
>>                                previously_included)
>> +            elif "pragma" in expr:
>> +                if len(expr) != 1:
>> +                    raise QAPISemError(info, "Invalid 'pragma' directive")
>
> You may also want to check that you have an actual dictionary; otherwise...
>
>> +                for name, value in expr['pragma'].iteritems():
>
> calling .iteritems() can lead to some funky python messages. For
> example, tweaking qapi-schema.json to use
>
> { 'pragma': [ { 'doc-required': true } ] }
>
> =>
>
> $ make
>   GEN     qmp-commands.h
> Traceback (most recent call last):
>   File "/home/eblake/qemu/scripts/qapi-commands.py", line 317, in <module>
>     schema = QAPISchema(input_file)
>   File "/home/eblake/qemu/scripts/qapi.py", line 1496, in __init__
>     parser = QAPISchemaParser(open(fname, "r"))
>   File "/home/eblake/qemu/scripts/qapi.py", line 286, in __init__
>     for name, value in expr['pragma'].iteritems():
> AttributeError: 'list' object has no attribute 'iteritems'
> Makefile:431: recipe for target 'qmp-commands.h' failed

You're right.  Need to decide whether to fix it in a respin or on top.

> Not the end of the world, but we've done a nice job elsewhere of
> avoiding cryptic python traces.
>
>> +        global doc_required
>> +        if name == 'doc-required':
>> +            if not isinstance(value, bool):
>> +                raise QAPISemError(info,
>> +                                   "Pragma 'doc-required' must be boolean")
>> +            doc_required = value
>
> No testsuite coverage of this message?

Not yet, i.e. you're right, test coverage is advisable even for exotic
stuff that's expected not to change like pragma.

> If you decide to not bother, or to defer error message(s) cleanup to
> followups (especially since we are running short on time for fixing the
> actual doc regression from going into 2.9), then using this patch as-is
> can have:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

> Of course, if you spin a v2 that actually addresses my concerns, it
> probably will be more involved than something that can trivially keep my
> R-b (in part because you'll probably also want a testsuite addition to
> cover any new error message...).

Possible :)