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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Fri, Jan 10, 2025 at 7:19 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> 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.

[...]

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

Makes sense (although I'd use FIXME rather than TODO).

If resolving them turns out to be hard, we can talk about adding
restrictions to help you over the hump.

>                                          I still think it's a little weird,
> but you don't, and you're the QAPI guy ;)

I admit constructing believable use cases took a bit of thought.
Calling them a little weird can't offend me :)