WIP: switch to clap

[matklad]

This PR is a proof-of-concept for rewrite of Cargo's argument parsing from docopt into clap (the previous episode).

Only two commands (bench and build) are ported over to clap, and the code is of "prototype" quality :) However, I'd love to get general feedback about the general approach before moving forward with implementation.

The main problem which this aims to solve is excessive code duplication between various Cargo commands. In particular, when commands share some options (which often is the case!) each command has to repeat code, responsible for handling of these options. Annoyingly, the options are almost, but not quite, the same: for example, different commands have different descriptions of options ("test all packages" vs "benchmark all packages").

This actually makes quite difficult to use something like structopt or clap-derive for the task, so I've decided to go with the usual stringly-typed approach of clap.

The idea is to have a giant clap::App with all build-in subcommands, share definitons of options by providing .arg_color()-like extensions for defining options, and share code by providing fn worksapce_from_options(args: &ArgMatches)-like functions. Although only two commands, build and bench, are ported, they do share a significant chunk of code.

Why we might want to do switch to clap:

• reduce code duplication
• have sane subcommand handling
• use the same library for parsing options in rustup and Cargo
• take advantage of various clap features for input validation, completion generation, text-wrapping and what not

Why we might want to maintain the status quo (docopt):

• less work right now (but more maintenance work down the road)
• understanding what a particular command is doing might be easier with docopt (though the complexity to emulate subcommands with docopt I think compensates for this :-) )
• There's a high chance that we won't be able to replicate all quirks of the current implementations, so, for some edge cases, the arguments might get interpreted differently.

👏 👏 👏

r? @alexcrichton

[ehuss]

I'm just curious, I noticed you talking to someone on IRC last week about this...is there any desire to allow users of cargo-the-library to be able to reuse some of the argument code (I think the example was to help rustfmt behave the same for -p/--all/--exclude behavior)?

Also, please let me know if you are interested in any help with this little project.

[matklad]

to allow users of cargo-the-library to be able to reuse some of the argument code

This is plausible, though I think we should first just clean up the argument parsing inside Cargo itself, and only then think about extracting it as a crate.

[alexcrichton]

This seems plausible to me! For the WIP though would it be possible to delete src/bin/build.rs and see that this has no regressions?

[matklad]

For the WIP though would it be possible to delete src/bin/build.rs and see that this has no regressions?

It's not super trivial to do this exactly: ideally, you'd route build commands to the clap parser and all other commands to the old docopt parser, but you sort-of need to know the name of command for this, which means you should use some argument parser =P Though, I think I can do smth like os::args().contains("build") for that maybe?

What I've checked is that there's no regression¹ for happy cases for build and bench. That is, if clap parses the arguments, it gives the same results.

There are some "regressions" for bad command lines cases in that clap gives better error messages :)

I have not tried adding support for external commands and aliases yet, I expect these might cause some troubles.

However, I am sure that "passes Cargo test suite" is not enough to guarantee exactly same parsing of arguments for all cases. But, hopefully, it'll be good enough for practical purposes!

1. except for a single weird regression in death::ctrl_c_kills_everyone which I haven't debugged yet.

[alexcrichton]

Ok no worries if it's not lossless just yet. I'm still more than happy to go in this direction!

One suggestion I might have is to define a helper method on the trait for something like "standard set of build-related flags" which automatically implies things like frozen, locked, color, etc. That way adding a new flag to all subcommands is in theory one line-ish.

[matklad]

One suggestion I might have is to define a helper method on the trait for something like "standard set of build-related flags" which automatically implies things like frozen, locked, color, etc. That way adding a new flag to all subcommands is in theory one line-ish.

This is more or less exactly what I am doing here, in the CommonArgs trait: https://github.com/rust-lang/cargo/pull/5129/files#diff-e3233a25c684f536dd0ce8009b0e7c2dR229 :)

That way adding a new flag to all subcommands is in theory one line-ish.

Yep, that's precisely the intention!

[matklad]

I'll close the PR for now so as to save CI machines from useless work. Will reopen when this is nearer to completion!

[matklad]

Current status: I've ported existing subcomands over to clap, the diff is 1,802 additions and 2,749 deletions. :)