Skip to content

Conversation

@weihanglo
Copy link
Member

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 directly or indirectly. And compile_options calls check_optional_opts 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.

@rust-highfive
Copy link

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2020
@Eh2406
Copy link
Contributor

Eh2406 commented Oct 26, 2020

This looks good to me,
@bors r? @ehuss for a second opinion.

@rust-highfive rust-highfive assigned ehuss and unassigned Eh2406 Oct 26, 2020
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks useful!

I noticed that some commands do not seem to correctly validate the argument with this change. clean, tree, uninstall, and update will silently run with a -p argument without a value, when they should return an error. Can you adjust those so that they will fail (and preferably print out an available list)?

I also noticed that the single-argument commands (rustc, rustdoc, pkgid, and run) don't print the available list. I won't block the PR if you don't want to add them, but I think it would be nice.

Finally, I'm a little concerned that this might contribute some confusion. In most cases, the -p argument can accept any package in the resolve graph, not just workspace members. However, I think listing everything would be too noisy. I'm not sure how to improve that. Maybe if it said "Available workspace members:" or something like that?

Comment on lines 45 to 46
fn print_availables(output: String, availables: &[&str], plural_name: &str) -> CargoResult<()> {
let mut output = output;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn print_availables(output: String, availables: &[&str], plural_name: &str) -> CargoResult<()> {
let mut output = output;
fn print_availables(mut output: String, availables: &[&str], plural_name: &str) -> CargoResult<()> {

Comment on lines 64 to 69
let mut output = String::new();
writeln!(
output,
"\"--package <SPEC>\" requires a SPEC format value.\n\
Run `cargo help pkgid` for more infomation about SPEC format."
)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mut output = String::new();
writeln!(
output,
"\"--package <SPEC>\" requires a SPEC format value.\n\
Run `cargo help pkgid` for more infomation about SPEC format."
)?;
let output = "\"--package <SPEC>\" requires a SPEC format value.\n\
Run `cargo help pkgid` for more infomation about SPEC format.\n"
.to_string();

@weihanglo
Copy link
Member Author

weihanglo commented Oct 27, 2020

I also noticed that the single-argument commands (rustc, rustdoc, pkgid, and run) don't print the available list. I won't block the PR if you don't want to add them, but I think it would be nice.

Oh I see. I was thinking that the only accept one package so showing a list does not help. But I was wrong. It still benefits 😂. Will fix it.

I'm not sure how to improve that. Maybe if it said "Available workspace members:" or something like that?

That’s also what I was concerned about, therefore I decided to add a help message guiding people to learn more about pkgid. Showing "Available workspace members:" seems very straightforward. However, if a single-package crate shows message like that, would it confuses user about workspace/package, though both kind of compilations can get workspace information internally in cargo.

Would this be better?

[ERROR] "--package <SPEC>" requires a SPEC format value, which can be any package ID specifier in the dependency graph.
Run `cargo help pkgid` for more information about SPEC format."

Possible packages/workspace members:
    foo

@weihanglo weihanglo requested a review from ehuss October 27, 2020 19:10
@ehuss
Copy link
Contributor

ehuss commented Oct 28, 2020

Looks good, thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Oct 28, 2020

📌 Commit 4b9c503 has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2020
@bors
Copy link
Contributor

bors commented Oct 28, 2020

⌛ Testing commit 4b9c503 with merge becb4c2...

@bors
Copy link
Contributor

bors commented Oct 28, 2020

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing becb4c2 to master...

@bors bors merged commit becb4c2 into rust-lang:master Oct 28, 2020
@weihanglo weihanglo deleted the fix/8591 branch October 29, 2020 00:46
@ehuss ehuss added this to the 1.49.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cargo run -p should list valid packages like --bin lists targets

5 participants