Switch subcommand parsing to an enum Subcommand

[mkeeter]

(staged on #661)

tl;dr

This PR adds an enum Subcommand containing every subcommand's arguments, so that all of our command parsing is done by the outer #[derive(Parser)].

How things stand

Subcommands are accumulated by the humility-bin's build.rs. In current code, every command crate has to define a fn init(), e.g.

pub fn init() -> Command {
    Command {
        app: UartConsoleArgs::command(),
        name: "console-proxy",
        run: console_proxy,
    }
}

The build script uses cargo-metadata to find all humility-cmd-* dependencies of humility-bin, then constructs a dcmds function which returns a table of subcommands:

fn dcmds() -> Vec<CommandDescription> {
    vec![
      CommandDescription {
            init: cmd_console_proxy::init,
            // every subcommand's codegen has the same docmsg ¯\_(ツ)_/¯
            docmsg: "For additional documentation, run \"humility doc {}\"."
       },
       // ...etc
    ]
}

Then, we dispatch based on command name; the first thing that every command's init function has to do is to parse its arguments from the cmd: Vec<String> trailing argument:

fn console_proxy(context: &mut ExecutionContext) -> Result<()> {
    let subargs = UartConsoleArgs::try_parse_from(&context.cli.cmd)?;
    // ...do stuff with subargs

Because these subcommands are dispatched by our code (and not known at #[derive(Parser)] time), we have to engage in additional shenanigans to get the command docstrings properly plumbed through to Clap.

Let's just not do that

After this PR, every subcommand is required to define a pub type Args (replacing the pub fn init()). This object must implement a new HumilitySubcommand trait, e.g.

pub type Args = UartConsoleArgs;
impl HumilitySubcommand for UartConsoleArgs {
    fn run(args: Args, context: &mut ExecutionContext) -> anyhow::Result<()> {
        console_proxy(args, context)
    }
}

We still use a build.rs to iterate over subcommands, but now we produce a pub enum Subcommand:

#[derive(::clap::Parser, Debug)]
pub enum Subcommand {
    Auxflash(cmd_auxflash::Args),
    ConsoleProxy(cmd_console_proxy::Args),
    Counters(cmd_counters::Args),
    Dashboard(cmd_dashboard::Args),
    Debugmailbox(cmd_debugmailbox::Args),
    Diagnose(cmd_diagnose::Args),
    Discover(cmd_discover::Args),
    Doc(cmd_doc::Args),
    Dump(cmd_dump::Args),
    Ereport(cmd_ereport::Args),
    Exec(cmd_exec::Args),
    Extract(cmd_extract::Args),
    // ...etc

This is embedded verbatim in the CLI argument object of humility-bin. With this organization, the clap parser Just Works™. We also generate a pub fn dispatch which takes the enum and calls the appropriate runner function with the Args variant as its first argument (removing the subargs parsing from the function itself).

Other changes

• I removed humility_cmd::Command, which left Dumper as the only member of that crate, so I renamed it to humility-hexdump
• I removed a few places where we call std::process::exit in favor of returning an ExitCode from main. This is still a little unorthodox, but I wanted to exactly preserve our existing CLI output. A few cases of exit remain, and I'll get to them later

[labbott]

I think this looks okay but I want to look at the clap stuff a little closer.

We should also double check that humility-probless gets built properly and runs without libusb (just checking that we can get a help message in the switch zone)

[jamesmunns]

A couple of naggy nits (feel free to mostly ignore), but overall looks good to me.

Also, I think Args might not be the best name we can pick, it's more like a subcommand? That being said, that name isn't great either since it's used in Clap.

The one note I do think is worth doing: I think we could reduce some boilerplate by just "inlining" the "run" functions into the trait impl.

[jamesmunns]

It might be worth just putting the pub type Args = ...; at the top of each crate, and maybe include a doc comment like "Used by humility-bin/build.rs to generate the top level CLI", so folks see it in the future, and realize it's not load-bearing in the crate itself, and just used for codegen duck-typing purposes.

Right now, we use a sort of inconsistent mix of Args, the specific type, and Self in the impls. It might make sense to just pick one.

[jamesmunns]

Here we don't even use the alias, we use Self. Can we do this everywhere?

[jamesmunns]

I don't understand why we have this pattern of proxying the trait impl to a free function? It seems like a lot of syntactic noise for not a lot of benefit. Could this function just be the run method of HumilitySubcommand? Same comment for roughly all the other subcommands.

[mkeeter]

It could, but I was trying to minimize the diff here for ease of review. We could do a follow-up PR to mechanically inline everything?

[jamesmunns]

Up to you! I think if you put the trait impls where the free funcs used to be, it would diff pretty cleanly, but happy to defer that since it's just a nit anyway.

[jamesmunns]

Ah, here is the magic. Man I don't love this.

That being said: I can't really think of a better alternative here than linkme though, which would be a lateral level of shenanigans, and probably wouldn't work as well with clap. But whew.

Opened #678 to possibly rethink this, as I now understand why it has to be Like This.

[jamesmunns]

Minor nit, do we ever not use these impls in a duck-typing kind of way?

I guess requiring a trait impl "by convention" helps us avoid forgetting to impl Parser for each of the Args types, but I'm not sure if that's much value since we're duck-typing the Args name in codegen anyway.

[mkeeter]

@jamesmunns Thanks for the feedback! I killed a few birds with one stone by introducing a new macro:

humility_cmd!(WritewordArgs, writeword);

This addresses a bunch of issues that you raised:

• It's a loud marker that Something Special Is Happening Here (versus the duck-typed Args name)
• It makes the "impl function calling free function" behavior less obviously weird
• It hides the duck typing (and puts the standardized duck names in a single place)