| Message ID | 7a25bd4ee1f8b06c7a51d20486aaa8bc8e1282ea.camel@amazon.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | doc/sphinx/hxtool.py: add optional label argument to SRST directive | expand |
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
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.
On Tue, 2023-12-12 at 15:04 +0000, Peter Maydell wrote: > > > --- 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.) Hm, wait... I did this as you suggest in v2 but can I also use this trick to eliminate the 'emitrefs' option altogether? Remember, the problem was that qemu-options.hx gets included *both* from invocation.rst and from qemu-manpage.rst, so the label gets emitted twice and is thus ambiguous. The 'emitrefs' option prevented it from being emitted, but is one more hoop to jump through for the next person who wants to use this facility. As I mentioned the 'emitrefs' flag in the documentation, the "if it needs documenting, FIX IT" instinct kicked in hard... What if we build the top-level filename into the label, e.g. invocation-qemu-options-initrd Then we don't need 'emitrefs' at all.
On Sat, 27 Jan 2024 at 10:16, David Woodhouse <dwmw2@infradead.org> wrote: > > On Tue, 2023-12-12 at 15:04 +0000, Peter Maydell wrote: > > > > > --- 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.) > > Hm, wait... I did this as you suggest in v2 but can I also use this > trick to eliminate the 'emitrefs' option altogether? > > Remember, the problem was that qemu-options.hx gets included *both* > from invocation.rst and from qemu-manpage.rst, so the label gets > emitted twice and is thus ambiguous. The 'emitrefs' option prevented it > from being emitted, but is one more hoop to jump through for the next > person who wants to use this facility. As I mentioned the 'emitrefs' > flag in the documentation, the "if it needs documenting, FIX IT" > instinct kicked in hard... > > What if we build the top-level filename into the label, e.g. > invocation-qemu-options-initrd > > Then we don't need 'emitrefs' at all. Yeah, that seems like a reasonable approach too: the label name gets a bit longer and clunkier but avoiding the need to have to say specifically "emit labels like this" is nice. -- PMM
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) + 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. 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.