From 70ff33a5378407021e3e496d01d72156f775c30f Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 16 Mar 2018 17:35:13 +0300 Subject: [PATCH] Fix a regression with parsing multivalue options By default, clap interprets ``` cargo run --bin foo bar baz ``` as ``` cargo run --bin foo --bin bar --bin baz ``` This behavior is different from docopt and does not play nicely with positional arguments at all. Luckily, clap has a flag to get the behavior we want, it just not the default! It will become the default in the next version of clap, but, until that time, we should be careful when using the combination of `.long`, `.value_name` and `.multiple(true)`, and don't forget to specify `.number_of_values(1)` as well. --- src/bin/cli.rs | 1 + src/bin/command_prelude.rs | 54 ++++++++++++++++++++++++----------- src/bin/commands/bench.rs | 2 +- src/bin/commands/build.rs | 2 +- src/bin/commands/check.rs | 2 +- src/bin/commands/clean.rs | 7 +---- src/bin/commands/doc.rs | 2 +- src/bin/commands/owner.rs | 16 ++++------- src/bin/commands/pkgid.rs | 2 +- src/bin/commands/run.rs | 2 +- src/bin/commands/rustc.rs | 2 +- src/bin/commands/rustdoc.rs | 2 +- src/bin/commands/test.rs | 2 +- src/bin/commands/uninstall.rs | 6 +--- src/bin/commands/update.rs | 7 +---- tests/testsuite/run.rs | 33 +++++++++++++++++++++ 16 files changed, 89 insertions(+), 53 deletions(-) diff --git a/src/bin/cli.rs b/src/bin/cli.rs index ae96579bb2c..a6ca961af58 100644 --- a/src/bin/cli.rs +++ b/src/bin/cli.rs @@ -165,6 +165,7 @@ See 'cargo help ' for more information on a specific command.", .short("Z") .value_name("FLAG") .multiple(true) + .number_of_values(1) .global(true), ) .subcommands(commands::builtin()); diff --git a/src/bin/command_prelude.rs b/src/bin/command_prelude.rs index 450abd7a6bc..b93d16dc26c 100644 --- a/src/bin/command_prelude.rs +++ b/src/bin/command_prelude.rs @@ -15,17 +15,22 @@ pub type App = clap::App<'static, 'static>; pub trait AppExt: Sized { fn _arg(self, arg: Arg<'static, 'static>) -> Self; - fn arg_package(self, package: &'static str, all: &'static str, exclude: &'static str) -> Self { - self._arg( - opt("package", package) - .short("p") - .value_name("SPEC") - .multiple(true), - )._arg(opt("all", all)) - ._arg(opt("exclude", exclude).value_name("SPEC").multiple(true)) + fn arg_package_spec( + self, + package: &'static str, + all: &'static str, + exclude: &'static str, + ) -> Self { + self.arg_package_spec_simple(package) + ._arg(opt("all", all)) + ._arg(multi_opt("exclude", "SPEC", exclude)) } - fn arg_single_package(self, package: &'static str) -> Self { + fn arg_package_spec_simple(self, package: &'static str) -> Self { + self._arg(multi_opt("package", "SPEC", package).short("p")) + } + + fn arg_package(self, package: &'static str) -> Self { self._arg(opt("package", package).short("p").value_name("SPEC")) } @@ -51,18 +56,18 @@ pub trait AppExt: Sized { all: &'static str, ) -> Self { self.arg_targets_lib_bin(lib, bin, bins) - ._arg(opt("example", examle).value_name("NAME").multiple(true)) + ._arg(multi_opt("example", "NAME", examle)) ._arg(opt("examples", examles)) - ._arg(opt("test", test).value_name("NAME").multiple(true)) + ._arg(multi_opt("test", "NAME", test)) ._arg(opt("tests", tests)) - ._arg(opt("bench", bench).value_name("NAME").multiple(true)) + ._arg(multi_opt("bench", "NAME", bench)) ._arg(opt("benches", benchs)) ._arg(opt("all-targets", all)) } fn arg_targets_lib_bin(self, lib: &'static str, bin: &'static str, bins: &'static str) -> Self { self._arg(opt("lib", lib)) - ._arg(opt("bin", bin).value_name("NAME").multiple(true)) + ._arg(multi_opt("bin", "NAME", bin)) ._arg(opt("bins", bins)) } @@ -73,15 +78,15 @@ pub trait AppExt: Sized { example: &'static str, examples: &'static str, ) -> Self { - self._arg(opt("bin", bin).value_name("NAME").multiple(true)) + self._arg(multi_opt("bin", "NAME", bin)) ._arg(opt("bins", bins)) - ._arg(opt("example", example).value_name("NAME").multiple(true)) + ._arg(multi_opt("example", "NAME", example)) ._arg(opt("examples", examples)) } fn arg_targets_bin_example(self, bin: &'static str, example: &'static str) -> Self { - self._arg(opt("bin", bin).value_name("NAME").multiple(true)) - ._arg(opt("example", example).value_name("NAME").multiple(true)) + self._arg(multi_opt("bin", "NAME", bin)) + ._arg(multi_opt("example", "NAME", example)) } fn arg_features(self) -> Self { @@ -157,6 +162,21 @@ pub fn opt(name: &'static str, help: &'static str) -> Arg<'static, 'static> { Arg::with_name(name).long(name).help(help) } +pub fn multi_opt( + name: &'static str, + value_name: &'static str, + help: &'static str, +) -> Arg<'static, 'static> { + // Note that all `.multiple(true)` arguments in Cargo should specify + // `.number_of_values(1)` as well, so that `--foo val1 val2` is + // **not** parsed as `foo` with values ["val1", "val2"]. + // `number_of_values` should become the default in clap 3. + opt(name, help) + .value_name(value_name) + .multiple(true) + .number_of_values(1) +} + pub fn subcommand(name: &'static str) -> App { SubCommand::with_name(name).settings(&[ AppSettings::UnifiedHelpMessage, diff --git a/src/bin/commands/bench.rs b/src/bin/commands/bench.rs index b9ee85d539c..01676b51500 100644 --- a/src/bin/commands/bench.rs +++ b/src/bin/commands/bench.rs @@ -29,7 +29,7 @@ pub fn cli() -> App { "Benchmark all targets (default)", ) .arg(opt("no-run", "Compile, but don't run benchmarks")) - .arg_package( + .arg_package_spec( "Package to run benchmarks for", "Benchmark all packages in the workspace", "Exclude packages from the benchmark", diff --git a/src/bin/commands/build.rs b/src/bin/commands/build.rs index 5b063065332..2a772e03da2 100644 --- a/src/bin/commands/build.rs +++ b/src/bin/commands/build.rs @@ -6,7 +6,7 @@ pub fn cli() -> App { subcommand("build") .alias("b") .about("Compile a local package and all of its dependencies") - .arg_package( + .arg_package_spec( "Package to build", "Build all packages in the workspace", "Exclude packages from the build", diff --git a/src/bin/commands/check.rs b/src/bin/commands/check.rs index a6ce5d7b2ce..45361740d61 100644 --- a/src/bin/commands/check.rs +++ b/src/bin/commands/check.rs @@ -5,7 +5,7 @@ use cargo::ops::{self, CompileMode}; pub fn cli() -> App { subcommand("check") .about("Check a local package and all of its dependencies for errors") - .arg_package( + .arg_package_spec( "Package(s) to check", "Check all packages in the workspace", "Exclude packages from the check", diff --git a/src/bin/commands/clean.rs b/src/bin/commands/clean.rs index 908a0c1ee64..8d0928cc2b2 100644 --- a/src/bin/commands/clean.rs +++ b/src/bin/commands/clean.rs @@ -5,12 +5,7 @@ use cargo::ops::{self, CleanOptions}; pub fn cli() -> App { subcommand("clean") .about("Remove artifacts that cargo has generated in the past") - .arg( - opt("package", "Package to clean artifacts for") - .short("p") - .value_name("SPEC") - .multiple(true), - ) + .arg_package_spec_simple("Package to clean artifacts for") .arg_manifest_path() .arg_target_triple("Target triple to clean output for (default all)") .arg_release("Whether or not to clean release artifacts") diff --git a/src/bin/commands/doc.rs b/src/bin/commands/doc.rs index 0a503a49b04..a6a2546750f 100644 --- a/src/bin/commands/doc.rs +++ b/src/bin/commands/doc.rs @@ -9,7 +9,7 @@ pub fn cli() -> App { "open", "Opens the docs in a browser after the operation", )) - .arg_package( + .arg_package_spec( "Package to document", "Document all packages in the workspace", "Exclude packages from the build", diff --git a/src/bin/commands/owner.rs b/src/bin/commands/owner.rs index 267da4c8fa6..f20be31b191 100644 --- a/src/bin/commands/owner.rs +++ b/src/bin/commands/owner.rs @@ -6,17 +6,13 @@ pub fn cli() -> App { subcommand("owner") .about("Manage the owners of a crate on the registry") .arg(Arg::with_name("crate")) + .arg(multi_opt("add", "LOGIN", "Name of a user or team to add as an owner").short("a")) .arg( - opt("add", "Name of a user or team to add as an owner") - .short("a") - .value_name("LOGIN") - .multiple(true), - ) - .arg( - opt("remove", "Name of a user or team to remove as an owner") - .short("r") - .value_name("LOGIN") - .multiple(true), + multi_opt( + "remove", + "LOGIN", + "Name of a user or team to remove as an owner", + ).short("r"), ) .arg(opt("list", "List owners of a crate").short("l")) .arg(opt("index", "Registry index to modify owners for").value_name("INDEX")) diff --git a/src/bin/commands/pkgid.rs b/src/bin/commands/pkgid.rs index 4653f898d09..7010092d60e 100644 --- a/src/bin/commands/pkgid.rs +++ b/src/bin/commands/pkgid.rs @@ -6,7 +6,7 @@ pub fn cli() -> App { subcommand("pkgid") .about("Print a fully qualified package specification") .arg(Arg::with_name("spec")) - .arg_single_package("Argument to get the package id specifier for") + .arg_package("Argument to get the package id specifier for") .arg_manifest_path() .after_help( "\ diff --git a/src/bin/commands/run.rs b/src/bin/commands/run.rs index 4f618f43040..77e5d8e13b9 100644 --- a/src/bin/commands/run.rs +++ b/src/bin/commands/run.rs @@ -13,7 +13,7 @@ pub fn cli() -> App { "Name of the bin target to run", "Name of the example target to run", ) - .arg_single_package("Package with the target to run") + .arg_package("Package with the target to run") .arg_jobs() .arg_release("Build artifacts in release mode, with optimizations") .arg_features() diff --git a/src/bin/commands/rustc.rs b/src/bin/commands/rustc.rs index 268b835a630..f0c32e69756 100644 --- a/src/bin/commands/rustc.rs +++ b/src/bin/commands/rustc.rs @@ -7,7 +7,7 @@ pub fn cli() -> App { .setting(AppSettings::TrailingVarArg) .about("Compile a package and all of its dependencies") .arg(Arg::with_name("args").multiple(true)) - .arg_single_package("Package to build") + .arg_package("Package to build") .arg_jobs() .arg_targets_all( "Build only this package's library", diff --git a/src/bin/commands/rustdoc.rs b/src/bin/commands/rustdoc.rs index f3744cebf16..6f039c8051b 100644 --- a/src/bin/commands/rustdoc.rs +++ b/src/bin/commands/rustdoc.rs @@ -11,7 +11,7 @@ pub fn cli() -> App { "open", "Opens the docs in a browser after the operation", )) - .arg_single_package("Package to document") + .arg_package("Package to document") .arg_jobs() .arg_targets_all( "Build only this package's library", diff --git a/src/bin/commands/test.rs b/src/bin/commands/test.rs index e0f8e0014ee..93db6dccd95 100644 --- a/src/bin/commands/test.rs +++ b/src/bin/commands/test.rs @@ -32,7 +32,7 @@ pub fn cli() -> App { .arg(opt("doc", "Test only this library's documentation")) .arg(opt("no-run", "Compile, but don't run tests")) .arg(opt("no-fail-fast", "Run all tests regardless of failure")) - .arg_package( + .arg_package_spec( "Package to run tests for", "Test all packages in the workspace", "Exclude packages from the test", diff --git a/src/bin/commands/uninstall.rs b/src/bin/commands/uninstall.rs index 3ed9ec51605..2031851195b 100644 --- a/src/bin/commands/uninstall.rs +++ b/src/bin/commands/uninstall.rs @@ -6,11 +6,7 @@ pub fn cli() -> App { subcommand("uninstall") .about("Remove a Rust binary") .arg(Arg::with_name("spec").multiple(true)) - .arg( - opt("bin", "Only uninstall the binary NAME") - .value_name("NAME") - .multiple(true), - ) + .arg(multi_opt("bin", "NAME", "Only uninstall the binary NAME")) .arg(opt("root", "Directory to uninstall packages from").value_name("DIR")) .after_help( "\ diff --git a/src/bin/commands/update.rs b/src/bin/commands/update.rs index 8b93c368266..c5a992a3da1 100644 --- a/src/bin/commands/update.rs +++ b/src/bin/commands/update.rs @@ -5,12 +5,7 @@ use cargo::ops::{self, UpdateOptions}; pub fn cli() -> App { subcommand("update") .about("Update dependencies as recorded in the local lock file") - .arg( - opt("package", "Package to clean artifacts for") - .short("p") - .value_name("SPEC") - .multiple(true), - ) + .arg_package_spec_simple("Package to update") .arg(opt( "aggressive", "Force updating all dependencies of as well", diff --git a/tests/testsuite/run.rs b/tests/testsuite/run.rs index 2c298aaef00..c6918910524 100644 --- a/tests/testsuite/run.rs +++ b/tests/testsuite/run.rs @@ -1109,3 +1109,36 @@ fn run_multiple_packages() { .with_stderr_contains("[ERROR] package `d3` is not a member of the workspace"), ); } + +#[test] +fn explicit_bin_with_args() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + assert_eq!(std::env::args().nth(1).unwrap(), "hello"); + assert_eq!(std::env::args().nth(2).unwrap(), "world"); + } + "#, + ) + .build(); + + assert_that( + p.cargo("run") + .arg("--bin") + .arg("foo") + .arg("hello") + .arg("world"), + execs().with_status(0), + ); +}