RFC: obuild: refactor handling of commands and options

[ptoscano]

The current system mixes manual command line parsing for global options, and the Arg module for command options, which is not optimal:

• no way to generate the list of global command line options, nor to document them
• the per-command options are used only when parsing, so showing them for help is not possible
• two different ways to parse options

Also, the handling of commands themselves is messy:

• all of them are implemented in the same module, overloading it with different tasks
• their help texts are structured in lists of lines, and with a separate list to maintain
• the names of the commands are hardcoded in a couple of places

To overcome this situation, create a small Cmd module to represent a single command, with all the information stored there: name, options, help texts, and the actual function for it. Split all the commands in own modules named Cmd_<command>, which contains only the command
declaration, its function, and all the helper stuff for it.
Consequently refactor the command line handling: use the dynamic parsing feature of Arg, so the global command line options (which are now converted to Arg) are parsed first, and then the command options are parsed.

The results are various:

• each command is in its own modules, with its options clearly represented, and help texts side with them
• global command line options are parsed properly
• main.ml is very small, way more readable, and will almost require no changes for adding new commands (see drawback below)
• the help command had to be basically rewritten due to the refactoring; OTOH, obuild help <command> works fine now
• obuild --help works fine now (fixes --help doesn't work #136)

The only drawback is that, due to the way obuild is built using obuild, the modules of the commands are not built if nothing references them. As result, create no-op aliases for all of them in main.ml.

This is mostly good, but still marked as RFC for few reasons:

• the indentation of each new module comes from the function it uses (see Standardize on indentation? #36)
• some of the help texts still need to be written (see the various XXX)
• in general it is a medium/big change, so more eyes/testers would be better

[UnixJunkie]

My two cents: don't open modules, this is not Haskell code, use well-chosen module aliases.
Printf is the only module I think should be opened, because its functions are well-known (eprintf, printf, fprintf, sprintf).

But even for printf, a module alias may not hurt:

module Pr = Printf

[ptoscano]

My two cents: don't open modules,

Yes, this is not something I would have done otherwise. As I wrote earlier (see the "drawback" paragraph), this is due to the way obuild builds stuff: considering nothing requires these new modules explicitly, then the dependency detection does not consider them part of the sources, and thus simply does not build them at all.

Unless you have any idea about out to overcome this situation and still let obuild explicitly build modules even not explicitly used, another option could be change the modules to add a function like register, so the let () = in main.ml would start with various Cmd_<foo>.register ().

[UnixJunkie]

Forgive me, but doing this compiles fine:

diff --git a/src/app_utils.ml b/src/app_utils.ml
index 2081591..2f17293 100644
--- a/src/app_utils.ml
+++ b/src/app_utils.ml
@@ -1,18 +1,20 @@
 open Printf
-open Obuild.Helper
-open Obuild.Gconf
-open Obuild
+
+module Helper = Obuild.Helper
+module Gconf = Obuild.Gconf
+module Ob = Obuild
 
 let read_setup () =
-  FindlibConf.load ();
-  let setup = Dist.read_setup () in
+  Ob.FindlibConf.load ();
+  let setup = Ob.Dist.read_setup () in
   (* all_options are restored from setup file *)
-  Configure.set_opts setup;
+  Ob.Configure.set_opts setup;
   setup
 
 let project_read () =
-  try Project.read gconf.strict
-  with exn -> verbose Verbose "exception during project read: %s\n" (Printexc.to_string exn);
+  try Ob.Project.read Gconf.gconf.strict
+  with exn -> Helper.verbose Verbose "exception during project read: %s\n"
+                (Printexc.to_string exn);
     raise exn
 
 let unimplemented () =

Hence, I don't see your point.
Apart from that, it looks like a fine contribution.

[UnixJunkie]

Shameless plug, if you need/want to parse command lines with super terse code, there is this:
https://github.com/UnixJunkie/minicli
It is not meant to help with the generation of documentation, though.

[ptoscano]

Hence, I don't see your point.

I'll try to be more verbose:

1. download my patch
2. remove the various open Cmd_<foo> in src/main.ml
3. build

What happens during the build of obuild using obuilt (either with an existing version, or after bootstrap) is:

• no other module opens or uses the various Cmd_<foo> modules
• the obuild executable in obuild.obuild has main-is: main.ml, and thus main.ml
• obuild resolves the dependency of main.ml using ocamldep: after point (2) above, the output of ocamldep will not include any cmd_*.ml file
• the cmd_*.ml files are not built
• at runtime, none of the commands are available, because their modules were not built, and thus no Cmd.register_cmd happens

[UnixJunkie]

OK. But what I said is that module aliases do the same job as your opens, while the code becomes easier to maintain (because you know where things come from).

[ptoscano]

I see now -- your patch above slightly fooled me, since it was not related to the issue I talked about.

I updated the last commit now with that, thanks for the hint!

[jeromemaloberti]

While I agree with you that the cli code should be distributed in different files, I am not convinced by your approach. Particularly the dynamic registration, I would prefer that the compiler can check everything.
I would also tend to use an existing cli framework (cmdliner) and include it as a 3rd party since the license allow it.
That would make the use of obuild much more standardized without an external dependency.

[ptoscano]

While I agree with you that the cli code should be distributed in different files, I am not convinced by your approach.

Thanks for taking the time to review it.

Particularly the dynamic registration, I would prefer that the compiler can check everything.

I am not sure, what is not checked right now?
The dynamic registration just allows to write new commands as separate modules, just calling a simple API to make the command (with its own arguments) available.

I would also tend to use an existing cli framework (cmdliner) and include it as a 3rd party since the license allow it.

I gave a quick look at it, and it seems pretty big, and possibly way too much for what obuild seems to need.
Also, this refactor does not really block the adoption of any other CLI framework -- it actually makes it easier, since now each command (function, help text, and argumentd) is nicely isolated in its own module.

That would make the use of obuild much more standardized without an external dependency.

As package maintainer in a couple of distros, I recommend to not do this, even if it seems convenient. It is very likely that this copy will not be maintained and rot, and thus be only a maintenance burden (e.g. for compatibility with new OCaml versions).

[jeromemaloberti]

Indeed, before I will decide which framework would be useful, I will need to try to compile with different versions of ocaml, and check the general feeling of the cli.
I am not very worried about code rotting, since obuild just need a small number of features, so I can eventually remove the useless code.

[UnixJunkie]

I have a very small CLI parsing library:
https://github.com/UnixJunkie/minicli
I can relicense it under whatever you need.
The code footprint is super small.
Executables using the library are very terse also. e.g.:
https://github.com/UnixJunkie/minicli/blob/master/test.ml

[ptoscano]

I have a very small CLI parsing library:
https://github.com/UnixJunkie/minicli
I can relicense it under whatever you need.
The code footprint is super small.
Executables using the library are very terse also. e.g.:
https://github.com/UnixJunkie/minicli/blob/master/test.ml

You already mentioned it here :) However:

• it does not support subcommands
• it does not support different arguments for the main command and for the subcommands
• it does not support neither short help string, nor longer help text

I still do think that the code I wrote is still a good compromise between refactoring vs external code embedding.

[UnixJunkie]

@ptoscano Yes, and minicli will never support all that (because of the "mini" in the name, and the fact that I use it for fast software prototyping).
cmdliner is one of the "full featured" alternatives.

[UnixJunkie]

@ptoscano Just quoting your code, for fun:

let args = [
  ("-j", Arg.Int (fun i -> gconf.parallel_jobs <- i), "N maximum number of jobs in parallel");
  ("--jobs", Arg.Int (fun i -> gconf.parallel_jobs <- i), "N maximum number of jobs in parallel");
...

In minicli, you would write this:

let parallel_jobs = CLI.get_int_def ["-j";"--jobs"] args 1 in
...

and you would put some documentation string in the usage function of your program.

[ptoscano]

@ptoscano Yes, and minicli will never support all that (because of the "mini" in the name, and the fact that I use it for fast software prototyping).

Then it cannot be used for obuild, as it needs to have global options and per-command options. And that's why ...

@ptoscano Just quoting your code, for fun:

let args = [
  ("-j", Arg.Int (fun i -> gconf.parallel_jobs <- i), "N maximum number of jobs in parallel");
  ("--jobs", Arg.Int (fun i -> gconf.parallel_jobs <- i), "N maximum number of jobs in parallel");
...

In minicli, you would write this:

let parallel_jobs = CLI.get_int_def ["-j";"--jobs"] args 1 in
...

and you would put some documentation string in the usage function of your program.

... your example to prove that minicli is better than what I wrote does not work. Your example will make -j as global option, not only specific for the "build" subcommand. Even more so, it does not help with documentation, which is a current problem in obuild (single & incomplete Help module, not in sync with the actual commands, and so on).

Since you are in mood of "just for fun", what about a) trying my code b) acknowledge that your code simply is not enough for obuild? Thanks.