Re: [PATCH 00/27] Add qapi-domain Sphinx extension

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 10664 bytes --]

On Fri, Apr 19, 2024, 10:45 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This series adds a new qapi-domain extension for Sphinx, which adds a
> > series of custom directives for documenting QAPI definitions.
> >
> > GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/1259566476
> >
> > (Link to a demo HTML page at the end of this cover letter, but I want
> > you to read the cover letter first to explain what you're seeing.)
> >
> > This adds a new QAPI Index page, cross-references for QMP commands,
> > events, and data types, and improves the aesthetics of the QAPI/QMP
> > documentation.
>
> Cross-references alone will be a *massive* improvement!  I'm sure
> readers will appreciate better looks and an index, too.
>
> > This series adds only the new ReST syntax, *not* the autogenerator. The
> > ReST syntax used in this series is, in general, not intended for anyone
> > to actually write by hand. This mimics how Sphinx's own autodoc
> > extension generates Python domain directives, which are then re-parsed
> > to produce the final result.
> >
> > I have prototyped such a generator, but it isn't ready for inclusion
> > yet. (Rest assured: error context reporting is preserved down to the
> > line, even in generated ReST. There is no loss in usability for this
> > approach. It will likely either supplant qapidoc.py or heavily alter
> > it.) The generator requires only extremely minor changes to
> > scripts/qapi/parser.py to preserve nested indentation and provide more
> > accurate line information. It is less invasive than you may
> > fear. Relying on a secondary ReST parse phase eliminates much of the
> > complexity of qapidoc.py. Sleep soundly.
>
> I'm a Sphinx noob.  Let me paraphrase you to make sure I understand.
>
> You proprose to generate formatted documentation in two steps:
>
> • First, the QAPI generator generates .rst from the QAPI schema.  The
>   generated .rst makes use of a custom directives.
>

Yes, but this .rst file is built in-memory and never makes it to disk, like
Sphinx's autodoc for Python.

(We can add a debug knob to log it or save it out to disk if needed.)


> • Second, Sphinx turns the .rst into formatted documentation.  A Sphinx
>   qapi-domain extension implements the custom directives
>

Yes.


> This mirrors how Sphinx works for Python docs.  Which is its original
> use case.
>
> Your series demonstrates the second step, with test input you wrote
> manually.
>
> You have code for the first step, but you'd prefer to show it later.
>

Right, it's not fully finished, although I have events, commands, and
objects working. Unions, Alternates and Events need work.


> Fair?
>

Bingo!


> > The purpose of sending this series in its current form is largely to
> > solicit feedback on general aesthetics, layout, and features. Sphinx is
> > a wily beast, and feedback at this stage will dictate how and where
> > certain features are implemented.
>
> I'd appreciate help with that.  Opinions?


> > A goal for this syntax (and the generator) is to fully in-line all
> > command/event/object members, inherited or local, boxed or not, branched
> > or not. This should provide a boon to using these docs as a reference,
> > because users will not have to grep around the page looking for various
> > types, branches, or inherited members. Any arguments types will be
> > hyperlinked to their definition, further aiding usability. Commands can
> > be hotlinked from anywhere else in the manual, and will provide a
> > complete reference directly on the first screenful of information.
>
> Let me elaborate a bit here.
>
> A command's arguments can be specified inline, like so:
>
>     { 'command': 'job-cancel', 'data': { 'id': 'str' } }
>
> The arguments are then documented right with the command.
>
> But they can also be specified by referencing an object type, like so:
>
>     { 'command': 'block-dirty-bitmap-remove',
>       'data': 'BlockDirtyBitmap' }
>
> Reasons for doing it this way:
>
> • Several commands take the same arguments, and you don't want to repeat
>   yourself.
>
> • You want generated C take a single struct argument ('boxed': true).
>
> • The arguments are a union (which requires 'boxed': true).
>
> Drawback: the arguments are then documented elsewhere.  Not nice.
>
> Bug: the generated documentation fails to point there.
>
> You're proposing to inline the argument documentation, so it appears
> right with the command.
>
> An event's data is just like a command's argument.
>
> A command's return value can only specified by referencing a type.  Same
> doc usability issue.
>
> Similarly, a union type's base can specified inline or by referencing a
> struct type, and a union's branches must be specified by referencing a
> struct type.  Same doc usability issue.
>
> At least, the generated documentation does point to the referenced
> types.
>

Right. My proposal is to recursively inline referenced bases for the
top-level members so that this manual is useful as a user reference,
without worrying about the actual QAPI structure.

Return types will just be hotlinked.


> > (Okay, maybe two screenfuls for commands with a ton of
> > arguments... There's only so much I can do!)
>
> *cough* blockdev-add *cough*


> > This RFC series includes a "sandbox" .rst document that showcases the
> > features of this domain by writing QAPI directives by hand; this
> > document will be removed from the series before final inclusion. It's
> > here to serve as a convenient test-bed for y'all to give feedback.
> >
> > All that said, here's the sandbox document fully rendered:
> > https://jsnow.gitlab.io/qemu/qapi/index.html
> >
> > And here's the new QAPI index page created by that sandbox document:
> > https://jsnow.gitlab.io/qemu/qapi-index.html
> >
> > Known issues / points of interest:
> >
> > - The formatting upsets checkpatch. The default line length for the
> >   "black" formatting tool is a little long. I'll fix it next spin.
> >
> > - I did my best to preserve cross-version compatibility, but some
> >   problems have crept in here and there. This series may require
> >   Sphinx>= 4.0, like the dbus extension. Notably, the Ubuntu build fails
> >   on Gitlab CI currently. The next version will text against more Sphinx
> >   versions more rigorously. Sphinx version 5.3.0 and above should work
> >   perfectly.
> >
> > - There's a bug in Sphinx itself that may manifest in your testing,
> >   concerning reported error locations. There's a patch in this series
> >   that fixes it, but it's later in the series. If you run into the bug
> >   while testing with this series, try applying that patch first.
> >
> > - QAPI 'namespaces' aren't yet handled. i.e., there's nothing to
> >   distinguish entities between QMP, QGA and QSD yet. That feature will
> >   be added to a future version of this patchset (Likely when the
> >   generator is ready for inclusion: without it, references will clash
> >   and the index will gripe about duplicated entries.)
>
> qemu-storage-daemon's QMP is a proper subset of qemu-system-FOO's.
> Regardless, each of them has its own, independent reference manual.
> That's defensible.
>
> But the way we build them can complicate matters.  For instance, when I
> tried to elide types not used for QMP from the reference manuals, I got
> defeated by Sphinx caching.
>

I haven't tried yet, but I believe it should be possible to "tag" each
referenced QAPI object and mark it as "visited". Each namespaced definition
would be tagged separately.

(i.e. qemu.block-core.blockdev-backup and qsd.block-core.blockdev-backup
would be two distinct entities.)

Then, the renderer could ignore/squelch untagged entities. (I think.)

Maybe some work to do to un-index unvisited entities.

Or we can go the other route and bake this info into the schema and squelch
stuff earlier. We can add this feature later. I am still not sure why your
prototype for this ran into cache issues, but I am convinced it should be
fixable by making namespaced entities distinct.

We could perhaps bake the namespace into the qapidoc directive, e.g.:

 .. qapi:autodoc:: schema.json
   :namespace: QSD

(And the default adds no namespace.)


> > - Per-member features and ifcond are not yet accounted for; though
> >   definition-scoped features and ifconds are. Please feel free to
> >   suggest how you'd like to see those represented.
> >
> > - Inlining all members means there is some ambiguity on what to do with
> >   doc block sections on inlined entities; features and members have an
> >   obvious home - body, since, and misc sections are not as obvious on
> >   how to handle. This will feature more prominently in the generator
> >   series.
>
> Yes, this is a real problem.
>
> The member documentation gets written in the context of the type.  It
> may make sense only in that context.  Inlining copies it into a
> different context.


> Features may need to combine.  Say a command's arguments are a union
> type, and several branches of the union both contain deprecated members.
> These branch types all document feature @deprecated.  Simply inlining
> their feature documentation results in feature @deprecated documented
> several times.  Ugh.  Combining them would be better.  But how?  Do we
> need to rethink how we document features?
>

To be honest, I figured I'd plow ahead and then find the counter-examples
programmatically and decide what to do once I had my pile of edge cases.

It might involve rewriting docs in some places, but I think the usability
win is completely worth the hassle.

It's possible there might be some rework needed to maximize cogency of the
generated docs, but I think prioritizing a user-facing reference for QMP is
the objectively correct end goal and I'm more than happy to work backwards
from that desired state.


> > - Some features could be implemented in either the QAPI domain syntax
> >   *or* the generator, or some combination of both. Depending on
> >   aesthetic feedback, this may influence where those features should be
> >   implemented.
> >
> > - The formatting and aesthetics of branches are a little up in the air,
> >   see the qapi:union patch for more details.
> >
> > - It's late, maybe other things. Happy Friday!
> >
> > Hope you like it!
>
> Looks promising!
>

Fingers crossed. This has bothered me about QEMU for years.

--js

>

[-- Attachment #2: Type: text/html, Size: 15333 bytes --]