[ovs-dev] [RFC 2/2] doc: Convert ovs-vswitchd to rST

Fri May 19 09:48:06 UTC 2017

On Thu, 2017-05-18 at 16:02 -0700, Ben Pfaff wrote:
> Bravo.  This must have been a lot of work.
> 
> More comments follow...
> 
> On Wed, May 10, 2017 at 09:32:19PM -0400, Stephen Finucane wrote:
> > This is the largest man page by some distance. There are a couple
> > of changes needed to make this work:
> > 
> > - Combining italics and bold without spaces (i.e. <b>hello</b>-
> > <i>world</i>) and nesting italics and bold (i.e. <b><i>hello-
> > world</i></b>) is not supported by rST. Where present, this has
> > been removed.
> > 
> > - Duplicated opts are not possible. Where before we would have had
> > a list like so:
> > 
> >     -vPATTERN:destination:pattern
> >       Description
> > 
> >     -vFACILITY:facility
> >       Description
> > 
> >    We now have:
> > 
> >      -v <verbosity>
> > 
> >        PATTERN:<destination>:<pattern>
> >          Description
> > 
> >        FACILITY:<facility>
> >          Description
> > 
> >   This is necessary if we want to make best use of the Sphinx
> > indexing.
> 
> I'm not too aware of Sphinx indexing.  When I type "Sphinx indexing"
> into Google, it tells me about a search engine named Sphinx, which I
> don't think is what you mean.  Can you point to something about it?

Sure - here's the example from our own docs:

http://docs.openvswitch.org/en/latest/genindex/

> If the indexing is just a matter of being able to jump to the
> definition of -v from somewhere else, I wonder whether it matters
> whether both definitions are indexed?  I think that both definitions
> would be right next to each other in this case.

In fairness, it probably doesn't. I'll change this to use 'object' for
all except the most basic, no-arg one. 'object' produces the same
formatting as 'option' but doesn't generate a reference/add things to
the index.

> There's a bigger problem with the above transformation, though: -v
> takes an optional argument, which means that only accepts an argument
> if the argument is mashed up against it.  If you write "-v ..." with
> a space, it's not an argument, it's "-v" followed by a non-option
> argument.

They didn't account for this when writing the Sphinx directive, clearly
:) We should resolve this particular case with the above change, but
I'll have to rely on you to look for any other cases I've missed.

> > - TODO
> > 
> > Signed-off-by: Stephen Finucane <stephen at that.guru>
> >
> > - There's a big loss of formatting for subcommands. While I don't
> > think the bit of loss in the option descriptions is _that_ big a
> > deal, the subcommands are longer and could benefit from the
> > formatting more. I could probably fix it in Sphinx internally for
> > Sphinx 1.6, but that'd take a year or two to propagate to distros.
> 
> I don't understand the issue yet.  What happens if we just use RST
> that produces the desired formatting?  (Is this about indexing
> again?)

Two issues. First, take a look at the current ovs-vswitchd doc:

  http://openvswitch.org/support/dist-docs/ovs-vswitchd.8.html

If you look at the 'ssl:ip:port' description, you'll note that 'ssl' is
 in bold, 'ip' and 'port' are underlined, and the colons in between
have no formatting. We can't do this in Sphinx as you need spaces
between inline formatting markers.

Secondly, look at an example of the 'option' directive, e.g.:

   .. option:: -V, --version

       Prints version information to the console.

This can be viewed like so:

   .. <directive_name>:: <directive arguments>

      <directive contents>

While '<directive contents>' is parsed as reStructuredText, '<directive
arguments>' is not. This means if we include things like '*' (italics),
'**' (bold), or '``' (code/literal), they'll be rendered as they are.

IMO, the option directive provides enough usefulness to make this
worthwhile, but I called it out just in case.

> > - I'm not sure if there's a way to use this kind of macro:
> > 
> >     \*(DX\fBadd\-dp \fIdp\fR [\fInetdev\fR[\fB,\fIoption\fR]...]
> > 
> >   which is called like so:
> > 
> >     .ds DX \fBdpctl/\fR
> >     .de DO
> >     \\$2 \\$1 \\$3
> > 
> >   and yields:
> > 
> >     dpctl/add-dp dp [netdev[,option]...]
> > 
> >   This will probably result in some duplication, else a _lot_ of
> > 'inc'
> >   files
> 
> We could always just document these commands in only one place, e.g.
> the ovs-dpctl manpage, and then refer to that manpage from ovs-
> vswitchd manpage with a little extra explanation, e.g. "You can use
> ovs-dpctl commands via ovs-appctl: instead of 'ovs-dpctl show', type
> 'ovs-appctl dpctl/show'."

I'd thought about duplicating the info instead. However, if you're
happy to simply reference things, then this is good by me.

> > - I'm not sure how we should pass in autoconf (?) variables like
> >   @RUNDIR at . Could we do this in Python or something?
> 
> I haven't done the right research on this yet.

I guess we could just start statically defining these, with a statement
that things may change depending on your installation. If not, I'll
think of something :)

> > - Some other stuff that I've forgotten
> > 
> > In any case though, it should go folks a rough idea of what a
> > larger man page in rST/Sphinx will actually look like.
> 
> I notice that in PDF builds, long lines in examples get cut off at
> the right margin instead of line-wrapping, which is unfortunate.

Do we care about the PDF builds? I'd considered disabling those,
personally. If we do, then I'm sure there'll be something on
StackOverflow about that exact problem, heh.

> This needs the following to avoid a build error:
> 
> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> index 3c75d906afec..a7109f8e2f79 100644
> --- a/Documentation/automake.mk
> +++ b/Documentation/automake.mk
> @@ -91,6 +91,23 @@ DOC_SOURCE = \
>  	Documentation/internals/contributing/documentation-style.rst 
> \
>  	Documentation/internals/contributing/libopenvswitch-abi.rst
> \
>  	Documentation/internals/contributing/submitting-patches.rst
> \
> +	Documentation/ref/common.inc \
> +	Documentation/ref/coverage-unixctl.inc \
> +	Documentation/ref/daemon.inc \
> +	Documentation/ref/dpctl.inc \
> +	Documentation/ref/memory-unixctl.inc \
> +	Documentation/ref/ofproto-dpif-unixctl.inc \
> +	Documentation/ref/ofproto-tnl-unixctl.inc \
> +	Documentation/ref/ofproto-unixctl.inc \
> +	Documentation/ref/ovs-vswitchd.8.rst \
> +	Documentation/ref/remote-active.inc \
> +	Documentation/ref/remote-passive.inc \
> +	Documentation/ref/service.inc \
> +	Documentation/ref/ssl-bootstrap.inc \
> +	Documentation/ref/ssl.inc \
> +	Documentation/ref/unixctl.inc \
> +	Documentation/ref/vlog-unixctl.inc \
> +	Documentation/ref/vlog.inc \
>  	Documentation/requirements.txt \
>  	$(addprefix Documentation/ref/,$(RST_MANPAGES))
>  FLAKE8_PYFILES += Documentation/conf.py

Ta. FWIW, I'd just been building this manually with 'sphinx-build'
(which is how I missed the flake8 issue).

Thanks for the review, Ben,

Stephen