Upgrade to cmdliner.1.1.1 and improve clarity of manuals

[hernoufM]

• Kind: enhancement

Description

This PR adds support for newest cmdliner version to learn-ocaml.
The new version has added support for UTF-8 encoded characters within man pages.
At the same time, a new module Cmd has been added, whose functionalities will replace deprecated
Term.eval* evaluation functions and Term.info.
To see more : https://github.com/dbuenzli/cmdliner/blob/master/CHANGES.md.

Additionally, this PR contains changes that:

• Moves server's arguments description within man page to the related section.
• Regroups standard and custom options together (either in OPTIONS or in COMMON OPTIONS)
• Complete documentation about exit codes for every command and nested sub-commands.
• Some minor changes that improve clarity and organization within manual pages.

Checklist

• Merge cmdliner sections OPTIONS → COMMON OPTIONS ?
• Check/Fix cmdliner exits semantics (that has to be preserved) and documentation
• Fix learn-ocaml-server --help title
• Bump cmdliner to "1.1.1"

[AltGr]

Looks good, thanks!
We probably need to add the constraints to the .opam files too, I guess the change is not backwards-compatible with older versions of cmdliner: something like "cmdliner" {>= "1.1.0"}

[hernoufM]

Thank you for your comment! It is done :-)

[erikmd]

Thanks @hernoufM 👍

I just skimmed the PR, but here is a very first nitpick-feedback: could you rebase -i your commits so the first line of commit messages fully follows conventional commits? (a colon is required, while the final dot is unneeded)

i.e. deps(opam) Upgrade cmdliner dependency to 1.1.0 version.
→ deps(opam): Upgrade cmdliner dependency to 1.1.0 version

[hernoufM]

Ah, sorry! I'll fix it up.

[erikmd]

@hernoufM Thanks a lot for your PR.

I guess this also solves a long-standing minor issue I had spotted with learn-ocaml man pages, namely:

$ learn-ocaml --version
0.14.0
$ learn-ocaml --help >/dev/null
troff: <standard input>:146: warning: can't find character with input code 195
troff: <standard input>:146: warning: can't find character with input code 196
troff: <standard input>:146: warning: can't find character with input code 197
troff: <standard input>:146: warning: can't find character with input code 169

FTR, I did a full diff of {learn-ocaml --help, learn-ocaml-client --help, learn-ocaml-server --help} (see below) and I have only one question:

Do you believe it is feasible to merge the three OPTIONS sections into the so-called COMMON OPTIONS sections?

otherwise, the PR can be merged as is…

Diff (details)

learn-ocaml

--- learn-ocaml_0.14.0.txt	2022-04-05 01:14:30.946964015 +0200
+++ learn-ocaml_pr_480.txt	2022-04-05 01:14:34.986923752 +0200
@@ -3,7 +3,8 @@
        learn-ocaml - Learn-ocaml web-app manager
 
 SYNOPSIS
-       learn-ocaml [OPTION]... [COMMAND]...
+       learn-ocaml [--base-url=BASE_URL] [--app-dir=DIR] [--repo=DIR]
+       [OPTION]… [COMMAND]…
 
 DESCRIPTION
        This program performs various tasks related to generating, serving and
@@ …,… …,… @@
 OPTIONS
        --base-url=BASE_URL (absent LEARNOCAML_BASE_URL env)
            Set the base URL of the website. Should not end with a trailing
            slash. Currently, this has no effect on the backend - 'learn-ocaml
            serve'. Mandatory for 'learn-ocaml build' if the site is not
            hosted in path '/', which typically occurs for static deployment.
 
-       --cert=BASENAME (absent LEARNOCAML_CERT env)
-           HTTPS certificate: this option turns on HTTPS, and requires files
-           BASENAME.pem and BASENAME.key to be present. They will be used as
-           the server certificate and key, respectively. A passphrase may be
-           asked on the terminal if the key file is encrypted.
-
-       --help[=FMT] (default=auto)
-           Show this help in format FMT. The value FMT must be one of `auto',
-           `pager', `groff' or `plain'. With `auto', the format is `pager` or
-           `plain' whenever the TERM env var is `dumb' or undefined.
-
        -o DIR, --app-dir=DIR (absent=./www)
            Directory where the app should be generated for the build command,
            and from where it is served by the serve command.
 
-       -p PORT, --port=PORT (absent LEARNOCAML_PORT env)
-           The TCP port on which to run the server. Defaults to 8080, or 8443
-           if HTTPS is enabled.
-
        --repo=DIR (absent=.)
            The path to the repository containing the exercises, lessons and
            tutorials.
 
-       --sync-dir=DIR (absent=./sync)
-           Directory where to store user sync tokens
-
-       --version
-           Show version information.
-
 GRADER OPTIONS
        --display-outcomes
            display the toplevel's outcomes
@@ -98,7 +78,7 @@
            Display detailed grading reports to stdout
 
 BUILDER OPTIONS
        --contents-dir=DIR (absent=/usr/share/learn-ocaml/www)
            directory containing the base learn-ocaml app contents
 
        --disable-exercises
@@ -147,10 +128,43 @@
            Number of building jobs to run in parallel
 
 SERVER OPTIONS
+       --cert=BASENAME (absent LEARNOCAML_CERT env)
+           HTTPS certificate: this option turns on HTTPS, and requires files
+           BASENAME.pem and BASENAME.key to be present. They will be used as
+           the server certificate and key, respectively. A passphrase may be
+           asked on the terminal if the key file is encrypted.
+
+       -p PORT, --port=PORT (absent LEARNOCAML_PORT env)
+           The TCP port on which to run the server. Defaults to 8080, or 8443
+           if HTTPS is enabled.
+
+       --sync-dir=DIR (absent=./sync)
+           Directory where to store user sync tokens
+
 REPOSITORY FORMAT
        The repository specified by --repo is expected to contain
        sub-directories lessons, tutorials, playground and exercises.
 
+COMMON OPTIONS
+       --help[=FMT] (default=auto)
+           Show this help in format FMT. The value FMT must be one of auto,
+           pager, groff or plain. With auto, the format is pager or plain
+           whenever the TERM env var is dumb or undefined.
+
+       --version
+           Show version information.
+
+EXIT STATUS
+       learn-ocaml exits with the following status:
+
+       0   on success.
+
+       123 on indiscriminate errors reported on standard error.
+
+       124 on command line parsing errors.
+
+       125 on unexpected internal errors (bugs).
+
 ENVIRONMENT
        These environment variables affect the execution of learn-ocaml:

learn-ocaml-client

@@ -177,42 +191,44 @@
        learn-ocaml-client - Learn-ocaml grading client.
 
 SYNOPSIS
-       learn-ocaml-client COMMAND ...
+       learn-ocaml-client [COMMAND] …
 
 DESCRIPTION
        Learn-ocaml-client, default command is grade.
 
 COMMANDS
-       create-token
+       create-token [--server=URL] [OPTION]… [NICKNAME] [SECRET]
            Create a token.
 
-       exercise-list
+       exercise-list [--local] [--server=URL] [--token=TOKEN] [OPTION]…
            Get a structured json containing a list of the exercises of the
            server
 
-       fetch
+       fetch [--local] [--server=URL] [--token=TOKEN] [OPTION]…
+       [EXERCISE_ID]…
            Fetch the user's solutions.
 
-       grade
+       grade [OPTION]… [FILE]
            Learn-ocaml grading client.
 
-       init
+       init [--local] [--server=URL] [--token=TOKEN] [OPTION]… [NICKNAME]
+       [SECRET]
            Initialize the configuration file.
 
-       print-server
+       print-server [--local] [--server=URL] [--token=TOKEN] [OPTION]…
            Just print the configured server.
 
-       print-token
+       print-token [--local] [--server=URL] [--token=TOKEN] [OPTION]…
            Just print the configured user token.
 
-       server-version
+       server-version [--local] [--min] [--server=URL] [OPTION]…
            Print the version of the server (from CLI or from the cookie file,
            which is kept untouched anyway).
 
-       set-options
+       set-options [--local] [--server=URL] [--token=TOKEN] [OPTION]…
            Set configuration.
 
-       template
+       template [--local] [--server=URL] [--token=TOKEN] [OPTION]… [ID]
            Get the template of a given exercise.
 
 ARGUMENTS
@@ -222,16 +238,11 @@
 OPTIONS
        --color=WHEN (absent=auto)
            Colorise the output, and also allows use of UTF-8 characters. WHEN
-           must be one of `always', `never' or `auto'.
+           must be one of 'always', 'never' or 'auto'.
 
        --console
            output the results to the console. This is the default.
 
-       --help[=FMT] (default=auto)
-           Show this help in format FMT. The value FMT must be one of `auto',
-           `pager', `groff' or `plain'. With `auto', the format is `pager` or
-           `plain' whenever the TERM env var is `dumb' or undefined.
-
        --html
            output the results as HTML, to the console.
 
@@ -262,9 +273,26 @@
        -v, --verbose
            Be more verbose. Can be repeated.
 
+COMMON OPTIONS
+       --help[=FMT] (default=auto)
+           Show this help in format FMT. The value FMT must be one of auto,
+           pager, groff or plain. With auto, the format is pager or plain
+           whenever the TERM env var is dumb or undefined.
+
        --version
            Show version information.
 
+EXIT STATUS
+       learn-ocaml-client exits with the following status:
+
+       0   on success.
+
+       123 on indiscriminate errors reported on standard error.
+
+       124 on command line parsing errors.
+
+       125 on unexpected internal errors (bugs).
+
 ENVIRONMENT
        These environment variables affect the execution of
        learn-ocaml-client:

learn-ocaml-server

@@ -291,7 +319,7 @@
        learn-ocaml - Learn-ocaml web-app manager
 
 SYNOPSIS
-       learn-ocaml [OPTION]... 
+       learn-ocaml [--base-url=BASE_URL] [--app-dir=DIR] [OPTION]…
 
 DESCRIPTION
        This is the server for learn-ocaml. It is equivalent to running
@@ -300,28 +328,12 @@
        build beforehand.
 
 SERVER OPTIONS
-OPTIONS
-       --base-url=BASE_URL (absent LEARNOCAML_BASE_URL env)
-           Set the base URL of the website. Should not end with a trailing
-           slash. Currently, this has no effect on the backend - 'learn-ocaml
-           serve'. Mandatory for 'learn-ocaml build' if the site is not
-           hosted in path '/', which typically occurs for static deployment.
-
        --cert=BASENAME (absent LEARNOCAML_CERT env)
            HTTPS certificate: this option turns on HTTPS, and requires files
            BASENAME.pem and BASENAME.key to be present. They will be used as
            the server certificate and key, respectively. A passphrase may be
            asked on the terminal if the key file is encrypted.
 
-       --help[=FMT] (default=auto)
-           Show this help in format FMT. The value FMT must be one of `auto',
-           `pager', `groff' or `plain'. With `auto', the format is `pager` or
-           `plain' whenever the TERM env var is `dumb' or undefined.
-
-       -o DIR, --app-dir=DIR (absent=./www)
-           Directory where the app has been generated by the learn-ocaml
-           build command, and from where it will be served.
-
        -p PORT, --port=PORT (absent LEARNOCAML_PORT env)
            The TCP port on which to run the server. Defaults to 8080, or 8443
            if HTTPS is enabled.
@@ -329,9 +341,37 @@
        --sync-dir=DIR (absent=./sync)
            Directory where to store user sync tokens
 
+OPTIONS
+       --base-url=BASE_URL (absent LEARNOCAML_BASE_URL env)
+           Set the base URL of the website. Should not end with a trailing
+           slash. Currently, this has no effect on the backend - 'learn-ocaml
+           serve'. Mandatory for 'learn-ocaml build' if the site is not
+           hosted in path '/', which typically occurs for static deployment.
+
+       -o DIR, --app-dir=DIR (absent=./www)
+           Directory where the app has been generated by the learn-ocaml
+           build command, and from where it will be served.
+
+COMMON OPTIONS
+       --help[=FMT] (default=auto)
+           Show this help in format FMT. The value FMT must be one of auto,
+           pager, groff or plain. With auto, the format is pager or plain
+           whenever the TERM env var is dumb or undefined.
+
        --version
            Show version information.
 
+EXIT STATUS
+       learn-ocaml exits with the following status:
+
+       0   on success.
+
+       123 on indiscriminate errors reported on standard error.
+
+       124 on command line parsing errors.
+
+       125 on unexpected internal errors (bugs).
+
 ENVIRONMENT
        These environment variables affect the execution of learn-ocaml:

[erikmd]

Beyond my minor comment on sections-merging,

I believe there's an issue regarding the documentation (and maybe semantics?) of exit status:

shouldn't this code for learn-ocaml-client:

    let open Cmd.Exit in
    [ info ~doc:"Default exit." ok
    ; info ~doc:"Unable to reach the server." 1
    ; info ~doc:"Input error: unable to find a server URL." 2
    ; info ~doc:"The client's version is incompatible (too old?) w.r.t. the server." 70
    ]

be part of the learn-ocaml-client --help output somehow?

[erikmd]

As an aside, you might want to tweak the cmdliner code for learn-ocaml-server --help within this PR:

so it starts with learn-ocaml-server - Learn-ocaml native server, NOT learn-ocaml - Learn-ocaml web-app manager…

[hernoufM]

@erikmd Thank for your detailed review!

I guess this also solves a long-standing minor issue I had spotted with learn-ocaml man pages

I've also encountered this by testing the previous version. Hopefully it was fixed.

Do you believe it is feasible to merge the three OPTIONS sections into the so-called COMMON OPTIONS sections?

Yes, of course! It could be done by specifying explicitly section's name with ~docs argument for each option. Do you think it is possible to specify server options in OPTIONS section since learn-ocaml-server doesn't possess nested commands?

Beyond my minor comment on sections-merging,

I believe there's an issue regarding the documentation (and maybe semantics?) of exit status:

shouldn't this code for learn-ocaml-client:

    let open Cmd.Exit in
    [ info ~doc:"Default exit." ok
    ; info ~doc:"Unable to reach the server." 1
    ; info ~doc:"Input error: unable to find a server URL." 2
    ; info ~doc:"The client's version is incompatible (too old?) w.r.t. the server." 70
    ]

be part of the learn-ocaml-client --help output somehow?

In fact, at the moment, the program cannot be executed without specifying one of the commands. Actually, it could, but, it is launched as being larn-ocaml-client grade. It's a reason why OPTIONS, EXIT STATUS, ENVIRONMENT and ARGUMENTS sections list items specific to grade command. This problem (including the one with exit codes), in my opinion, can be solved in 3 ways:

• The perfect solution is to act in the same way as opam does. More specifically, it means that learn-ocam-client will just print a message with correct usage and learn-ocaml-client --help will list only common options and arguments . On the other hand, environment variables and exit codes sections could enumerate every existing variable/exit code, provided they will be generalized (that could be a very interesting task to add on the checklist). The problem is that, probably, there are some script files somewhere that are already configured to use learn-ocaml-client as being grade.
• Another one, is to mark this command as deprecated (introduced in cmdliner.1.1.0) without changing default redirection to grade.
• List every option (and other items) for every command, but pass only subset of information to grade function.

I'd like to know your opinion on this point :)

[erikmd]

Hi @hernoufM

I guess this also solves a long-standing minor issue I had spotted with learn-ocaml man pages

I've also encountered this by testing the previous version. Hopefully it was fixed.

Yes!

Do you believe it is feasible to merge the three OPTIONS sections into the so-called COMMON OPTIONS sections?

Yes, of course! It could be done by specifying explicitly section's name with ~docs argument for each option.

OK.

Do you think it is possible to specify server options in OPTIONS section since learn-ocaml-server doesn't possess nested commands?

Sure. But I'd go once step further: for learn-ocaml-server, I'd put --help and --version in OPTIONS as well, using
Term.info's parameter sdocs: defines the title of the section in which the standard --help and --version arguments are listed

Beyond my minor comment on sections-merging,
I believe there's an issue regarding the documentation (and maybe semantics?) of exit status:
shouldn't this code for learn-ocaml-client:

    let open Cmd.Exit in
    [ info ~doc:"Default exit." ok
    ; info ~doc:"Unable to reach the server." 1
    ; info ~doc:"Input error: unable to find a server URL." 2
    ; info ~doc:"The client's version is incompatible (too old?) w.r.t. the server." 70
    ]

be part of the learn-ocaml-client --help output somehow?

In fact, at the moment, the program cannot be executed without specifying one of the commands. Actually, it could, but, it is launched as being larn-ocaml-client grade. It's a reason why OPTIONS, EXIT STATUS, ENVIRONMENT and ARGUMENTS sections list items specific to grade command. This problem (including the one with exit codes), in my opinion, can be solved in 3 ways:

• The perfect solution is to act in the same way as opam does. More specifically, it means that learn-ocam-client will just print a message with correct usage and learn-ocaml-client --help will list only common options and arguments . On the other hand, environment variables and exit codes sections could enumerate every existing variable/exit code, provided they will be generalized (that could be a very interesting task to add on the checklist).

What do you mean by "be generalized"?

If you think it's feasible to document at least a bit more the exit codes within this PR, please take a look… otherwise we'll do this in another PR. But anyway, it would be nice to tackle this now, because as shown in my "--help diff", the version bump to 1.1.0 has the side-effect of showing more exit codes… but not all that are interesting :-/

The problem is that, probably, there are some script files somewhere that are already configured to use learn-ocaml-client as being grade.

Indeed. Backward compatibility is important.
(Except possibly when bumping learn-ocaml's version to 1.0.0… which may happen soon actually: when #481 is merged :-)

• Another one, is to mark this command as deprecated (introduced in cmdliner.1.1.0) without changing default redirection to grade.

That might look weird because in general, we deprecate a subcommand that will be removed, not the "root" command :'-)

So I'd suggest to do none of these two options in this PR (as removing some default/implied subcommand just because the cmdliner wrapper code would be more sensible… looks like a wrong reason for changing the CLI syntax :)

Anyway, I'd be happy to discuss this with you anew after the merge of this PR! e.g. at the time the learn-ocaml.1.0.0 release is pending.

• List every option (and other items) for every command, but pass only subset of information to grade function.

IIUC your suggestion, this idea means merging all options from all subcommands in the --help output, then "wiring" them to the good sub-function? I'm not sure it would improve the readability… so I hope this change is not necessary from your viewpoint.

Kind regards,
Erik

[hernoufM]

@erikmd Hello!
In accordance with the points that you mentioned above, I've fixed every point in checklist. I would like to draw your attention to some details:
Firstly, to inform users I've added a short section in learn-ocaml-client --help that indicates that all items listed in the man (except for COMMANDS, DESCRIPTION and SYNOPSIS sections) are relative to grade command :

GRADE
       From now on, this manual describes the specification of the default
       grade command, that could also be retrieved by 'learn-ocaml-client
       grade --help'.

Secondly, I've reviewed every command/subcommand in order to record and document exit codes that are now displayed differently for everyone of them.

What do you mean by "be generalized"?

I meant, that there are some commands that uses different exit code for similar issues. For example, every command exits with 1 when config file wasn't found but server_version does it with 2. But anyway, I guess that it's not a critical point :)

To conclude, i guess that it will be better to rewrite title and description for this PR (something like "Refactoring and improving clarity for manuals").

Sincerely,
Mohamed

[erikmd]

Thanks @hernoufM for your thorough refactoring and debrief ! I'll have another look ASAP.

[erikmd]

FYI we now get in this GHA run:

https://github.com/ocaml-sf/learn-ocaml/runs/6119796229?check_suite_focus=true

<><> Installing new switch packages <><><><><><><><><><><><><><><><><><><><><><>
Switch invariant: ["ocaml-system"]
Error:  Could not determine which packages to install for this switch:
  * Missing dependency:
    - cmdliner >= 1.1.1
    no matching version


Switch initialisation failed: clean up? ('n' will leave the switch partially installed) [Y/n] y
tar: This does not look like a tar archive
tar: Exiting with failure status due to previous errors
Error: Process completed with exit code 2.

and this does not seems like a spurious failure (I restarted the run to no avail)

@AltGr @hernoufM Do you think this blocking issue will be automatically solved at the upcoming update of the image ocamlpro/ocaml:4.12, and if yes, when?

[AltGr]

@AltGr @hernoufM Do you think this blocking issue will be automatically solved at the upcoming update of the image ocamlpro/ocaml:4.12, and if yes, when?

It should, yes, I have re-run the image generation on OCamlPro's side

[erikmd]

CI is ✔️ now; thanks @hernoufM and @AltGr!