On Fri, Jan 10, 2025 at 7:19 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@redhat.com> writes:
>
> > On Thu, Jan 9, 2025, 5:34 AM Markus Armbruster <arm...@redhat.com>
> wrote:
> >
> >> John Snow <js...@redhat.com> writes:
> >>
> >> > On Fri, Dec 20, 2024 at 9:15 AM Markus Armbruster <arm...@redhat.com>
> wrote:
> >> >
> >> >> John Snow <js...@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 <js...@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.
>
> >> [...]
>
>
Hm, alright. I think I'll just stub out conditionals with a TODO and circle
back to how I'll handle them; I need to do a pass on ifcond handling in
general, so I guess I'll get to it then. I still think it's a little weird,
but you don't, and you're the QAPI guy ;)
--js
