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(-)
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>
---
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)
+ 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.
--
2.34.1
Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
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:
>
> > --- 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
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.
© 2016 - 2024 Red Hat, Inc.