oxidecomputer / helios

Helios: Or, a Vision in a Dream. A Fragment.
Mozilla Public License 2.0
371 stars 10 forks source link

tools: accept "help" top-level command, show usage on errors #174

Closed iximeow closed 2 months ago

iximeow commented 3 months ago

helios-build has supported --help since forever, but as @rmustacc found in #12 and i rediscovered yesterday, not -h nor a top-level "help" command. seems like it'd be good to accept all of --help, -h, helios-build help, or the bare ./helios-build to get usage information, so this change does that.

also, because it's in baseopts, this means -h is now a flag for all the subcommands too.

jclulow commented 3 months ago

I don't necessarily want to throw the h short option away for --help. I think it's far less consistently available than --help, and sometimes it makes sense to use it to align with use of -h for "human readable" or "host name" or whatever else.

I agree that a top-level help command is valuable, though!

jclulow commented 3 months ago

I have pushed some changes to the branch:

iximeow commented 3 months ago

ah! good changes.

you prompted me to check in a few places about -h and i realized that beadm, dladm, and generally most tools i've "successfully" used with -h have actually been interpreting -h as a subcommand (or argument) which is not known, then calling usage() to tell the user what they can do. for helios-build, we'd do similar with a change like:

         println!("{}", opts.usage(&out));
     };

-    let res = opts.parse(std::env::args_os().skip(1))?;
+    let res = match opts.parse(std::env::args_os().skip(1)) {
+      Ok(res) => res,
+      Err(e) => {
+        eprintln!("Error: {}", e);
+        usage();
+
+        // If we bail!() out here, we'll get another `Error: ...` printed to stderr
+        // by Rust. We've done all the reporting we want though, so just exit(1)
+        // now.
+        std::process::exit(1);
+      }
+    };
+
     if res.opt_present("help") {
         usage();
         return Ok(());

that should guide users a bit better than the status quo of

./helios-build -h
Error: Unrecognized option: 'h'

while not necessarily eating -h as a flag here or later subcommands. what do you think?

jclulow commented 2 months ago

I think that's reasonable, but I would print the usage first and then just bail in the usual way with the error. Sometimes the usage information can be more than a an entire screen vertically, and it's super irritating to be hit in the face with it unexpectedly and then to have to go and hunt for what you did wrong! If we put the error last, at least it's right there where your eyes are with the next shell prompt.

 $ ./helios-build -h
Usage: helios [OPTIONS] COMMAND [OPTIONS] [ARGS...]

    setup            clone required repositories and run setup tasks

    genenv           generate environment file for illumos build

    bldenv           enter a bldenv shell for illumos so you can run dmake
    onu              install your non-DEBUG build of illumos on this system
    build-illumos    run a full nightly(1) and produce packages
    merge-illumos    merge DEBUG and non-DEBUG packages into one repository
    experiment-image experimental image construction for Gimlets

    help             display usage information

Options:
    --help              display usage information

Error: Unrecognized option: 'h'

I've pushed a change to make it look like the above, and to ensure that the usage text ends up on stderr in a failure case, or stdout (for grep, etc) if you explicitly requested it.