On Thu, 9 Nov 2023 at 10:33, Woodhouse, David <[email protected]> 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 <[email protected]> > Reviewed-by: Paul Durrant <[email protected]>
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/[email protected]/ 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
