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

[Peter Maydell]

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.

> ---
> https://qemu-project.gitlab.io/qemu/system/i386/xen.html tells the user
> to "see the command line documentation for the -initrd option". It'd be
> a whole lot nicer if we could *link* to it. It actually worked on my
> test box, but only because I'm using an older version of Sphinx which
> didn't complain about the duplicate refs, and just picked *one* to link
> to.
>
>  docs/sphinx/hxtool.py      | 18 +++++++++++++++++-
>  docs/system/i386/xen.rst   |  2 +-
>  docs/system/invocation.rst |  1 +
>  qemu-options.hx            |  2 +-
>  4 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/docs/sphinx/hxtool.py b/docs/sphinx/hxtool.py
> index 9f6b9d87dc..bfb0929573 100644
> --- a/docs/sphinx/hxtool.py
> +++ b/docs/sphinx/hxtool.py
> @@ -78,18 +78,28 @@ def parse_archheading(file, lnum, line):
>          serror(file, lnum, "Invalid ARCHHEADING line")
>      return match.group(1)
>
> +def parse_srst(file, lnum, line):
> +    """Handle an SRST directive"""
> +    # The input should be "SRST(label)".
> +    match = re.match(r'SRST\((.*?)\)', line)
> +    if match is None:
> +        serror(file, lnum, "Invalid SRST line")
> +    return match.group(1)
> +
>  class HxtoolDocDirective(Directive):
>      """Extract rST fragments from the specified .hx file"""
>      required_argument = 1
>      optional_arguments = 1
>      option_spec = {
> -        'hxfile': directives.unchanged_required
> +        'hxfile': directives.unchanged_required,
> +        'emitrefs': directives.flag
>      }
>      has_content = False
>
>      def run(self):
>          env = self.state.document.settings.env
>          hxfile = env.config.hxtool_srctree + '/' + self.arguments[0]
> +        emitrefs = "emitrefs" in self.options
>
>          # Tell sphinx of the dependency
>          env.note_dependency(os.path.abspath(hxfile))
> @@ -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.)

>
>  Host OS requirements
>  --------------------
> diff --git a/docs/system/invocation.rst b/docs/system/invocation.rst
> index 4ba38fc23d..ef75dad2e2 100644
> --- a/docs/system/invocation.rst
> +++ b/docs/system/invocation.rst
> @@ -11,6 +11,7 @@ disk_image is a raw hard disk image for IDE hard disk 0. 
> Some targets do
>  not need a disk image.
>
>  .. hxtool-doc:: qemu-options.hx
> +    :emitrefs:
>
>  Device URL Syntax
>  ~~~~~~~~~~~~~~~~~
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 42fd09e4de..464e7257b0 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3987,7 +3987,7 @@ ERST
>
>  DEF("initrd", HAS_ARG, QEMU_OPTION_initrd, \
>             "-initrd file    use 'file' as initial ram disk\n", QEMU_ARCH_ALL)
> -SRST
> +SRST(initrd)
>
>  ``-initrd file``
>      Use file as initial ram disk.
> --
> 2.34.1

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