From: Aleksey Kladov Date: Fri, 16 Mar 2018 14:35:13 +0000 (+0300) Subject: Fix a regression with parsing multivalue options X-Git-Tag: archive/raspbian/0.35.0-2+rpi1~3^2^2^2^2^2^2^2~22^2~2^2~34^2 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=70ff33a5378407021e3e496d01d72156f775c30f;p=cargo.git 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. --- diff --git a/src/bin/cli.rs b/src/bin/cli.rs index ae96579bb..a6ca961af 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 450abd7a6..b93d16dc2 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 b9ee85d53..01676b515 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 5b0630653..2a772e03d 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 a6ce5d7b2..45361740d 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 908a0c1ee..8d0928cc2 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 0a503a49b..a6a254675 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 267da4c8f..f20be31b1 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 4653f898d..7010092d6 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 4f618f430..77e5d8e13 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 268b835a6..f0c32e697 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 f3744cebf..6f039c805 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 e0f8e0014..93db6dccd 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 3ed9ec516..203185119 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 8b93c3682..c5a992a3d 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 2c298aaef..c69189105 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), + ); +}