refactor: Pull out cargo-add MSRV code for reuse
### What does this PR try to resolve?
#12078 added MSRV code in `cargo add`. Our assumption when writing it is that we'd need to generalize the code before reusing it in other places, like `cargo install`. This PR focused purely on that refactor because I'm hopeful it will be useful for other work I'm doing. Despite not having a user for this yet, I think the `cargo install` case is inevitable and I feel this does a bit to clean up MSRV related code by using a more specific type everywhere.
### How should we test and review this PR?
Each commit gradually progresses things along
fix(toml): Improve parse errors
### What does this PR try to resolve?
When we adopted `toml_edit`, we got TOML syntax errors that showed the context for where the error occurred. However, the work was not done to extend this to semantic errors reported by serde.
This updates `Cargo.toml` and `Cargo.lock` code to provide that context on semantic errors. `config.toml` is not done because the schema is decentralized.
In theory, this will also improve performance because we aren't having to allocate a lot of intermediate data to then throw away for every `Cargo.toml` we read.
### How should we test and review this PR?
Check by commit to see this change gradually.
- The `package.cargo-features` change was made to drop out dependence on `toml::Table` so we could do the direct deserialization
Create dedicated unstable flag for asymmetric-token
Asymmetric tokens are gated by `-Zcredential-process`. Since we're considering stabilizing that soon, this moves asymmetric token support to have its own unstable flag.
It was previously gated by `-Zregistry-auth`, and some of the docs were not updated when it moved.
r? `@Eh2406`
Before, we'd render the source for TOML syntax errors but not semantic errors.
Now we render for both.
Originally I changed `parse_document` to returned `T: DeserializeOwned`
but that adds an extra "could not parse TOML" which is both redundant
and makes it sound like its a syntax issue.
config: merge lists in precedence order
When merging configuration lists, the current order does not match the expected precedence. This makes merged lists follow precedence order, with higher precedence items merged later in lists.
When a list in configuration exists in multiple places, Cargo merges the lists together. The ordering of this merging is unexpected and does not follow the precedence rules that non-list configuration uses.
The current merging order appears to be:
* project-specific `config.toml`
* global `config.toml`
* command-line (`--config`)
* environment variable (`CARGO_*`)
This PR changes the order to follow the precedence rules with higher precedence configuration merging later in the lists.
* global `config.toml`
* project-specific `config.toml`
* environment variable (`CARGO_*`)
* command-line (`--config`)
This aligns with config such as `build.rustflags` where later flags take precedence over earlier ones.
Since `--config` is relatively new, it's unlikely to cause too much breakage by making it come after environment variables.
Switching global and project-specific ordering is more likely to cause breakage, since it's been around longer (reported as an issue in #8128). Projects relying on global configuration flags (in `$CARGO_HOME\config.toml` or in `.cargo/config.toml` further from the project) being merged first in lists will be broken.
For most uses of merged lists (such as `build.rustflags`), if the flags do not conflict with each other, there will be no impact.
Fixes#12506Fixes#8128
When merging configuration lists, the current order does not match
the expected precedence. This makes merged lists follow precedence
order, with higher precedence items merged later in lists.
It confuses people that both `--no-fail-fast` and `--keep-going` exist
on `cargo test` and `cargo bench` but with slightly different behavior.
The intended use cases for `--keep-going` involve build commands like
`build`/`check`/`clippy` but never `test`/`bench`.
Hence, this commit removes `--keep-going` from `test`/`bench` and
provides guidance of `--no-fail-fast` instead.
If people really want to build as many tests as possible, they can also
do it in two steps:
cargo build --tests --keep-going
cargo test --test --no-fail-fast
prompt the use of `--nocapture` flag if `cargo test` process is terminated via a signal.
Fixes#10855
As per the discussion on this issue, we want to prompt the user to use `--nocapture` if a test is terminated abnormally. The motivation for this change is described in the issue.
We check for 3 things before we display this flag. -
- `!is_simple` (if the test ended with a non 101 status code)
- `harness` (if the standard test harness was used), and
- `!nocapture` (whether or not the `--nocapture` flag was already passed to the test)
There's further tests added to `test::nonzero_exit_status` that check that the `stderr` is correct for the various combinations possible when a test ends with a non-101 status code.
The new expected behavior is -
- Display `--nocapture` note for only non-zero exit statuses, when the `--nocapture` flag is not passed.
- Only display the note if we use a standard test harness since custom test harnesses do not implement the `--nocapture` flag.
To implement the check for the `--nocapture` flag, the function definition for `report_test_errors` was changed to add the `test_args: &[&str]` parameter. This parameter is passed from the immediate calling function. This private function is only called twice change and is not causing regression after making the appropriate changes to both the places it's called in.
Fix cargo remove incorrectly removing used patches
### What does this PR try to resolve?
Fixes an issue where patches are being removed when member dependencies don't explicitly contain the patched crate.
Fixes#12419
### How should we test and review this PR?
- Created a test for the failing use case
- Verify passing test
<!--
### Additional information
Other information you want to mention in this PR, such as prior arts,
future extensions, an unresolved problem, or a TODO list.
-->
fix(cli): Make `--help` easier to browse
This mirrors some of the categories from `cargo help` (the man pages) using [`clap::Arg::help_heading`](https://docs.rs/clap/latest/clap/struct.Arg.html#method.help_heading).
There are fewer categories to avoid extra vertical space. Instead, they
are left int the `Options` category but put first.
The goal is to try to make it easier to scan `--help` output as the list of flags can get quite long and its easy to miss what features are there.
This mirrors some of the categories from `cargo help` (the man pages).
There are fewer categories to avoid extra vertical space. Instead, they
are left int the `Options` category but put first.
Fix printing multiple warning messages for unused fields in [registries] table
Cargo currently prints the same warning message multiple times for unexpected fields in the `[registries.<registry>]` or `[registry]` tables. This is because Cargo warns each time that the structure is deserialized from `Config`. Depending on which code path is taken I've seen the warning printed up to 6 times.
* A cache of deserialized registry configurations is added to the `Config` struct.
* Registry authentication is changed to directly read the config when searching for a registry name, rather than deserializing each registry configuration.
A test is added to ensure both `[registries]` and `[registry]` only warn once for unexpected fields.
In 1.71, `.cargo-ok` changed to contain a JSON `{ v: 1 }` to indicate
the version of it. A failure of parsing will result in a heavy-hammer
approach that unpacks the `.crate` file again. This is in response to a
security issue that the unpacking didn't respect umask on Unix systems.
Without this, an attacker can leverage globally writable files buried
in the `.crate` file. After a user downloaded and unpacked the file,
the attacker can then write malicous code to the downloaded sources.
`#[allow(internal_features)]` in RUSTC_BOOTSTRAP test
This will be required in the future (where "the future" is rust-lang/rust#108955 which fails CI because of cargo here).
This does emit the unknown lints lint right now but that doesn't matter as it's just warn-by-default - internal_features is deny-by-default though, so it causes errors.
fix(update): Tweak CLI behavior
### What does this PR try to resolve?
When looking at `cargo update` for #12425, I noticed that the two flags related to `--package` were not next to it or each other. I figured grouping them like that would make things easier to browse.
When looking into that, I noticed that the two flags conflict and figured we'd provide a better error message if we did that through clap.
### How should we test and review this PR?
Looking per commit will help show the behavior changes.
### Additional information
I wanted to scope this to being simple, non-controversial, low effort, incremental improvements with this change so I did not look into the history of `--aggressive` not requiring `--package` like `--precise` does and figure out if there is any consistency we can be working towards.
Use thiserror for credential provider errors
### What does this PR try to resolve?
Errors from credential providers currently must a single string. This leads to a lot of `.map_err(|e|cargo_credential::Error::Other(e.to_string())`, which loses the `source()` of these errors.
This changes the `cargo_credential::Error` to use `thiserror` and adds a custom serialization for `std::error::Error` that preserves the source error chain across serialization / deserialization.
A unit test is added to verify serialization / deserialization.
Per my proposal here: https://github.com/rust-lang/cargo/issues/6669#issuecomment-1658593167
I tried to keep the edit minimalistic to match the surrounding style.
If the maintainers are amenable to it, I think it could also be useful to do one or more of:
- Offer concrete guidance on what to do to run actually-all tests (`--all-targets` then separately `--doc`).
- Link to the issue at: https://github.com/rust-lang/cargo/issues/6669
- Mention that `cargo test` without `--all-targets` runs doctests by default, which seems not immediately obvious.
I'd be happy to attempt to add any of the above that the maintainers feel would be a good fit here.
