Re: [PATCH 11/23] docs/qapidoc: add preamble() method

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Thu, Jan 9, 2025, 5:34 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > On Fri, Dec 20, 2024 at 9:15 AM Markus Armbruster <armbru@redhat.com> 
>> > wrote:
>> >
>> >> John Snow <jsnow@redhat.com> writes:
>> >>
>> >> > This method adds the options/preamble to each definition block. Notably,
>> >> > :since: and :ifcond: are added, as are any "special features" such as
>> >> > :deprecated: and :unstable:.
>> >> >
>> >> > Signed-off-by: John Snow <jsnow@redhat.com>
>> >> > ---
>> >> >  docs/sphinx/qapidoc.py | 33 ++++++++++++++++++++++++++++++++-
>> >> >  1 file changed, 32 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> >> > index 6f8f69077b1..85c7ce94564 100644
>> >> > --- a/docs/sphinx/qapidoc.py
>> >> > +++ b/docs/sphinx/qapidoc.py
>> >> > @@ -38,7 +38,7 @@
>> >> >  from qapi.error import QAPIError, QAPISemError
>> >> >  from qapi.gen import QAPISchemaVisitor
>> >> >  from qapi.parser import QAPIDoc
>> >> > -from qapi.schema import QAPISchema
>> >> > +from qapi.schema import QAPISchema, QAPISchemaEntity
>> >> >  from qapi.source import QAPISourceInfo
>> >> >
>> >> >  from sphinx import addnodes
>> >> > @@ -125,6 +125,37 @@ def ensure_blank_line(self) -> None:
>> >> >              # +2: correct for zero/one index, then increment by one.
>> >> >              self.add_line_raw("", fname, line + 2)
>> >> >
>> >> > +    # Transmogrification helpers
>> >> > +
>> >> > +    def preamble(self, ent: QAPISchemaEntity) -> None:
>> >> > +        """
>> >> > +        Generate option lines for qapi entity directives.
>> >> > +        """
>> >> > +        if ent.doc and ent.doc.since:
>> >> > +            assert ent.doc.since.tag == QAPIDoc.Tag.SINCE
>> >> > +            # Generated from the entity's docblock; info location is 
>> >> > exact.
>> >> > +            self.add_line(f":since: {ent.doc.since.text}", 
>> >> > ent.doc.since.info)
>> >> > +
>> >> > +        if ent.ifcond.is_present():
>> >> > +            doc = ent.ifcond.docgen()
>> >> > +            # Generated from entity definition; info location is 
>> >> > approximate.
>> >> > +            self.add_line(f":ifcond: {doc}", ent.info)
>> >> > +
>> >> > +        # Hoist special features such as :deprecated: and :unstable:
>> >> > +        # into the options block for the entity. If, in the future, new
>> >> > +        # special features are added, qapi-domain will chirp about
>> >> > +        # unrecognized options and fail.
>> >> > +        for feat in ent.features:
>> >> > +            if feat.is_special():
>> >> > +                # We don't expect special features to have an ifcond 
>> >> > property.
>> >> > +                # (Hello, intrepid developer in the future who changed 
>> >> > that!)
>> >> > +                # ((With luck, you are not me.))
>> >> > +                assert not feat.ifcond.is_present()
>> >>
>> >> Nope :)
>> >>
>> >> The attempt to add a conditional special feature now fails with
>> >>
>> >>     Sphinx parallel build error:
>> >>     AssertionError
>> >>
>> >> If you want to outlaw conditional special features, reject them cleanly
>> >> in schema.py, document the restriction in docs/devel/qapi-code-gen.rst,
>> >> and explain why in the commit message.  Recommend a separate commit, to
>> >> make it stand out in git-log.
>> >
>> > Do you advocate this? I wasn't sure what it *meant* for a special feature
>> > to be conditional; I couldn't conceive of what it meant to have an ifcond
>> > for "deprecated" or "unstable", for instance. It sounds like it isn't well
>> > defined, but we happen to not expressly forbid it.
>>
>> Semantics are clear enough to me.
>>
>> "Conditional special feature" combines "conditional feature" (which is a
>> special case of conditional thing) with special feature (which is a
>> special case of feature).
>>
>> The QAPI schema language supports compile-time conditionals for certain
>> things.  Generated code behaves as if the thing didn't exist unless its
>> condition is true.
>>
>> QAPI schema features are strings exposed to clients in introspection.
>>
>> Combine the two: a conditional feature is exposed if and only if its
>> condition is true.
>>
>> Existing uses: dynamic-auto-read-only if CONFIG_POSIX, fdset if
>> CONFIG_BLKIO_VHOST_VDPA_FD.
>>
>> A special feature is a feature that has special meaning to the
>> generator, i.e. we generate different code in places when it's present.
>>
>> Combine: we enable different code for a conditional special feature only
>> when its condition is true.
>>
>> No existing uses so far.
>>
>> Implementation is straightforward, too.
>>
>
> Mechanically, it's well defined... but that isn't my concern.
>
> What I asked you was: "what would it mean for deprecated to be conditional?"
>
> Without understanding that, I have no chance to design a system that
> handles that information accordingly for the documentation.

Special feature deprecated means the use of the thing that has it is
deprecated with this particular qemu-system-FOO.

Why "this particular"?  Because it wasn't deprecated in some prior
version of QEMU.  Features (special or not) are always, always about the
specific qemu-system-FOO binary you're talking to via QMP.

Conditionals are compile-time schema configuration.  Either the
condition is true or not at compile time.  If it's true, the thing that
has it is there, else it isn't.

Now combine the two.  If the condition of special feature 'deprecate' is
true, the thing that has it is deprecated with this particular
qemu-system-FOO.  Else it isn't.

>> Any code we generate for a conditional thing is guarded by #if
>> ... #endif.
>>
>> For features, we generate suitable data into qapi-introspect.c.
>>
>> For special features, we generate a little additional code here and
>> there; look for .is_special() to find it.
>>
>> Bug: this additional code lacks #if ... #endif.  Simple oversight,
>> should be easy enough to fix.
>>
>> > I guard against it here because, similarly, I have no idea how to handle
>> > the case where it's true.
>> >
>> > I didn't realize we technically allow it, though ... would you like me to
>> > move to expressly forbid it in the parser? (Failing that, I have no idea
>> > how to display this information otherwise, so I'd need you to sketch
>> > something out for me; so my inclination is to forbid it as you suggest.
>> > Future developers can always lift the restriction once they have some
>> > use-case in mind and a plan for how to display that information.)
>>
>> I think we should first make a reasonable effort at figuring out how to
>> handle conditional special features.  If we fail, we can add the
>> restriction instead.
>
> I don't think I agree; I don't think it's worth spending time defining a
> feature we don't use in our minds to then design around a hypothetical
> future use.
>
> Easier to admit we don't use it and save defining the semantics for the
> future developer who stumbles across a reason for needing it.

Semantics of "apply conditional to feature" don't need defining, because
they are simply the same as "apply conditional to any other thing."
When you have a definition for the general case that works for all the
special cases, then outlawing one special case *increases* complexity.
That's why I'm reluctant.

I figure your actual problem more concrete than "what does it even
mean?", namely how to present conditionals in generated documentation.
In that context, features *may* differ from any other thing!

Aside: the (mostly generated) QMP introspection is about this particular
qemu-system-FOO, like all of QMP, whereas the (mostly generated) QMP
documentation is about any qemu-system-FOO that could be built from this
particular working tree.  May lead to doc generation problems that have
no match elsewhere.

>> How do you handle features in general, and special features in
>> particular?
>>
>
> Features: they go in the :feat: field list.
> Special features: they go in the :feat: field list, and also receive a
> special option flag to the directive to allow for special rendering of the
> "signature bar" (the header line for a given entity definition in the
> rendered documentation.)
>
> e.g.
>
> @deprecated will generate two things:
>
> 1) the definition block will get a :deprecated: option, which enables the
> html renderer to (potentially) do special things like color the entry
> differently, add a "warning pip"/eyecatch, etc. It's scoped to the whole
> definition.
>
> 2) A :feat deprecated: line which is added to the features "field list" as
> per normal for features.
>
>
>> How do you handle conditionals in general?
>>
>
> In this series, I don't!
>
> ...except for top-level conditionals, which are currently passed as-is as
> an argument to the :ifcond: directive option. The format of that argument
> and what the HTML renderer actually does with it is currently TBD.
>
> (In current prototypes I just print the conditional string in the signature
> bar.)
>
>
>> How do you combine feature and conditional?
>>
>
> Notably TBD. Unhandled both in this series as-is *and* in my larger WIP.

How do you combine definition and conditional?  You answered that right
above: largely TBD.

How do you combine enum value / object type member / command argument /
event argument and conditional?  Also answered right above: not yet.

What does that mean for the doc generator code?  Does it simply ignore
conditionals there?

>> How could you combine special feature and conditonal?
>>
>
> Bewilderingly uncertain.
>
> Mechanically, it *could* be handled the same way :ifcond: directive options
> are.
>
> e.g.
>
> ```
> .. qapi:command:: x-beware-of-the-leopard
>    :deprecated: {ifcond stuff here}
>    :unstable: {ifcond stuff here}
>
>    lorem ipsum dolor sit amet
>
>    :feat deprecated: blah blah (if: ...)
>    :feat unstable: blah blah (if: ...)
> ```
>
> Semantically and in the rendered output, I have absolutely no clue
> whatsoever; and doubt I could figure it out without a use case in front of
> me to design around ... so I'd prefer to put up a traffic barrier for now.
>
> i.o.w. the design issue arises specifically because special features are
> semanticaly special and are "hoisted" to be block-level in the
> transmogrified rST doc, and so syntax needs to be designed, implemented and
> tested if they are to be conditional.

You choose to make them special in the documentation.  They haven't been
special there so far.  I'm not telling you not to, I'm just pointing out
this is a problem we elect to have :)

> Yet, we have no such cases to facilitate the design, implementation and
> testing.
>
> So, I'd like to prohibit until such time as we have such a case.

I believe what you need isn't so much an explanation of semantics, it's
use cases.  And that's fair!

Let me offer two.

1. Imagine we're trying to develop something big & complex enough to
   make keeping it out of tree until done impractical.  We feel even an
   in-tree branch would be impractical.  Instead, we commit it to
   master, default off, configure --enable-complex-thing to get it.  Not
   exactly something we do all the time, but not outlandish, either.

   Further imagine that the big & complex thing will involve some new
   QMP commands replacing existing ones.  As usual, we want to mark the
   ones being replaced deprecated, so we can remove them after a grace
   period.

   But simply deprecating them in the schema would be premature!  The
   big & complex thing may fail, and if it does, we rip it out.  If it
   succeeds, we enable it unconditionally.

   We can express what we're doing by making feature deprecated
   conditional on CONFIG_COMPLEX_THING, which is defined by
   --enable-complex-thing.

   That way, the complex thing doesn't affect the QMP interface unless
   we enable it.  Good.  And if we enable it, we do get the deprecation,
   and can test management applications cope with it.

2. Imagine we turned allow-preconfig into a special feature.  Further
   imagine some whole-stack feature requires a certain command to have
   allow-preconfig, and you got tasked with implementing it.  Which you
   duly did, to everybody's satisfaction.  It is now days to the
   release, rc4 has sailed, and some dude comes out of the woodwork to
   report the command crashes in preconfig state.  Weird!  It worked
   *fine* in your meticulous testing.  After some back and forth, you
   discover that the reporter builds with --enable-exotic-crap, which
   you didn't test, because you weren't even aware it exists.  You
   enable it, and now the command crashes for you, too.  Fixing this
   after rc4 is out of the question, too much churn.  You could,
   however, make feature allow-preconfig conditional on not
   CONFIG_EXOTIC_CRAP.

>> [...]