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

[Markus Armbruster <armbru@redhat.com>]

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.

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.

How do you handle features in general, and special features in
particular?

How do you handle conditionals in general?

How do you combine feature and conditional?

How could you combine special feature and conditonal?

[...]