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