Re: [PATCH] doc/sphinx/hxtool.py: add optional label argument to SRST directive

[David Woodhouse]

On Tue, 2023-12-12 at 15:04 +0000, Peter Maydell wrote:
> On Thu, 9 Nov 2023 at 10:33, Woodhouse, David <dwmw@amazon.co.uk> wrote:
> > 
> > We can't just embed labels directly into files like qemu-options.hx which
> > are included from multiple top-level RST files, because Sphinx sees the
> > labels as duplicate: https://github.com/sphinx-doc/sphinx/issues/9707
> > 
> > So add an 'emitrefs' option to the Sphinx hxtool-doc directive, which is
> > set only in invocation.rst and not from the HTML rendition of the man
> > page. Along with an argument to the SRST directive which causes a label
> > of the form '.. _LABEL-reference-label:' to be emitted when the emitrefs
> > option is set.
> > 
> > Now where the Xen PV documentation refers to the documentation for the
> > -initrd command line option, it can emit a link directly to it.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > Reviewed-by: Paul Durrant <paul@xen.org>
> 
> Thanks for splitting out this patch, and sorry I didn't get to
> reviewing it earlier. The general idea is great, and I have
> a few suggested tweaks below.
> 
> Something is weird about how you're sending out patchmails,
> by the way: the patch appears in lore.kernel.org and in patchew,
> but when patchew tries to apply it, or when I do locally, git complains
> that it's empty:
> https://patchew.org/QEMU/7a25bd4ee1f8b06c7a51d20486aaa8bc8e1282ea.camel@amazon.co.uk/
> 
> I think this is probably because the patch is a lot of
> base-64-encoded multipart/mime. Sending it as good old
> fashioned plain text will likely work better.

Yeah, I suspect that's a tooling bug; 'git am' and the like really
*ought* to be able to decode MIME, including QP and base64, and
converting legacy 8-bit character sets to UTF-8. But in reality it's
all a bit haphazard.

I do generally *try* to send through my own mail servers instead of
through Exchange which likes to turn everything into base64. Must have
selected the wrong account as the sender that time.

> > @@ -113,6 +123,12 @@ def run(self):
> >                          serror(hxfile, lnum, 'expected ERST, found SRST')
> >                      else:
> >                          state = HxState.RST
> > +                        if emitrefs and line != "SRST":
> > +                            label = parse_srst(hxfile, lnum, line)
> 
> I think that rather than only calling parse_srst() under this
> if(), we should do it always, and have parse_srst() accept
> "SRST" alone as valid (meaning empty label, same as "SRST()").
> Then we can append to the rstlist 'if emitrefs and label != ""'.
> 
> > +                            if label != "":
> > +                                rstlist.append("", hxfile, lnum - 1)
> > +                                refline = ".. _" + label + 
> > "-reference-label:"
> > +                                rstlist.append(refline, hxfile, lnum - 1)
> >                  elif directive == 'ERST':
> >                      if state == HxState.CTEXT:
> >                          serror(hxfile, lnum, 'expected SRST, found ERST')
> > diff --git a/docs/system/i386/xen.rst b/docs/system/i386/xen.rst
> > index 81898768ba..536dd6a2f9 100644
> > --- a/docs/system/i386/xen.rst
> > +++ b/docs/system/i386/xen.rst
> > @@ -132,7 +132,7 @@ The example above provides the guest kernel command 
> > line after a separator
> >  (" ``--`` ") on the Xen command line, and does not provide the guest kernel
> >  with an actual initramfs, which would need to listed as a second multiboot
> >  module. For more complicated alternatives, see the command line
> > -documentation for the ``-initrd`` option.
> > +:ref:`documentation <initrd-reference-label>` for the ``-initrd`` option.
> 
> I think we should include the hxfile basename in the label name
> we generate. We also don't need to say "label", it's implicitly a
> label. Then when we refer to things we can say
>    <qemu-options-initrd>
>    <hmp-commands-screendump>
> 
> and it's fairly readable what we're referring back to.
> 
> (We could alternatively have the emitrefs option take an argument
> for what to use in label names. I don't have a strong view on
> which would be better.)

> We really need to document the .hx file syntax (currently this is
> only in comments at the top of individual .hx files). I'll
> put together a quick patch that does that, which will give us
> somewhere to add the information about how label-generation
> works in this patch.

Thanks. That one is merged now; I'll dust the label patch off and
address your comments above, and resend.

smime.p7s
Description: S/MIME cryptographic signature