Skip to content

Commit

Permalink
Auto merge of #8808 - weihanglo:fix/8591, r=ehuss
Browse files Browse the repository at this point in the history
List available packages if providing `--package` with an empty value

May resolves #8591

## How

First we need to take the responsibility of check command line arguments from claps. I've examine all 10 build commands and all of them call [`ArgMatchesExt::compile_options`](https://github.com/rust-lang/cargo/blob/2f115a76e5a1e5eb11cd29e95f972ed107267847/src/cargo/util/command_prelude.rs#L389-L395) directly or indirectly. And `compile_options` [calls `check_optional_opts`](https://github.com/rust-lang/cargo/blob/2f115a76e5a1e5eb11cd29e95f972ed107267847/src/cargo/util/command_prelude.rs#L499-L501) to check if target selection options given an empty value. So we can do the same logic there.

I've also add a error message for an edge case though that one would never trigger at this moment.
  • Loading branch information
bors committed Oct 28, 2020
2 parents 358ee54 + 4b9c503 commit becb4c2
Show file tree
Hide file tree
Showing 11 changed files with 167 additions and 38 deletions.
6 changes: 6 additions & 0 deletions src/bin/cargo/commands/clean.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::command_prelude::*;

use cargo::ops::{self, CleanOptions};
use cargo::util::print_available_packages;

pub fn cli() -> App {
subcommand("clean")
Expand All @@ -18,6 +19,11 @@ pub fn cli() -> App {

pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
let ws = args.workspace(config)?;

if args.is_present_with_zero_values("package") {
print_available_packages(&ws)?;
}

let opts = CleanOptions {
config,
spec: values(args, "package"),
Expand Down
4 changes: 4 additions & 0 deletions src/bin/cargo/commands/pkgid.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::command_prelude::*;

use cargo::ops;
use cargo::util::print_available_packages;

pub fn cli() -> App {
subcommand("pkgid")
Expand All @@ -14,6 +15,9 @@ pub fn cli() -> App {

pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
let ws = args.workspace(config)?;
if args.is_present_with_zero_values("package") {
print_available_packages(&ws)?
}
let spec = args.value_of("spec").or_else(|| args.value_of("package"));
let spec = ops::pkgid(&ws, spec)?;
cargo::drop_println!(config, "{}", spec);
Expand Down
6 changes: 6 additions & 0 deletions src/bin/cargo/commands/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use anyhow::{bail, format_err};
use cargo::core::dependency::DepKind;
use cargo::ops::tree::{self, EdgeKind};
use cargo::ops::Packages;
use cargo::util::print_available_packages;
use cargo::util::CargoResult;
use std::collections::HashSet;
use std::str::FromStr;
Expand Down Expand Up @@ -176,6 +177,11 @@ subtree of the package given to -p.\n\
}

let ws = args.workspace(config)?;

if args.is_present_with_zero_values("package") {
print_available_packages(&ws)?;
}

let charset = tree::Charset::from_str(args.value_of("charset").unwrap())
.map_err(|e| anyhow::anyhow!("{}", e))?;
let opts = tree::TreeOptions {
Expand Down
9 changes: 9 additions & 0 deletions src/bin/cargo/commands/uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ pub fn cli() -> App {

pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
let root = args.value_of("root");

if args.is_present_with_zero_values("package") {
return Err(anyhow::anyhow!(
"\"--package <SPEC>\" requires a SPEC format value.\n\
Run `cargo help pkgid` for more information about SPEC format."
)
.into());
}

let specs = args
.values_of("spec")
.unwrap_or_else(|| args.values_of("package").unwrap_or_default())
Expand Down
5 changes: 5 additions & 0 deletions src/bin/cargo/commands/update.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::command_prelude::*;

use cargo::ops::{self, UpdateOptions};
use cargo::util::print_available_packages;

pub fn cli() -> App {
subcommand("update")
Expand All @@ -20,6 +21,10 @@ pub fn cli() -> App {
pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
let ws = args.workspace(config)?;

if args.is_present_with_zero_values("package") {
print_available_packages(&ws)?;
}

let update_opts = UpdateOptions {
aggressive: args.is_present("aggressive"),
precise: args.value_of("precise"),
Expand Down
26 changes: 23 additions & 3 deletions src/cargo/util/command_prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::util::restricted_names::is_glob_pattern;
use crate::util::{paths, toml::TomlProfile, validate_package_name};
use crate::util::{
print_available_benches, print_available_binaries, print_available_examples,
print_available_tests,
print_available_packages, print_available_tests,
};
use crate::CargoResult;
use anyhow::bail;
Expand Down Expand Up @@ -52,11 +52,15 @@ pub trait AppExt: Sized {
}

fn arg_package_spec_simple(self, package: &'static str) -> Self {
self._arg(multi_opt("package", "SPEC", package).short("p"))
self._arg(optional_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"))
self._arg(
optinal_opt("package", package)
.short("p")
.value_name("SPEC"),
)
}

fn arg_jobs(self) -> Self {
Expand Down Expand Up @@ -220,6 +224,10 @@ pub fn opt(name: &'static str, help: &'static str) -> Arg<'static, 'static> {
Arg::with_name(name).long(name).help(help)
}

pub fn optinal_opt(name: &'static str, help: &'static str) -> Arg<'static, 'static> {
opt(name, help).min_values(0)
}

pub fn optional_multi_opt(
name: &'static str,
value_name: &'static str,
Expand Down Expand Up @@ -498,6 +506,14 @@ pub trait ArgMatchesExt {

if let Some(ws) = workspace {
self.check_optional_opts(ws, &opts)?;
} else if self.is_present_with_zero_values("package") {
// As for cargo 0.50.0, this won't occur but if someone sneaks in
// we can still provide this informative message for them.
anyhow::bail!(
"\"--package <SPEC>\" requires a SPEC format value, \
which can be any package ID specifier in the dependency graph.\n\
Run `cargo help pkgid` for more information about SPEC format."
)
}

Ok(opts)
Expand Down Expand Up @@ -588,6 +604,10 @@ about this warning.";
workspace: &Workspace<'_>,
compile_opts: &CompileOptions,
) -> CargoResult<()> {
if self.is_present_with_zero_values("package") {
print_available_packages(workspace)?
}

if self.is_present_with_zero_values("example") {
print_available_examples(workspace, compile_opts)?;
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ pub use self::to_semver::ToSemver;
pub use self::vcs::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo};
pub use self::workspace::{
print_available_benches, print_available_binaries, print_available_examples,
print_available_tests,
print_available_packages, print_available_tests,
};

mod canonical_url;
Expand Down
39 changes: 32 additions & 7 deletions src/cargo/util/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ fn get_available_targets<'a>(
filter_fn: fn(&Target) -> bool,
ws: &'a Workspace<'_>,
options: &'a CompileOptions,
) -> CargoResult<Vec<&'a Target>> {
) -> CargoResult<Vec<&'a str>> {
let packages = options.spec.get_packages(ws)?;

let mut targets: Vec<_> = packages
Expand All @@ -19,14 +19,15 @@ fn get_available_targets<'a>(
.iter()
.filter(|target| filter_fn(target))
})
.map(Target::name)
.collect();

targets.sort();

Ok(targets)
}

fn print_available(
fn print_available_targets(
filter_fn: fn(&Target) -> bool,
ws: &Workspace<'_>,
options: &CompileOptions,
Expand All @@ -43,24 +44,48 @@ fn print_available(
} else {
writeln!(output, "Available {}:", plural_name)?;
for target in targets {
writeln!(output, " {}", target.name())?;
writeln!(output, " {}", target)?;
}
}
bail!("{}", output)
}

pub fn print_available_packages(ws: &Workspace<'_>) -> CargoResult<()> {
let packages = ws
.members()
.map(|pkg| pkg.name().as_str())
.collect::<Vec<_>>();

let mut output = "\"--package <SPEC>\" requires a SPEC format value, \
which can be any package ID specifier in the dependency graph.\n\
Run `cargo help pkgid` for more information about SPEC format.\n\n"
.to_string();

if packages.is_empty() {
// This would never happen.
// Just in case something regresses we covers it here.
writeln!(output, "No packages available.")?;
} else {
writeln!(output, "Possible packages/workspace members:")?;
for package in packages {
writeln!(output, " {}", package)?;
}
}
bail!("{}", output)
}

pub fn print_available_examples(ws: &Workspace<'_>, options: &CompileOptions) -> CargoResult<()> {
print_available(Target::is_example, ws, options, "--example", "examples")
print_available_targets(Target::is_example, ws, options, "--example", "examples")
}

pub fn print_available_binaries(ws: &Workspace<'_>, options: &CompileOptions) -> CargoResult<()> {
print_available(Target::is_bin, ws, options, "--bin", "binaries")
print_available_targets(Target::is_bin, ws, options, "--bin", "binaries")
}

pub fn print_available_benches(ws: &Workspace<'_>, options: &CompileOptions) -> CargoResult<()> {
print_available(Target::is_bench, ws, options, "--bench", "benches")
print_available_targets(Target::is_bench, ws, options, "--bench", "benches")
}

pub fn print_available_tests(ws: &Workspace<'_>, options: &CompileOptions) -> CargoResult<()> {
print_available(Target::is_test, ws, options, "--test", "tests")
print_available_targets(Target::is_test, ws, options, "--test", "tests")
}
13 changes: 13 additions & 0 deletions tests/testsuite/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1181,6 +1181,19 @@ fn uninstall_multiple_and_specifying_bin() {
.run();
}

#[cargo_test]
fn uninstall_with_empty_pakcage_option() {
cargo_process("uninstall -p")
.with_status(101)
.with_stderr(
"\
[ERROR] \"--package <SPEC>\" requires a SPEC format value.
Run `cargo help pkgid` for more information about SPEC format.
",
)
.run();
}

#[cargo_test]
fn uninstall_multiple_and_some_pkg_does_not_exist() {
pkg("foo", "0.0.1");
Expand Down
Loading

0 comments on commit becb4c2

Please sign in to comment.