Add completions for `cargo remove`
### What does this PR try to resolve?
This PR continues the deferred work of #11099 by adding bash and zsh completions for the cargo remove subcommand.
### How should we test and review this PR?
There doesn't seem to be much in the way of testing for these completions automatically, so I would suggest verifying that they work in practice and sufficiently cover the subcommand's surface area.
### Additional Information
I will also soon post a PR for cargo remove's documentation.
Config file loaded via CLI takes priority over env vars
### What does this PR try to resolve?
Fixes#10992
Behaviour changes: After this commit, config files loaded via CLI take
priority over env vars. Config files loaded via [`config-include`]
feature also get a higher priority over env vars, if the initial config
file is being loaded via CLI.
Cargo knows why it loads a specific config file with `WhyLoad` enum,
and store in the field of `Definition::Cli(…)`. With this additional data,
config files loaded via `--config <path>` get a `Definition::Cli(Some(…))`.
In contrast, `--config <value>` with ordinary values become `Definition::Cli(None)`.
The error message of the `Definition::Cli(Some(…))` is identical to
`Definition::Path(…)`, so we won't lose any path information to debug.
[`config-include`]: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-include
### How should we test and review this PR?
Reviewing commit by commit is probably fine. The first two commits adding tests to `config-cli` and `config-include`, which exercises the expected behaviour we are going to fix in the next commits.
To check the precedence, set up a project with an extra config file, such as
```
# Expect to have a target-dir named `foo`
CARGO_BUILD_TARGET_DIR='env' cargo c --config 'build.target-dir = "cli"' --config .cargo/foo.toml
# Inside .cargo/foo.toml
[build]
target-dir = "foo"
```
### Additional information
This is a bit hacky IMO. I don't like leaking the path info to `Definition::Cli`. However, it is really tricky to provide information for deserialization before values get merged.
As the existence of `FeaturesFor` is acting as a key that discriminate
activated features for the same package with different circumstances.
Artifact dependencies deserve its own variant and it makes things clear to me.
- `NormalOrDevOrArtifactTarget(None)` -> `NormalOrDev`
- `NormalOrDevOrArtifactTarget(Some(Target))` -> `ArtifactDep(Target)`
Implement RFC 3289: source replacement ambiguity
### Implements [RFC 3289](https://github.com/rust-lang/rfcs/pull/3289)
* When the crates-io source is replaced, the user needs to specify `--registry <NAME>` when running an API operation to disambiguate which registry to use. Otherwise, cargo will issue a new error.
* In source replacement, the `replace-with` key can reference the name of an alt registry in the `[registries]` table.
* Publishing to source-replaced crates.io is no longer permitted using the crates.io token (`registry.token`). We have had a deprecation warning in place since #7973 (1.45.0).
### Testing
* Tests that interacting with crates.io use the new `replace_crates_io` function, which internally sets an environment variable to change the URL of crates.io.
Changes are insta-stable.
cc #10894
r? `@Eh2406`
Use correct version of cargo in test
Fix `cargo_remove::offline` test using wrong version of cargo in test, the local version and calling instance of cargo instead of the one being tested.
Issue discovered in #10907 after merge of #11099.
Check empty input for login
### What does this PR try to resolve?
close https://github.com/rust-lang/cargo/issues/11144
Check empty input for login.
### How should we test and review this PR?
- unit test
- cargo login and enter
Add retry support to sparse registries
Sparse (HTTP) registries currently do not respect Cargo's retry policy for http requests.
This change makes sparse registries use the same retry system as package downloads.
fix(test): Distinguish 'testname' from escaped arguments
When working on clap v4, it appeared that `last` and `trailing_var_arg`
are mutually exclusive, so I called that out in the debug asserts in
#4187. Unfortunately, I didn't document my research on this
as my focus was elsewhere.
When updating cargo to use clap v4 in #11159, I found `last` and
`trailing_var_arg` were used together. I figured this was what I was
trying to catch with above and I went off of my intuitive sense of these
commands and assumed `trailing_var_arg` was intended, not knowing about
the `testname` positional that is mostly just a forward to `libtest`,
there for documentation purposes, except for a small optimization.
So it looks like we just need the `last` call and not the
`trailing_var_arg` call.
This restores us to behavior from 531ce1321d which is what I originally
wrote the tests against.
It looks like #11159 was merged after the last beta branch was made, so we shouldn't
need to cherry-pick this into any other release.
For reviewing this, I made the test updates in the first commit, showing the wrong behavior. The following commit fixes the behavior and updates the tests to expected behavior.
Fixes#11191
When working on clap v4, it appeared that `last` and `trailing_var_arg`
are mutually exclusive, so I called that out in the debug asserts in
clap-rs/clap#4187. Unfortunately, I didn't document my research on this
as my focus was elsewhere.
When updating cargo to use clap v4 in #11159, I found `last` and
`trailing_var_arg` were used together. I figured this was what I was
trying to catch with above and I went off of my intuitive sense of these
commands and assumed `trailing_var_arg` was intended, not knowing about
the `testname` positional that is mostly just a forward to `libtest`,
there for documentation purposes, except for a small optimization.
So it looks like we just need the `last` call and not the
`trailing_var_arg` call.
This restores us to behavior from 531ce1321 which is what I originally
wrote the tests against.
Fix sparse registry lockfile urls containing 'registry+sparse+'
The `Cargo.lock` file for alternative sparse registries incorrectly lists the url as `registry+sparse+` rather than `sparse+`.
Fixes#10963
Import `cargo remove` into cargo
## What does this PR try to resolve?
This PR merges `cargo remove` from [cargo-edit](https://github.com/killercup/cargo-edit) into cargo.
### Motivation
- General approval from community, see https://github.com/rust-lang/cargo/issues/5586 and https://github.com/rust-lang/cargo/issues/10520.
- Satisfying symmetry between add and remove.
- Help users clean up their manifests (for example, when users forget to remove optional dependencies from feature lists).
With https://github.com/rust-lang/cargo/pull/10472, cargo-add was added to cargo. As part of that discussion, it was also proposed that `cargo rm` (now `cargo remove`) eventually be added as well.
### Drawbacks
- Additional code always opens the door for more bugs and features
- The scope of this command is fairly small though
- Known bugs and most known features were resolved before this merge proposal
### Behavior
`cargo remove` operates on one or more dependencies from a manifest, removing them from a specified dependencies section (using the same flags as `cargo-add`) and from `[features]` activations if the dependency is optional. Feature lists themselves are not automatically removed when made empty. Like with cargo-add, the lock file is automatically updated.
Note: like `cargo add`, `cargo remove` refers to dependency names, rather than crate names, which can be different with the presence of the `name` field.
Note: `cargo rm` has been renamed to `cargo remove`, based on prior art and user feedback (see [discussion](https://github.com/rust-lang/cargo/issues/10520)). Although this renaming is arguably an improvement, adding an `rm` alias could make the switch easier for existing users of cargo-edit (at the cost of a naming conflict which would merit insta-stabilization).
#### Help output
<details>
```shell
$ cargo run -- remove --help
cargo-remove
Remove dependencies from a Cargo.toml manifest file
USAGE:
cargo remove [OPTIONS] <DEP_ID>...
ARGS:
<DEP_ID>... Dependencies to be removed
OPTIONS:
-p, --package [<SPEC>...] Package to remove from
-v, --verbose Use verbose output (-vv very verbose/build.rs output)
--manifest-path <PATH> Path to Cargo.toml
--offline Run without accessing the network
-q, --quiet Do not print cargo log messages
--dry-run Don't actually write the manifest
-Z <FLAG> Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details
-h, --help Print help information
SECTION:
--dev Remove as development dependency
--build Remove as build dependency
--target <TARGET> Remove as dependency from the given target platform
```
</details>
#### Example usage
```
cargo remove serde
cargo remove criterion httpmock --dev
cargo remove winhttp --target x86_64-pc-windows-gnu
cargo remove --package core toml
```
## How should we test and review this PR?
This is following the pattern from cargo-add which was implemented in three different PRs (implementation, documentation, and completions), in the interest of reducing the focusing discussions in each PR and allowing cargo-add's behavior to settle to avoid documentation churn.
1. https://github.com/rust-lang/cargo/pull/10472
2. https://github.com/rust-lang/cargo/pull/10578
3. https://github.com/rust-lang/cargo/pull/10577
The remaining changes (documentation and shell completions) will follow shortly after.
Some work has already begun on this feature in https://github.com/rust-lang/cargo/pull/11059.
Work on this feature was carried out on the [`merge-rm`](https://github.com/killercup/cargo-edit/compare/master...merge-rm) branch of cargo-edit with PRs reviewed by `@epage.` If you are interested in seeing how this feature evolved to better match cargo's internals, you might find the commit history there to be helpful. As this PR is reviewed, changes will be made both here and on that branch, with the commit history being fully maintained on the latter.
`cargo remove` is structured like most other subcommands:
- `src/bin/cargo/commands/remove.rs` contains the cli handling and top-level execution.
- `src/cargo/ops/cargo_remove.rs` contains the implementation of the feature itself.
In order to support this feature, the `remove_from_table` util was added to `util::toml_mut::manifest::LocalManifest`.
Tests are split out into a separate commit to make it easier to review the production code and tests. Tests have been implemented with `snapbox`, structured similarly to the tests of `cargo add`.
### Prior art
- Python: [`poetry remove`](https://python-poetry.org/docs/cli/#remove)
- Supports dry run
- JavaScript: [`yarn remove`](https://yarnpkg.com/cli/remove)
- Supports wildcards
- JavaScript: [`pnpm remove`](https://pnpm.io/cli/remove)
- Go: [`go get`](https://go.dev/ref/mod#go-get)
- `go get foo@none` to remove
- Julia: [`pkg rm`](https://docs.julialang.org/en/v1/stdlib/Pkg/)
- Supports `--all` to remove all dependencies
- Ruby: [`bundle remove`](https://bundler.io/v2.2/man/bundle-remove.1.html)
- Dart: [`dart pub remove`](https://dart.dev/tools/pub/cmd/pub-remove)
- Supports dry run
- Lua: [`luarocks remove`](https://github.com/luarocks/luarocks/wiki/remove)
- Supports force remove
- .NET: [`Uninstall-Package`](https://docs.microsoft.com/en-us/nuget/reference/ps-reference/ps-ref-uninstall-package)
- Supports dry run
- Supports removal of dependencies
- Supports force remove (disregards dependencies)
- Haxe: [`haxelib remove`](https://lib.haxe.org/documentation/using-haxelib/#remove)
- Racket: [`raco pkg remove`](https://docs.racket-lang.org/pkg/cmdline.html#%28part._raco-pkg-remove%29)
- Supports dry run
- Supports force remove (disregards dependencies)
- Supports demotion to weak dependency (sort of a corollary of force remove)
### Insta-stabilization
In the discussion of `cargo add`'s stabilization story ([Zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Stablizing.20cargo-add)), it was brought up that the feature might benefit from being insta-stabilized to avoid making the cargo-edit version of the binary hard to access. Since `cargo rm` (from cargo-edit) was renamed to `cargo remove` here, [such a conflict no longer exists](https://crates.io/search?q=cargo%20remove), so this is less of a concern.
Since this feature is already has a had a long run of user testing in cargo-edit and doesn't have unsettled UI questions like cargo-add did, it might still be a candidate for insta-stabilization.
### Deferred work
Necessary future work:
- Add documentation.
- Add shell completions.
- Perform GC on workspace dependencies when they are no longer used (see https://github.com/rust-lang/cargo/issues/8415).
- This is inspired by a feature from the RFC that was dropped (unused dependencies triggering a warning)
- This was deferred out to avoid challenges with testing nightly features
It was found in the review of `cargo add` that it was best to defer these first two items to focus the discussion and as there was still behavior churn during the review of cargo-add.
### Future Possibilities
The following are features which we might want to add to `cargo remove` in the future:
- Add a `cargo rm` alias to ease transition for current cargo-edit users
- Automatically convert between dash and underscores in deps: https://github.com/killercup/cargo-edit/issues/690
- Remove unused dependencies: https://github.com/killercup/cargo-edit/issues/415
- Clean up caches: https://github.com/killercup/cargo-edit/issues/647
### Additional information
Fixes#10520.
Provide a better error message when mixing dep: with /
Features of the form `dep:foo/feature` aren't accepted as valid syntax. This generated a somewhat confusing error message of:
```
feature `f1` includes `dep:bar/bar-feat`, but `dep:bar` is not a dependency
```
This PR adds a more targeted error message that provides some suggestions on how to fix it.
There is more context in #9574 as to why the syntax is the way it is.
Remove lingering unstable flag `-Zfeatures`
From [Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/.E2.9C.94.20-Zfeatures):
> Eric Huss: When trying to validate that the feature resolver resolves features in the same way as the dependency resolver. I think it is identical, but there are a lot of complex use cases and flags that aren't covered in the testsuite so I'm not entirely confident. If we ever come across a scenario where it isn't the same, the compare option can be useful to see what differs.
In the future we could pick 61b94c3b0435250a0abb8080867b4af8d7dd88f8 to remove this Z flag.
Tweak wording
the "If, however, ..." construct reads a bit awkwardly, so tweaked it.
English is my native language, although I don't claim to be a grammar expert.
Expose libgit2-sys/vendored feature as vendored-libgit2
### What does this PR try to resolve?
Compiling cargo on MacOS can be problematic due present libraries, `cargo` already supports vendoring for openssl (and it helps https://github.com/pacak/cargo-show-asm/pull/51 to some users). This pull request attempts to extend vendoring support to libgit2 in order to fix more foreign library issues: https://github.com/pacak/cargo-show-asm/issues/50
While it is possible for me to enable it on my side (https://github.com/pacak/cargo-show-asm/pull/52) a special care needs to keep versions in sync. Exposing this feature in `cargo` directly will make such fixes for my and other crates more robust.
### How should we test and review this PR?
Disabled by default means existing behavior is unchanged, testing with it enabled to confirm if it still works should do the trick.
Ed Page
bors
Cassaundra Smith
Weihang Lo
Weihang Lo
Arlo Siemsen
Arlo Siemsen
Eric Huss
Jeff Widman
Michael Baikov
hi-rustin