[13/20] docs/qapidoc: fix nested parsing under untagged sections

Sphinx does not like sections without titles, because it wants to
convert every section into a reference. When there is no title, it
struggles to do this and transforms the tree inproperly.

Depending on the rST used, this may result in an assertion error deep in
the docutils HTMLWriter.

When parsing an untagged section (free paragraphs), skip making a hollow
section and instead append the parse results to the prior section.

Many Bothans died to bring us this information.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

John Snow <jsnow@redhat.com> writes:

> Sphinx does not like sections without titles, because it wants to
> convert every section into a reference. When there is no title, it
> struggles to do this and transforms the tree inproperly.
>
> Depending on the rST used, this may result in an assertion error deep in
> the docutils HTMLWriter.

I'm getting vibes of someone having had hours of "fun" with Sphinx...

Can you give you an idea of how a reproducer would look like?

> When parsing an untagged section (free paragraphs), skip making a hollow
> section and instead append the parse results to the prior section.
>
> Many Bothans died to bring us this information.

Terribly sad.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 34e95bd168d..cfc0cf169ef 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -286,14 +286,20 @@ def _nodes_for_sections(self, doc):
>              if section.tag and section.tag == 'TODO':
>                  # Hide TODO: sections
>                  continue
> +
> +            if not section.tag:
> +                # Sphinx cannot handle sectionless titles;
> +                # Instead, just append the results to the prior section.
> +                container = nodes.container()
> +                self._parse_text_into_node(section.text, container)
> +                nodelist += container.children
> +                continue
> +
>              snode = self._make_section(section.tag)
> -            if section.tag and section.tag.startswith('Example'):
> +            if section.tag.startswith('Example'):
>                  snode += self._nodes_for_example(dedent(section.text))
>              else:
> -                self._parse_text_into_node(
> -                    dedent(section.text) if section.tag else section.text,
> -                    snode,
> -                )
> +                self._parse_text_into_node(dedent(section.text), snode)
>              nodelist.append(snode)
>          return nodelist

Looks plausible.  I lack the Sphinx-fu to say more.

On Fri, Jun 14, 2024, 5:46 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > Sphinx does not like sections without titles, because it wants to
> > convert every section into a reference. When there is no title, it
> > struggles to do this and transforms the tree inproperly.
> >
> > Depending on the rST used, this may result in an assertion error deep in
> > the docutils HTMLWriter.
>
> I'm getting vibes of someone having had hours of "fun" with Sphinx...
>
> Can you give you an idea of how a reproducer would look like?
>

Yes - this is necessary for captioned example blocks that appear in
untagged sections, because those have titles.

When the sphinx html writer encounters a title under a section without a
title field, it malforms the tree (I cannot give you an example of this
easily, it's deep in the bowels) and produces an assertion error.

If you want to see it explode for yourself, just modify any untagged
section to include a captioned codeblock and watch it die.

If you apply either the note or Example conversion patches without this
fix, the old generator will choke. (Note patch dies because of my use of
".. admonition:: Notes", which also creates a title element.)

Simply put - docutils can tolerate title-less sections, Sphinx cannot. (And
it is not graceful about it.)


> > When parsing an untagged section (free paragraphs), skip making a hollow
> > section and instead append the parse results to the prior section.
> >
> > Many Bothans died to bring us this information.
>
> Terribly sad.
>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  docs/sphinx/qapidoc.py | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 34e95bd168d..cfc0cf169ef 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -286,14 +286,20 @@ def _nodes_for_sections(self, doc):
> >              if section.tag and section.tag == 'TODO':
> >                  # Hide TODO: sections
> >                  continue
> > +
> > +            if not section.tag:
> > +                # Sphinx cannot handle sectionless titles;
> > +                # Instead, just append the results to the prior section.
> > +                container = nodes.container()
> > +                self._parse_text_into_node(section.text, container)
> > +                nodelist += container.children
> > +                continue
> > +
> >              snode = self._make_section(section.tag)
> > -            if section.tag and section.tag.startswith('Example'):
> > +            if section.tag.startswith('Example'):
> >                  snode += self._nodes_for_example(dedent(section.text))
> >              else:
> > -                self._parse_text_into_node(
> > -                    dedent(section.text) if section.tag else
> section.text,
> > -                    snode,
> > -                )
> > +                self._parse_text_into_node(dedent(section.text), snode)
> >              nodelist.append(snode)
> >          return nodelist
>
> Looks plausible.  I lack the Sphinx-fu to say more.
>

Recommend just observing a before/after; the hash changes but the output
doesn't meaningfully change.

I intend to remove the old generator when we're done, so I think this is
probably safe to wave through with an ACK so long as there isn't
tremendously obvious regression (And, I have tested these patches from 3.x
to 7.x so I do not believe there is any compat risk.)

>