Re: [Qemu-devel] [PATCH v5 13/17] qapi: add qapi2texi script

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Tue, Dec 6, 2016 at 2:50 PM Markus Armbruster <address@hidden> wrote:
>
>> I had to resort to diff to find your replies, and massage the text
>> manually to produce a readable reply myself.  Please quote the usual
>> way.
>>
>>
> I'd have to switch to something else than gmail (which bothers me for
> various reasons, let's not discuss the merits of various mail clients
> please ;) In general, I don't have problems, but this mail is rather big,
> sorry for the inconvenience..

This time, it was only a bit of mojibake and lots of unwanted line wraps
%-)

I'll try to cut unnecessary quoted material this time.

>> Markus Armbruster <address@hidden> writes:
>> >
>> > > Marc-André Lureau <address@hidden> writes:
>> > >
>> > >> As the name suggests, the qapi2texi script converts JSON QAPI
>> > >> description into a texi file suitable for different target
>> > >> formats (info/man/txt/pdf/html...).
[...]
>> > > Missing: a brief discussion of deficiencies.  These include:
>> > >
>> > > * The generated QMP documentation includes internal types
>> > >
>> > >   We use qapi-schema.json both for defining the external QMP interface
>> > >   and for defining internal types.  qmp-introspect.py carefully
>> > >   separates the two, to not expose internal types.  qapi2texi.py happily
>> > >   exposes everything.
>> > >
>> > >   Currently, about a fifth of the types in the generated docs are
>> > >   internal:
>> > >
>> > >       AcpiTableOptions
>> > >       BiosAtaTranslation
>> > >       BlockDeviceMapEntry
>> > >       COLOMessage
>> > >       COLOMode
>> > >       DummyForceArrays
>> > >       FailoverStatus
>> > >       FloppyDriveType
>> > >       ImageCheck
>> > >       LostTickPolicy
>> > >       MapEntry
>> > >       MigrationParameter
>> > >       NetClientDriver
>> > >       NetFilterDirection
>> > >       NetLegacy
>> > >       NetLegacyNicOptions
>> > >       NetLegacyOptions
>> > >       NetLegacyOptionsKind
>> > >       Netdev
4>> > >       NetdevBridgeOptions
>> > >       NetdevDumpOptions
>> > >       NetdevHubPortOptions
>> > >       NetdevL2TPv3Options
>> > >       NetdevNetmapOptions
>> > >       NetdevNoneOptions
>> > >       NetdevSocketOptions
>> > >       NetdevTapOptions
>> > >       NetdevUserOptions
>> > >       NetdevVdeOptions
>> > >       NetdevVhostUserOptions
>> > >       NumaNodeOptions
>> > >       NumaOptions
>> > >       NumaOptionsKind
>> > >       OnOffAuto
>> > >       OnOffSplit
>> > >       PreallocMode
>> > >       QCryptoBlockCreateOptions
>> > >       QCryptoBlockCreateOptionsLUKS
>> > >       QCryptoBlockFormat
>> > >       QCryptoBlockInfo
>> > >       QCryptoBlockInfoBase
>> > >       QCryptoBlockInfoQCow
>> > >       QCryptoBlockOpenOptions
>> > >       QCryptoBlockOptionsBase
>> > >       QCryptoBlockOptionsLUKS
>> > >       QCryptoBlockOptionsQCow
>> > >       QCryptoSecretFormat
>> > >       QCryptoTLSCredsEndpoint
>> > >       QapiErrorClass
>> > >       ReplayMode
>> > >       X86CPUFeatureWordInfo
>> > >       X86CPURegister32
>> > >
>> > >   Generating documentation for internal types might be useful, but
>> > >   letting them pollute QMP interface documentation isn't.  Needs fixing
>> > >   before we release.  Until then, needs a FIXME comment in qapi2texi.py.
>> > >
>> > > * Union support is lacking
>> > >
>> > >   The doc string language is adequate for documenting commands, events,
>> > >   and non-union types.  It doesn't really handle union variants. Hardly
>> > >   surprising, as you fitted the language do existing practice, and
>> > >   existing (mal-)practice is neglecting to document union variant
>> > >   members.
>> > >
>> > > * Documentation is lacking
>> > >
>> > >   See review of qapi-code-gen.txt below.
>> > >
>> > > * Doc comment error message positions are imprecise
>> > >
>> > >   They always point to the beginning of the comment.
>> > >
>> > > * Probably more
>> > >
>> > >   We should update this with noteworthy findings during review.  I
>> > >   tried, but I suspect I missed some.
>> >
>> > ok
>> >
>> > >> Signed-off-by: Marc-André Lureau <address@hidden>
[...]
>> > >> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> > >> index 4d1b0e4..1b456b4 100644
>> > >> --- a/scripts/qapi.py
>> > >> +++ b/scripts/qapi.py
[...]
>> > >> +    def append(self, line):
>> > >> +        """Adds a # comment line, to be parsed and added to current 
>> > >> section"""
>> > >
>> > > Imperative mood:
>> > >
>> > >     """Add a comment line, to be parsed and added to the current 
>> > > section."""
>> > >
>> > > However, we're not always adding to the current section, we can also
>> > > start a new one.  The following avoids suggesting anything about the
>> > > current section:
>> > >
>> > >     """Parse a comment line and add it to the documentation."""
>> > >
>> > > When the function name is a verb, and its doc string starts with a
>> > > different verb, the name might be suboptimal.  Use your judgement.
>> > >
>> > > If this one-liner is too terse, we can try a multi-line doc string:
>> > >
>> > >     """Parse a comment line and add it to the documentation.
>> > >
>> > >     TODO should we tell more about how we parse?
>> > >
>> > >     Args:
>> > >         line: the comment starting with '#', with the newline stripped.
>> > >
>> > >     Raises:
>> > >         QAPISchemaError: TODO explain error conditions
>> > >     """
>> > >
>> > > Format stolen from the Google Python Style Guide[2], because PEP257 is
>> > > of not much help there.
>> > >
>> > > Once you start with such elaborate doc strings, pressure will likely
>> > > mount to do the same elsewhere.  Could be a distraction right now.  Use
>> > > your judgement.
>> >
>> > ok, thanks
>> >
>> > >> +        line = line[1:]
>> > >> +        if not line:
>> > >> +            self._append_freeform(line)
>> > >> +            return
>> > >> +
>> > >> +        if line[0] != ' ':
>> > >> +            raise QAPISchemaError(self.parser, "missing space after #")
>> > >> +        line = line[1:]
>> > >
>> > > QAPISchemaError takes the error position from its QAPISchemaParser
>> > > argument.  In other words, the error position depends on the state of
>> > > the parser when it calls this function.  Turns out the position is the
>> > > beginning of the comment.  Servicable here.  Action at a distance,
>> > > though.
>> > >
>> > > Less strict:
>> > >
>> > >            # strip leading # and space
>> > >            line = line[1:]
>> > >            if line[0] == ' ':
>> > >                line = line[1:]
>> > >
>> > > Also avoids special-casing empty comments.
>> >
>> > That would raise "IndexError: string index out of range" on empty comment
>> > lines ("#")
>>
>> Fixable:
>>                if line.startswith(' '):
>>
>> > Also I'd like to keep the error in case a space is missing (it caught some
>> > already).
>>
>> On the one hand, I share your distaste for lack of space between # and
>> comment text.  On the other hand, I dislike enforcing spacing
>> inconsistently: enforce in doc comments, but not in other comments.
>
> It's only enforced for doc comments, which I think is fair.

If it's worth enforcing for some comments (which I doubt), then why
isn't it worth enforcing for all of them?  Anyway, it's something we can
fiddle with on top.

[...]
>> > > Not fixed since v3:
>> > >
>> > > * Flat union where 'base' is a string, e.g. union UserDefFlatUnion in
>> > >   qapi-schema-test.json has base 'UserDefUnionBase', args is set(['a',
>> > >   'B', 'e', 'D', 'f', 'i', 'o', 'n', 's', 'r', 'U'])
>> > >
>> > > * Command where 'data' is a string, e.g. user_def_cmd0 in
>> > >   qapi-schema-test.json has data 'Empty2', args is set(['E', 'm', 'p',
>> > >   '2', 't', 'y'])
>> > >
>> > > * Event where 'data' is a string, no test case handy (hole in test
>> > >   coverage)
>> >
>> > ok, I changed it that way, that fixes it:
>> > +    if isinstance(data, list):
>> > +        args = set([name.strip('*') for name in data])
>> > +    else:
>> > +        args = set()
>>
>> Uh, sure this does the right thing when data is a dict?
>>
>
> yes, the line above takes care of extracting the keys in data.

Ah, I misunderstood.  First mapping dict to list of keys, then dealing
with either list or other should indeed work.

>> > >> +    if meta == 'alternate' or \
>> > >> +       (meta == 'union' and not expr.get('discriminator')):
>> > >> +        args.add('type')
>> > >
>> > > As explained in review of v3, this is only a subset of the real set of
>> > > members.  Computing the exact set is impractical when working with the
>> > > abstract syntax tree.  I believe we'll eventually have to rewrite this
>> > > code to work with the QAPISchemaEntity instead.
>> >
>> > I don't think we want to list all the members as this would lead to
>> > duplicated documentation. Instead it should document only the members of
>> > the expr being defined.
>>
>> You're sticking to established practice, which makes lots of sense.
>> However, established practice is a lot more clear for simple cases than
>> for things like members inherited from base types, variant members and
>> such.  At some point, we'll have to figure out how we want not so simple
>> cases documented.  Until then, we can't really decide how to check
>> documentation completeness.
>>
>> >                         In which case, it looks like this check is good
>> > enough, no?
>>
>> For now, yes.  Later on, maybe.
>>
>> Let's document the limitation in a comment and the commit message, and
>> move on.
>
> I'd appreciate your help to list the limitations in the commit message, as
> I have not as thorough understanding as you, and even worse I often don't
> use the same name for the various concepts.

I'll try.

Deducing concept names from code isn't always easy...

[...]
>> > >> diff --git a/scripts/qapi2texi.py b/scripts/qapi2texi.py
>> > >> new file mode 100755
>> > >> index 0000000..0cec43a
>> > >> --- /dev/null
>> > >> +++ b/scripts/qapi2texi.py
[...]
>> > >> +def texi_union(expr, doc):
>> > >> +    """
>> > >> +    Format an union to texi
>> > >
>> > > I think it's "a union".
>> >
>> > ok
>> >
>> > >> +    """
>> > >> +    attrs = "@{ " + texi_args(expr, "base") + " @}"
>> > >> +    discriminator = expr.get("discriminator")
>> > >> +    if not discriminator:
>> > >> +        union = "Flat Union"
>> > >> +        discriminator = "type"
>> > >> +    else:
>> > >> +        union = "Union"
>> > >
>> > > Condition is backwards.
>> >
>> > fixed
>> >
>> > >> +    attrs += " + '%s' = [ " % discriminator
>> > >> +    attrs += texi_args(expr, "data") + " ]"
>> > >> +    body = texi_body(doc)
>> > >> +
>> > >> +    return STRUCT_FMT(type=union,
>> > >> +                      name=doc.symbol,
>> > >> +                      attrs=attrs,
>> > >> +                      body=body)
>> > >
>> > > You're inventing syntax here.  Example output:
>> > >
>> > >  -- Union: QCryptoBlockOpenOptions { QCryptoBlockOptionsBase } +
>> > >           'format' = [ 'qcow': QCRYPTOBLOCKOPTIONSQCOW, 'luks':
>> > >           QCRYPTOBLOCKOPTIONSLUKS ]
>> > >
>> > >      The options that are available for all encryption formats when
>> > >      opening an existing volume
>> > >           Since: 2.6
>> > >
>> > >  -- Flat Union: ImageInfoSpecific { } + 'type' = [ 'qcow2':
>> > >           IMAGEINFOSPECIFICQCOW2, 'vmdk': IMAGEINFOSPECIFICVMDK, 'luks':
>> > >           QCRYPTOBLOCKINFOLUKS ]
>> > >
>> > >      A discriminated record of image format specific information
>> > >      structures.
>> > >           Since: 1.7
>> > >
>> > > Note that QCryptoBlockOpenOptions is actually a flat union, and
>> > > ImageInfoSpecific a simple union.  As I said, the condition is
>> > > backwards.
>> > >
>> > > The meaning of the attrs part is unobvious.  Familiarity with schema
>> > > syntax doesn't really help.
>> > >
>> > > Could we simply use schema syntax here?
>> >
>> > Union: QCryptoBlockOpenOptions {
>> > 'base': QCryptoBlockOptionsBase,
>> > 'discriminator': 'format',
>> > 'data':  { 'qcow': QCryptoBlockOptionsQCow, 'luks':
>> > QCryptoBlockCreateOptionsLUKS }
>> >
>> > }
>> >
>> > Doesn't look obvious either and pollute the documentation with
>> > schema-specific parameters.
>>
>> The schema syntax for flat unions is pretty horrid :)  I hope to improve
>> it, but there's more urgent fish to fry.
>>
>> > > If not: whatever format you use, you first need to explain it.
>> >
>> > I am not sure my solution is the best and will remain, but ok let's try to
>> > document it for now.
>>
>> Before you pour time into documenting what you have, we should probably
>> discuss what we need.
>>
>
> Can this be marked as limitation and improved laster? The series is big,
> patch is getting bigger over time, it's quite hard to handle all your
> requirements in one go.

I'm sure it is!

How about this: you split off the code to derive documentation from the
schema expressions into its own patch.  The initial qapi2texi patch gets
a FIXME comment instead, explaining what kind of information the
generated documentation lacks.  I think it's basically member types.
The split-off patch then resolves that FIXME.

This lets me apply the first patch.  We can then discuss how to do the
second patch better.  If you're out of time or energy on this topic (I'd
understand), you could even leave it to me.  After all, it's me who's
particular about it...

[...]