On Fri, Apr 19, 2024, 10:45 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@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 >
