Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--all-targets ignores specific targets #14542

Open
mo8it opened this issue Sep 13, 2024 · 1 comment
Open

--all-targets ignores specific targets #14542

mo8it opened this issue Sep 13, 2024 · 1 comment
Labels
A-cli Area: Command-line interface, option parsing, etc. C-bug Category: bug S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.

Comments

@mo8it
Copy link
Contributor

mo8it commented Sep 13, 2024

Problem

When both --all-targets and a specific target like --bin BIN_NAME are specified, Cargo ignores the specific target. This is not intuitive and not documented.

Steps

  1. Create a package with 2 binaries bin1 and bin2.
  2. Run cargo check --all-targets --keep-going --bin bin1. (--keep-going to make sure that it runs until the end)
  3. Both bin1 and bin2 are checked.

Possible Solution(s)

There are two solutions:

  1. My preference: Throw an error because the user is doing something wrong and the result isn't as they would expect.
  2. Document that --all-targets ignores any specific targets specified before or after it.

Notes

See related issue in Rust-Analyzer: rust-lang/rust-analyzer#18110

Version

cargo 1.82.0-beta.3 (8f40fc59f 2024-08-21)
release: 1.82.0-beta.3
commit-hash: 8f40fc59fb0c8df91c97405785197f3c630304ea
commit-date: 2024-08-21
host: x86_64-unknown-linux-gnu
libgit2: 1.8.1 (sys:0.19.0 vendored)
libcurl: 8.9.0-DEV (sys:0.4.74+curl-8.9.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Fedora 40.0.0 [64-bit]
@mo8it mo8it added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Sep 13, 2024
@epage epage added the A-cli Area: Command-line interface, option parsing, etc. label Sep 16, 2024
@epage
Copy link
Contributor

epage commented Sep 16, 2024

Generally, CLI parsers are position-independent. If you say --all-targets --bin NAME or --bin NAME --all-targets, the meaning is the same. In this case, the arguments are additive, so by adding in --all-targets, the --bin flag becomes redundant, not wrong.

The code for where this is handled is at

/// Constructs a filter from raw command line arguments.
pub fn from_raw_arguments(
lib_only: bool,
bins: Vec<String>,
all_bins: bool,
tsts: Vec<String>,
all_tsts: bool,
exms: Vec<String>,
all_exms: bool,
bens: Vec<String>,
all_bens: bool,
all_targets: bool,
) -> CompileFilter {
if all_targets {
return CompileFilter::new_all_targets();
}
let rule_lib = if lib_only {
LibRule::True
} else {
LibRule::False
};
let rule_bins = FilterRule::new(bins, all_bins);
let rule_tsts = FilterRule::new(tsts, all_tsts);
let rule_exms = FilterRule::new(exms, all_exms);
let rule_bens = FilterRule::new(bens, all_bens);
CompileFilter::new(rule_lib, rule_bins, rule_tsts, rule_exms, rule_bens)
}

This can work well for situations like an alias that you want to override a flag from. I had considered changing clap to prefer the "override" approach over "conflict", in terms of the exact same argument (--all-targets --all-targets). I had decided against this when I realized it can cause stability problems. Cargo changed to allow multiple --target <triple> flags. If we had allowed overriding on it, it would have changed from "override the last" to "append to list" which would have been a behavior change.

That doesn't mean that a conflict in this situation is the most correct approach. Also, if we did change it to an error, we'd need a case to be made for why it is safe and not a breaking change. #13629 does some analysis in that direction. A softer approach could be a warning. We would have to update the CompileFilters to make this decision where we have access to a Shell to send it.

Whether we document or warn for this, it would help to better understand the situation that led to this issue. Why did you combine these arguments? What did you expect from it?

@epage epage added S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. and removed S-triage Status: This issue is waiting on initial triage. labels Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. C-bug Category: bug S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.
Projects
None yet
Development

No branches or pull requests

2 participants