Re: [PATCH 00/57] docs: Add new QAPI transmogrifier

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> John Snow <jsnow@redhat.com> writes:
>
>> Hi! This series is based on armbru/pull-qapi-2025-02-26.
>>
>> This series is a "minimum viable" version of the new QAPI documentation
>> system. It does the bare minimum under the new framework, saving nice
>> features for later.
>
> Not saved for later: a massive improvement of the generated
> documentation's looks and usability.  Moreover, I hope the new generator
> will be easier to maintain than the old one, because its inner workings
> are closer to how Sphinx expects such things to work.  Fair?
>
>>
>> Patches 3-29 implement the qapi_domain extension.
>> Patches 30-57 implement the qapidoc "Transmogrifier".
>>
>> This series is still "RFC" quality, though it's quite nearly actually
>> ready for inclusion. The "add transmogrifier test document" patch is not
>> intended for actual merge, it's just there to demonstrate the new
>> document generator by producing output in docs/manual/qapi/index.html.
>>
>> Known shortcomings in this series:
>>
>> - Still no new QAPI unit tests. I'll add those for next go-around.
>
> Not a blocker as far as I'm concerned, because I feel you're unlikely to
> run away from this :)
>
>> - No new documentation. Also for next revision. I'll document the QAPI
>>   domain syntax and give a brief overview of how the transmogrifier
>>   functions, and a quick rundown of any new rST syntax that may be
>>   pertinent to QAPI documentation writers.
>
> Likewise.
>
>> - IFCOND information is still rendered in two places, we'll need to
>>   decide where and how we want to render it.
>
> I'll have a look, and then we'll talk.

Two shortcomings, actually:

  - IFCOND in definitions (enum, struct, union, alternate, command,
    event) are rendered in two places.

    Example: query-tpm has 'if': 'CONFIG_TPM'.  The rendered
    documentation looks like this:

        Command query-tpm ["#if" "CONFIG_TPM"] (Since: 1.5)
            *Availability*: "CONFIG_TPM"

           Return information about the TPM device

           Example::
              [...]

    With the old generator, it looks like

        "query-tpm" (Command)
        ---------------------

        Return information about the TPM device


        Since
        ~~~~~

        1.5

        Example::
           [...]

        If
        ~~

        "CONFIG_TPM"

    So, three ways to present the information, none of them immediately
    obvious to a casual reader.  The easiest to guess right is perhaps
    the "Availability" box.

    The two new ways are in more conspicious spots than the old one is.
    Not sure I like that; I believe ifconds are fairly uninteresting to
    users of QMP most of the time.  More on that below.

    The "Availability" box is even more conspicious than the [#if ...]
    bracket.  It also uses more space.

    For complex conditions, the [#if ...] bracket can make the first
    line less readable.  Example:

        Command query-cpu-definitions ["#if" "TARGET_PPC or TARGET_ARM or TARGET_I386 or TARGET_S390X or TARGET_MIPS or TARGET_LOONGARCH64 or TARGET_RISCV"] (Since: 1.2)
            *Availability*: "TARGET_PPC or TARGET_ARM or TARGET_I386 or
           TARGET_S390X or TARGET_MIPS or TARGET_LOONGARCH64 or TARGET_RISCV"

           Return a list of supported virtual CPU definitions

    The first line matters.  This tips the balance for me.  Let's go
    with the "Availability" box at least for now.

    There's a symbol before "Availability" in the generated HTML.  Its
    meaning is less than obvious.  I remember you explained it to me,
    but I forgot.  We can polish this later.

  - IFCOND elsewhere is not rendered at all.  This is a regression.  The
    old generator shows it like this:

        "mptcp": "boolean" (optional) (**If: **"HAVE_IPPROTO_MPTCP")
           enable multi-path TCP.  (Since 6.1)

    I believe the regression is tolerable for now, because the
    information lost is only modestly useful for users of QMP, and the
    massive improvements the new generator provides outweigh this small
    loss.

    Why only modestly useful?  It tells the reader the thing's
    availability depends on how QEMU was built.  It also provides a clue
    that should let a developer or sufficiently clever non-developer
    rebuild QEMU to provide the thing.  This would involve some code
    spelunking.

>> - No QAPI namespace support ... yet. So we can't enable it for QMP, QGA
>>   and QSD simultaneously just yet. I don't think it will be difficult.

[...]