On Tue, 2023-12-12 at 15:04 +0000, Peter Maydell wrote: > On Thu, 9 Nov 2023 at 10:33, Woodhouse, David <d...@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 <d...@amazon.co.uk> > > Reviewed-by: Paul Durrant <p...@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.ca...@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
