Generate labgrid-client subcommand and option documentation from argparse

[jonrebm]

Manage the documentation with sphinx and auto-document labgrid-client arguments and subcommands from source using sphinx-argparse.

Sections from the original man/labgrid-client.rst which are not obsoleted by the autogenerated reference are migrated into sphinx. Sphinx generates both the manpages as well as their html documentation counterpart.

I believe the result of this is much more useful than the current manpages we have that just don't sufficiently document usage of the subcommands.

See the readthedocs builds for results.

[codecov]

Codecov Report

❌ Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.6%. Comparing base (e380181) to head (d2f2131).
⚠️ Report is 35 commits behind head on master.

Files with missing lines	Patch %	Lines
labgrid/remote/client.py	94.1%	2 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #1707   +/-   ##
======================================
  Coverage    55.6%   55.6%           
======================================
  Files         172     172           
  Lines       13471   13477    +6     
======================================
+ Hits         7500    7506    +6     
  Misses       5971    5971           

Flag	Coverage Δ
3.10	55.6% <94.1%> (+<0.1%)	⬆️
3.11	55.6% <94.1%> (+<0.1%)	⬆️
3.12	55.6% <94.1%> (+<0.1%)	⬆️
3.13	55.6% <94.1%> (+<0.1%)	⬆️
3.9	55.7% <94.1%> (+<0.1%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Bastian-Krause]

Can we get rid of the "Default: " lines? Most of them are either misleading (expose internal implementation details) or redundant. Ideally the defaults should be part of the help strings where useful.

Here's what I mean:

labgrid-client
Labgrid is a scalable infrastructure and test architecture for embedded (linux) systems.

This is the client to control a boards status and interface with it on remote machines.

usage: labgrid-client [-h] [-x ADDRESS] [-c CONFIG] [-p PLACE] [-s STATE]
[-i INITIAL_STATE] [-d] [-v] [-P PROXY]
COMMAND ...
Named Arguments
-x, --coordinator
coordinator HOST[:PORT] (default: value from env variable LG_COORDINATOR, otherwise 127.0.0.1:20408)

-c, --config
env config file (default: value from env variable LG_ENV)

-p, --place
place name/alias (default: value from env variable LG_PLACE)

-s, --state
strategy state to switch into before command (default: value from env varibale LG_STATE)

-i, --initial-state
strategy state to force into before switching to desired state

-d, --debug
enable debug mode (show python tracebacks)

Default: False

Here.

-v, --verbose
Default: 0

Here.

-P, --proxy
proxy connections via given ssh host

Sub-commands
monitor
monitor events from the coordinator

labgrid-client monitor [-h]
resources (r)
list available resources

labgrid-client resources [-h] [-a] [-e EXPORTER]
[--sort-by-matched-place-change]
[match]
Positional Arguments
match
Named Arguments
-a, --acquired
Default: False

Here.

-e, --exporter
--sort-by-matched-place-change
sort by matched place’s changed date (oldest first) and show place and date

Default: False

Here.

places (p)
list available places

labgrid-client places [-h] [-a] [-r] [--sort-last-changed]
Named Arguments
-a, --acquired
Default: False

Here.

-r, --released
Default: False

Here.

--sort-last-changed
sort by last changed date (oldest first)

Default: False

Here.

who
list acquired places by user

labgrid-client who [-h] [-e]
Named Arguments
-e, --show-exporters
show exporters currently used by each place

Default: False

Here.

show
show a place and related resources

[...]

labgrid-client acquire [-h] [--allow-unmatched]
Named Arguments
--allow-unmatched
allow missing resources for matches when locking the place

Default: False

Here.

release (unlock)
release a place

labgrid-client release [-h] [-k]
Named Arguments
-k, --kick
release a place even if it is acquired by a different user

Default: False

Here.

[...]

labgrid-client console [-h] [-l] [-o] [--logfile FILE] [name]
Positional Arguments
name
optional resource name

Named Arguments
-l, --loop
keep trying to connect if the console is unavailable

Default: False

Here.

-o, --listenonly
do not modify local terminal, do not send input from stdin

Default: False

Here.

[...]

labgrid-client forward [-h] [--name NAME] [--local [LOCAL:]REMOTE]
[--remote REMOTE:LOCAL]
Named Arguments
--name, -n
optional resource name

--local, -L
Forward local port LOCAL to remote port REMOTE. If LOCAL is unspecified, an arbitrary port will be chosen

Default: []

Here.

--remote, -R
Forward remote port REMOTE to local port LOCAL

Default: []

Here.

[...]

write-files
copy files onto mass storage device

labgrid-client write-files [OPTION]... -T SOURCE DEST
labgrid-client write-files [OPTION]... [-t DIRECTORY] SOURCE...
Positional Arguments
SOURCE
source file(s) to copy

DEST
destination file name for SOURCE

Named Arguments
-w, --wait
storage poll timeout in seconds

Default: 10.0

-p, --partition
partition number to mount or 0 to mount whole disk (default: 1)

Default: 1

Redundant.

-t, --target-directory
copy all SOURCE files into DIRECTORY (default: partition root)

Default: /

Redundant.

-T
copy SOURCE file and rename to DEST

Default: False

Here.

--name, -n
optional resource name

write-image
write an image onto mass storage

labgrid-client write-image [-h] [-w WAIT] [-p PARTITION] [--skip SKIP]
[--seek SEEK] [--mode {dd,bmaptool}] [--name NAME]
filename
Positional Arguments
filename
filename to boot on the target

Named Arguments
-w, --wait
Default: 10.0

-p, --partition
partition number to write to

--skip
skip n 512-sized blocks at start of input

Default: 0

Here.

--seek
skip n 512-sized blocks at start of output

Default: 0

Here.

--mode
Possible choices: dd, bmaptool

Choose tool for writing images (default: dd)

Default: dd

Redundant.

--name, -n
optional resource name

reserve
create a reservation

labgrid-client reserve [-h] [--wait] [--shell] [--prio PRIO]
KEY=VALUE [KEY=VALUE ...]
Positional Arguments
KEY=VALUE
required tags

Named Arguments
--wait
wait until the reservation is allocated

Default: False

Here.

--shell
format output as shell variables

Default: False

Here.

--prio
priority relative to other reservations (default 0)

Default: 0.0

Redundant.

[...]

labgrid-client export [-h] [--format {shell,shell-export,json}] filename
Positional Arguments
filename
output filename

Named Arguments
--format
Possible choices: shell, shell-export, json

output format (default: shell-export)

Default: shell-export

Redundant.

version
show version

labgrid-client version [-h]

[Bastian-Krause]

One more observation: In "Named Arguments" there is no distinction between flags (e.g. -v, --verbose) and options expecting a value (e.g. -x, --coordinator). The usage contains these information. Is there an easy way of adding that?

[jonrebm]

@Bastian-Krause sadly, no. Generally I prefer the manpage format of autoprogram over that of sphinx-argparse, as it is more compact, idiomatic and does show option parameters:

LABGRID-CLIENT
usage: labgrid-client [-h] [-x ADDRESS] [-c CONFIG] [-p PLACE] [-s STATE] [-i INITIAL_STATE] [-d] [-v] [-P PROXY] COMMAND ...

-h, --help
        show this help message and exit

-x <address>, --coordinator <address>
        coordinator HOST[:PORT] (default: value from env variable LG_COORDINATOR, otherwise 127.0.0.1:20408)

We opted for sphinx-argparse for now in order to be able to replace subcommand descriptions. Maybe we keep an eye on progress of both alternatives.

That said, shpinx-argparse is fairly simple and it would be easy for us to add missing functionality at a later time if necessary.

[Bastian-Krause]

The overall idea of this PR seems to be to have the man pages in sync with the actual commands, document sub-commands and have them as readable as possible. I really like the idea. The key benefit over the current man pages and help text output is that we would have everything in a single document without the need of checking each sub-command's help text.

But there are some points that make me hesitant to approve this as is:

• I think the missing distinction between options and flags makes the man pages a lot less understandable. The help texts do not give enough hints to tell them apart, either. I think this is a clear change for the worse compared to before. I don't see a good way forward without spending more effort here.
• I don't understand why the generated man pages are no longer tracked in git. How are they supposed to be generated by a distro package (e.g. dh_virtualenv)? This is important since these packages will be the primary user of the man pages anyway (people using pip install will probably look at the HTML version).

Minor things:

• The "SUB-COMMANDS" section needs to have the positional/named arguments further indented. The sub-command, "Positional Arguments" and "Named Arguments" are all on the same indentation level. This makes the section unreadable.
• Environment variables in the man page of labgrid-exporter need to be indented.
• Repeating the whole command for sub-commands that take only -h is not really helpful, e.g. labgrid-client show [-h].
• The command repitition of labgrid-client write-files looks strange.
• I'm not sure we should ship the man page of labgrid-pytest as it currently is. The command exists only for the Debian integration. Seems confusing to me. It basically describes a single pytest CLI option introduced by the labgrid pytest plugin.

[jonrebm]

The overall idea of this PR seems to be to have the man pages in sync with the actual commands, document sub-commands and have them as readable as possible. I really like the idea. The key benefit over the current man pages and help text output is that we would have everything in a single document without the need of checking each sub-command's help text.

Yes although for me, the main motivation was to document the subcommands options and flags in the manpage and online manual. Currently we have no documentation on them other than each subcommands --help message which is really only accessible of you have labgrid installed. I have a very strong opinion that these need to be in the manpage and manual, and I hope we can agree on that.
@jluebbe rightly criticized the increased maintenance burden if we where to manually document them in the existing rst documentation therefore the decision to stop redundantly maintaining argparse docs and rst altogether and refer to the argparse as the single source of truth.

I think the missing distinction between options and flags makes the man pages a lot less understandable. The help texts do not give enough hints to tell them apart, either. I think this is a clear change for the worse compared to before. I don't see a good way forward without spending more effort here.

This is documented in the short-usage line of each command e.g.: labgrid-client dfu [-h] [--wait WAIT] [--name NAME] {download,detach,list} [altsetting] [filename]

I agree that it would be nice if, sphinx-argparse would include arguments for options in the named arguments list too. I believe the alternative, sphinxcontrib.autoprogram, does it the way you would want it:

Screenshot of manpages (sphinx-argparse left, autoprogram right)

We opted for sphinx-argparse because it lets us add extra rst-formatted details to specific parts of the generated doc. But both allow us to add sections before and after the options and command lists. It would not be hard to switch this pr to sphinxcontrib.autoprogram instead.

It would not be a lot of effort to make changes to either of the generators, they are quite straightforward, but we may not get those changes merged upstream (I'm still hoping for a reaction on sphinx-doc/sphinx-argparse#70 and sphinx-doc/sphinx-argparse#71).

I think getting the CLI completely documented, even with imperfect formatting, would be a great improvement.

I don't understand why the generated man pages are no longer tracked in git. How are they supposed to be generated by a distro package (e.g. dh_virtualenv)? This is important since these packages will be the primary user of the man pages anyway (people using pip install will probably look at the HTML version).

Ah, simply because I didn't understand why they where tracked in the first place. We don't want the manpage generator as build dependencies for distro packaging, right? I guess we can just track the generated manpages in git like we used to for that matter.

Minor things:

The "SUB-COMMANDS" section needs to have the positional/named arguments further indented. The sub-command, "Positional Arguments" and "Named Arguments" are all on the same indentation level. This makes the section unreadable.

I'd look into this after we discussed which generator we want to use and whether we're open to making changes to that generator

Environment variables in the man page of labgrid-exporter need to be indented.

I only moved that rst without changes, will look into how this happened.

Repeating the whole command for sub-commands that take only -h is not really helpful, e.g. labgrid-client show [-h].

This is how we set it up in argparse. I don't see an issue in showing the -h in either the documentation or the quick help:

$ labgrid-client show -h
usage: labgrid-client show [-h]

options:
  -h, --help  show this help message and exit

The command repitition of labgrid-client write-files looks strange.

I think it is correct and you'll see the same in the quick help:

usage: labgrid-client write-files [OPTION]... -T SOURCE DEST
       labgrid-client write-files [OPTION]... [-t DIRECTORY] SOURCE...

there are two mutually exclusive ways to invoke labgrid-client write-files

I'm not sure we should ship the man page of labgrid-pytest as it currently is. The command exists only for the Debian integration. Seems confusing to me. It basically describes a single pytest CLI option introduced by the labgrid pytest plugin.

All manpages other than labgrid-client's where just touched due to the migration from rst2man to sphinx manpages. Therefore, I would like to leave those unchanged at this point.

[jonrebm]

I just talked to @Bastian-Krause and we both agree autopgrogram looks better and that we can live without @skip and @replace and make those changes in the argparse object instead:

labgrid/doc/man/client.rst

Lines 20 to 41 in d2f2131

	help : @skip
	skip.
	complete : @skip
	skip.
	available subcommands : @skip
	skip.
	create : @replace
	add a new place with the name specified via ``--place`` or the
	``LG_PLACE`` environment variable.
	release-from : @replace
	Atomically release a place, but only if acquired by a specific user.
	Note that this command returns success as long as the specified user no
	longer owns the place, meaning it may be acquired by another user or
	not at all.
	flashscript : @replace
	Run arbitrary script with arguments to flash device

I will open a second pr for the autoprogram variant of this.
His point regarding labgrid-client write-files was the indentation (issue doesn't occur in autoprogram branch)
Regarding labgrid-pytest we used to not include that in the html documentation and the title of it is confusing but we can resolve that separately.