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