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

Tracking issue for Deduplicate Cargo workspace information RFC #8415

Closed
10 of 12 tasks
Mark-Simulacrum opened this issue Jun 26, 2020 · 47 comments · Fixed by #10859
Closed
10 of 12 tasks

Tracking issue for Deduplicate Cargo workspace information RFC #8415

Mark-Simulacrum opened this issue Jun 26, 2020 · 47 comments · Fixed by #10859
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 A-workspaces Area: workspaces C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete T-cargo Team: Cargo to-announce

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jun 26, 2020

This is a tracking issue for the RFC: Deduplicate Cargo workspace information (rust-lang/rfcs#2906).

Documentation:

Steps:

Changes from RFC

  • Not locking workspace dependencies, see alexchrichton's comment
  • Not inserting a version into path dependencies when publishing, see epage's comment
  • Changed the table package fields inherit from, from workspace to workspace.package, see epage's comment
  • Fields
    • Explicitly not supporting resolver, see epage's comment
    • Supporting edition which the RFC showed inconsistent status for
    • Supporting rust-version which is newer than the RFC
    • Supporting include and exclude which the RFC couldn't, see epage's comment

Unresolved questions:

  • Do we stablize inheriting from workspace.package and workspace.dependencies or do we switch which tables we use? see epage's comment
  • Preferred documentation style: RFC uses field = { workspace = true } but TOML allows field.workspace = true
  • Is the performance good enough?
@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for something unstable. label Jun 26, 2020
@Mark-Simulacrum
Copy link
Member Author

cc @ehuss @alexcrichton -- please update the description wrt to documentation/stabilization and potentially some of the unresolved questions, I myself don't really know what Cargo procedures are like these days.

@alexcrichton
Copy link
Member

Ok I've got a moment to write up some instructions, I shall do so! I think this'll be posted on TWiR soon to see if others are interested, so I'm hoping that I can help guide someone interested to implement this.

To get started, I'd probably break this down based on the reference level explanation:

Update [workspace] parsing

First you'll probably want to implement [workspace] parsing updates.

That'll be done by updating this structure with all the relevant fields. You'll need to take care when handling the dependencies, being careful to share code with existing dependency parsing.

Once the fields are parsed you'll want to carry them over to WorkspaceRootConfig which contains the format of each field that is persisted throughout the lifetime of Cargo itself. This'll be where dependencies are elaborated (again, sharing code from before) into a Dependency structure for example.

At this point you should be able to write some tests. All our tests are in tests/testsuite/*.rs and you'll probably want to make a new test suite at something like tests/testsuite/deduplicate_workspace.rs and add mod deduplicate_workspace; to tests/testsuite/main.rs. You can then run tests with cargo test deduplicate_workspace.

Placeholder { workspace = true }

Next you can tackle workspace = true directives in Cargo.toml. The best way to implement this is probably going to be making a generic thing like:

pub enum MaybeWorkspace<T> {
    Defined(T),
    Workspace { workspace: bool },
}

If possible, the best way to handle this will be to not persist this MaybeWorkspace type all throughout Cargo. Ideally a Manifest in Cargo would retain the actual value for each manifest. I'm not sure whether this is available at this time, but in theory you'll want to "elaborate" a manifest with a WorkspaceRootConfig on-hand where you can resolve any MaybeWorkspace<T> with the &WorkspaceRootConfig next to it.

New dependency directives

To implement new dependency directives you'll update this structure along with this method. Again ideally there will be a new &WorkspaceRootConfig argument which allows resolving all workspace = true dependencies by basically clone-ing the Dependency from the workspace.

Inferring version directives

For inferring version on path dependencies that will need to happen on the packaging phase of Cargo. First you'll want to update where the error is generated today. Next you'll want to update to_registry_toml which will update the manifest (or probably a clone of the current manifest) with version numbers.


And I think that should at least be enough to get almost all the way there! I'm sure there's things that will likely come up in review and questions a long the way. The hardest part will probably be getting WorkspaceRootConfig present while parsing other manifests, that may require some refactoring in Cargo (or maybe there's a better way I'm not seeing!)

@hbina
Copy link
Contributor

hbina commented Aug 15, 2020

I am not sure if I am capable enough to work on this, but I would like to follow the development to see how things work. Does https://github.com/alexcrichton/toml-rs also needs to be updated?

@kornelski
Copy link
Contributor

The general-purpose toml crate doesn't need any changes, because this feature uses standard TOML syntax.

I will need to update my Manifest-parsing crate: https://gitlab.com/crates.rs/cargo_toml (help welcome)

@yaymukund
Copy link
Contributor

yaymukund commented Aug 23, 2020

I've started work on this.

Thank you for the super detailed writeup, @alexcrichton . You can see my progress here, but for convenience I'll keep this list updated:

EDIT: The PR is open, so please check there.

Aiming to complete this list relatively soon - hopefully tomorrow in the coming week.

@yaymukund
Copy link
Contributor

yaymukund commented Aug 26, 2020

Hello, I've been making steady progress on this and wanted to give an update:

The WorkspaceRootConfig is not available when we're parsing the manifest, so we have to locate and parse it somewhere in do_read_manifest(). Currently the logic for finding the root and parsing its Config.toml lives in struct Workspace itself and makes use of workspace.packages to cache parsed tomls.

I see a few different ways to proceed:

  1. Require a Workspace in do_read_manifest somehow. Workspace::new() currently parses the entire workspace, so this could get complicated.
  2. Extract a new ParserCache out of Workspace. It would sit in front of the Packages cache, accept a manifest_path argument, and return a deserialized TomlManifest.
  3. Ignore the packages registry entirely. This would be easiest, but worst-case would require parsing every toml in the workspace every time you parse any manifest. I don't think this is a good solution, but mention it for completeness' sake.

I'm not blocked - am currently leaning towards approach 2 - but please let me know if I'm missing anything or if there's a better way to proceed.

Edit: In case anyone's seeing this now, I went with 2 and it seems to work :)

@salewski
Copy link
Contributor

Hi Folks,

First of all, a big "thank you" to @alexcrichton, @yaymukund, and everyone else who has put in so much effort on this feature. As my projects are growing in size, being able to manage dependency versions from a single location in the workspace Cargo.toml file will be a very welcome feature.

I do have a question about the following snippet from the "New dependency directives" section of rust-lang/rfcs#2906:

New dependency directives

...

  • features - this indicates, as usual, that extra features are being enabled
    over the already-enabled features in the directive found in
    [workspace.dependencies]. The result set of enabled features is the union of
    the features specified inline with the features specified in the directive in
    the workspace table.

For now if a workspace = true dependency is specified then also specifying the
default-features value is disallowed. The default-features value for a
directive is inherited from the [workspace.dependencies] declaration, which
defaults to true if nothing else is specified.

Should the "For now..." be understood as meaning "we want to revisit this, but it's not a Day One concern"?

The reason I ask is that it seems to preclude the ability to inherit from the workspace the version number for a given dependency if one or more of the package-level configurations requires a non-default feature set in only some instances. It would be nice if a package-level dependency were to be able to benefit from the centralized configuration of the crate 'version' number spec, yet still be able to specify a non-default 'features' set locally in the package-level config.

I think variations of the 'features' specification for dependent packages could benefit quite a bit from centralized configuration at the workspace level, but the need to specify 'features' at the package-level should not preclude the ability to still inherit the 'version' number from the workspace (at least not in the long term).

Two scenarios come to mind:

  1. A workspace with some number of packages that all have a dependency on a third-party crate of the same version, and for the most part all need the same set of features enabled. However one package needs that same version of the third-party crate, but with a different set of features enabled (not just additional features).

  2. A workspace with some number of packages that all have a dependency on a third-party crate of the same version, but all of the packages need a different set of features enabled.

If I understand rust-lang/rfcs#2906 correctly:

  • In scenario (1), most of the packages will benefit from the proposed workspace enhancements: They will all use the inherited version number and the inherited features configuration from the workspace.

    However, the package that needs the different set of features from the third-party will have to fully specify the dependency, its version, and the feature set that it needs in the package's Cargo.toml file. In particular, that package will not benefit from the ability to control the dependency version at the workspace level, so will need to have that dep's version number kept in sync with the workspace Cargo.toml manually.

    We would like to write:

    # my-workspace/Cargo.toml
    [workspace]
    members = [
        'thromdimbulator1',
        'thromdimbulator2',
    ]
    ...
    [dependencies]
    [dependencies.gick]
    version = 1973.9.12
    default-features = false
    features = ['skrux']
    

    and:

    # my-workspace/thromdimbulator1/Cargo.toml
    ...
    [dependencies.gick]
    version = { workspace = true }
    # inherits: default-features = false
    # inherits: features = ['skrux']
    

    and:

    # my-workspace/thromdimbulator2/Cargo.toml
    ...
    [dependencies.gick]
    version = { workspace = true }
    default-features = false
    features = ['snoor']
    

    But instead would have to write the thromdimbulator2/Cargo.toml file as:

    # my-workspace/thromdimbulator2/Cargo.toml
    ...
    [dependencies.gick]
    # Have to explicitly specify the version, and keep it in sync with the parent workspace...
    version = 1973.9.12
    # ...because we need a custom feature set:
    default-features = false
    features = ['snoor']
    
  • In scenario(2), none of the packages benefit from the third-party dep version number declared in the workspace-level Cargo.toml file because they each need a different set of features.

    We would like to write:

    # my-workspace/Cargo.toml
    [workspace]
    members = [
        'thromdimbulator3',
        'thromdimbulator4',
        'thromdimbulator5',
    ]
    ...
    [dependencies]
    [dependencies.gick]
    version = 1973.9.12
    # no features explicitly configured at workspace level b/c each package needs a different set
    

    and:

    # my-workspace/thromdimbulator3/Cargo.toml
    ...
    [dependencies.gick]
    version = { workspace = true }
    default-features = false
    features = ['goor', 'skrux']
    

    and:

    # my-workspace/thromdimbulator4/Cargo.toml
    ...
    [dependencies.gick]
    version = { workspace = true }
    default-features = false
    features = ['goor', 'snux']
    

    and:

    # my-workspace/thromdimbulator5/Cargo.toml
    ...
    [dependencies.gick]
    version = { workspace = true }
    default-features = false
    features = ['goor', 'snoor']
    

    But instead would not be able to use version = { workspace = true } in any of the package-level Cargo.toml files. All of the package-level declarations of the dep need to have their version numbers kept in sync manually.

With the ability to inherit the 'version' number of a dep so tightly co-mingled with that dep's 'features' configuration, the workspace enhancement will be of only limited usefulness wherever different sets of features for a dep are in use within the different packages of a workspace. And since feature sets are so widely used, I would expect many users to stumble over this caveat and find the behavior counter-intuitive.

None of this makes us worse off than we are today, where we need to manually keep the version numbers in sync. Especially since we have cargo-edit, which lets us do, e.g., cargo upgrade --workspace 'gick@=1973.9.12'.

@alexcrichton
Copy link
Member

@salewski it's always something we can tweak at a later date (or even now before stabilization), but in our discussions amongst the Cargo team awhile back the conclusion was that we couldn't really think of a non-surprising way of dealing with default-features. For projects that need to toggle it we figured that you could disable default features in [workspace.dependencies] and then enable the default feature in directives as necessary. This should still give the same benefit of a workspace-level definition and dependencies enabling the default feature would be just a little more wordy.

@salewski
Copy link
Contributor

For projects that need to toggle it we figured that you could disable default features in [workspace.dependencies] and then enable the default feature in directives as necessary. This should still give the same benefit of a workspace-level definition and dependencies enabling the default feature would be just a little more wordy.

@alexcrichton Ah, I see. That's not bad at all, and addresses my main concern about managing the version numbers centrally. Thanks for clarifying that!

The piece I was missing is the fact that specifying a non-additive, non-default feature set at the package level will no longer require that default-features = false be set locally (in the package-level Cargo.toml file), as long as that value has been set at the workspace level.

I now see that it is right there in the text of the rfc that I quoted, but I suppose the way I was reading it was too heavily shaped by current practice (where default-features = false and features = [...] always appear as a pair in the non-additive case) :-)

@wycats
Copy link
Contributor

wycats commented Feb 24, 2021

This feature would be incredible 😄

@epage
Copy link
Contributor

epage commented Mar 22, 2022

cargo add is in FCP, see #10472.

Some thoughts on how we might update it once this RFC is implemented:

  • cargo add foo should fallback to looking in the workspace's dependency table
    • I'm thinking we should shorten it to a dotted-key where possible, like we do with shortening to a string for version
  • Stop inserting the version for path dependencies
    • This will remove some complication dealing with detecting when the version should be present
    • This also means we don't have to update the toml parsing to handle inheritable version
  • When showing resolved features, take the workspace's features into account
  • Don't allow setting default-features to true or false when workspace = true

Open Questions

  • Should we allow adding to the workspace dependencies? Is this a flag when adding a regular dependency and if so, what? Do we modify the workspace manifest directly and if so, how do we handle non-virtual workspaces?

@epage
Copy link
Contributor

epage commented Mar 22, 2022

In thinking about the RFC and cargo-add, I realized there might be issues with versioning. If cargo-add is handling path dependencies assuming this RFC is implemented, then that will cause problems for people with a lower MSRV than what they are using for development. Maybe relying in the inferring of a path dependencies version by cargo-add should be held off until an edition change (unsure if its proper to check the rust-version).

Are there any problems with path dependencies inferring their version more generally? Once things are published, all is good.

  • For local development on latest version, all should work.
  • If a developer adds an inferred dependency, then hopefully they have CI running with their MSRV so it can catch it. which will be a point of frustration
  • If a contributor has an older version of rust, they could see failures. This would be frustrating and discouraging for other users.
  • The last step that this might matter is for cargo publish. If the new dependency style is still in, then the person publishing will most likely be using a compatible version of cargo unless release automation tools rely on the MSRV like I'm considering for cargo-release (feat: Publish using rust-version toolchain crate-ci/cargo-release#381)

So outside of cargo-add its a mixed bag of behavior. Unsure if it reaches the "painful enough" stage to where we might want to gate this feature generally and not just for cargo-add.

bors added a commit that referenced this issue Mar 25, 2022
Part 1 of RFC2906 - Packages can inherit fields from their root workspace

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

All changes were heavily inspired by #8664 and #9684

A big thanks to both `@yaymukund` and `@ParkMyCar.` I pulled a lot of inspiration from their branches.

I would also like to thank `@alexcrichton` for all the feedback on the first attempt. It has brought this into a much better state.

All changes have been made according to the RFC as well as `@alexcrichton's` [comment](#8664 (comment)).

This is part 1 of many, as described by [this comment](#9684 (comment)), [this comment](#9684 (review)) and redefined  [by this one](#10497 (comment)).

This PR focuses on inheriting in root package, including:
- Add `MaybeWorkspace<T>` to allow for `{ workspace = true }`
- Add a new variant to `TomlDependency` to allow inheriting dependencies from a workspace
- Add `InheritableFields` so that we have somewhere to get a value from when we `resolve` a `MaybeWorkspace<T>`
  - `license_file` and `readme` are in `InheritableFields` in this part but are not `MaybeWorkspace` for reasons [described here](#10497 (comment))
- Add a method to `resolve` a `MaybeWorkspace<T>` into a `T` that can fail if we have nowhere to pull from or the workspace does not define that field
- Disallow inheriting from a different `Cargo.toml`
  - This means that you can only inherit from inside the same `Cargo.toml`, i.e. it has both a `[workspace]` and a `[package]`
  - Forcing this requirement allows us to test the implementations of `MaybeWorkspace<T>` and the new `TomlDependency` variant without having to add searching for a workspace root and the complexity with it

Remaining implementation work for the RFC
- Support inheriting in any workspace member
- Correctly inherit fields that are relative paths
  - Including adding support for inheriting `license_file`,  `readme`, and path-dependencies
-  Path dependencies infer version directive
- Lock workspace dependencies and warn when unused
- Optimizations, as needed
- Evaluate any new fields for being inheritable (e.g. `rust-version`)
- Evaluate making users of `{ workspace = true }` define the workspace they pull from in `[package]`

Areas of concern:
- `TomlDependency` Deserialization is a mess, and I could not figure out how to do it in a cleaner way without significant headaches. I ended up having to do the same thing as last time [which was an issue](#9684 (comment)).
- Resolving on a `MaybeWorkspace` feels extremely verbose currently:
  ```rust
  project.homepage.clone().map(|mw| mw.resolve(&features, "homepage", || get_ws(inheritable)?.homepage())).transpose()?,
  ```
  This way allows for lazy resolution of inheritable fields and finding a workspace (in part 2) so you do not pay a penalty for not using this feature. The other bit of good news is `&features` will go away as it is just feature checking currently.

- This feature requires some level of duplication of code as well as remaking the original `TomlManifest` which adds extra length to the changes.
- This feature also takes a linear process and makes it potentially non-linear which adds lots of complexity (which will get worse in Part 2)

Please let me know if you have any feedback or suggestions!
@Nemo157
Copy link
Member

Nemo157 commented Jul 29, 2022

The biggest downside I see to the current design is that it doesn't really reduce the boilerplate for the non-dependency keys, instead of having to have repository = "https://repo" I now have repository.workspace = true in every crate. I don't see many places I would bother using this without inheriting by default.

@epage
Copy link
Contributor

epage commented Jul 29, 2022

If you then change your repository, you then have one place to update.

I see this less about reducing boiler plate and more about having one place to edit,. so long as the boilerplate isn't too high.

In particular, I manage a good number of crates. They all follow the same pattern so whenever I improve the pattern, I update the first alphabetically. Every once in a while, I will then diff that first repo against all of the other repos, updating them. There are many times when I forget there are other crates in the repo and don't update them. This will reduce the chance of that happening.

We can always add change to implicit in a new edition if we find that it is too much.

@Nemo157
Copy link
Member

Nemo157 commented Jul 29, 2022

That is true, but mass updates of these fields are already pretty trivial sed -i 's#https://repo#https://new_repo' **/Cargo.toml.

@epage
Copy link
Contributor

epage commented Jul 29, 2022

Not all fields are as trivially updatable.

@jyn514
Copy link
Member

jyn514 commented Jul 29, 2022

One place where this is very helpful is for git dependencies. Git dependencies have a lot of "noise" (much longer than version numbers) and are hard to edit automatically. If you have a mismatch you end up with duplicate versions, because cargo doesn't know how to unify versions from different rev commits. Having a single place to edit git deps would be extremely useful.

@Nemo157
Copy link
Member

Nemo157 commented Jul 29, 2022

Yep, dependencies are very useful, I'm currently testing using this for internal workspace references, it's nice to be able to collapse from the below down to stylish-core.workspace = true.

stylish-core.path = "../core"
stylish-core.version = "0.1.0"
stylish-core.default-features = false

One other set of fields I just noticed I have duplicated everywhere is:

[package.metadata.docs.rs]
all-features = true
targets = ["x86_64-unknown-linux-gnu"]
rustdoc-args = ["--cfg", "docsrs"]

Reskimming the RFC discussion I don't see any mention of package.metadata, and something like package.metadata.docs.rs.workspace = true syntax wouldn't be possible for it as it could already have meaning inside the inner tables.

@Muscraft
Copy link
Member

That is true, but mass updates of these fields are already pretty trivial sed -i 's#https://repo#https://new_repo' **/Cargo.toml.

While some things can be trivially updated using command line tools, first-party support that is guaranteed to work is usually a better solution.


I see this less about reducing boiler plate and more about having one place to edit,. so long as the boilerplate isn't too high.

@epage is correct that this is about having one place to edit more specifically deduplicating manifest information across workspace members. Having users write repository.workspace = true makes this an opt-in feature where inheriting by default would be (more than likely) a breaking change and be opt-out. Solving both being opt-out and it being a breaking change would require extending the current implementation to be similar to:

[workspace.package]
repository = { value = "https://repo", default = true, override = false }

If that is something that should be done, I think it should be a different change set or have an RFC to discuss it. This is mostly because it is out of the scope of this RFC.


As for [package.metadata], the RFC mentions it just above this link:

For now the metadata key is explicitly left out (due to complications around merging table values), but it can always be added in the future if necessary.

bors added a commit that referenced this issue Oct 6, 2022
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 #5586 and #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 #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](#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. #10472
2. #10578
3. #10577

The remaining changes (documentation and shell completions) will follow shortly after.

Some work has already begun on this feature in #11059.

Work on this feature was carried out on the [`merge-rm`](killercup/cargo-edit@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 #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: killercup/cargo-edit#690
- Remove unused dependencies: killercup/cargo-edit#415
- Clean up caches: killercup/cargo-edit#647

### Additional information

Fixes #10520.
bors added a commit that referenced this issue Oct 6, 2022
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 #5586 and #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 #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](#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. #10472
2. #10578
3. #10577

The remaining changes (documentation and shell completions) will follow shortly after.

Some work has already begun on this feature in #11059.

Work on this feature was carried out on the [`merge-rm`](killercup/cargo-edit@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 #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: killercup/cargo-edit#690
- Remove unused dependencies: killercup/cargo-edit#415
- Clean up caches: killercup/cargo-edit#647

### Additional information

Fixes #10520.
bors added a commit that referenced this issue Oct 6, 2022
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 #5586 and #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 #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](#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. #10472
2. #10578
3. #10577

The remaining changes (documentation and shell completions) will follow shortly after.

Some work has already begun on this feature in #11059.

Work on this feature was carried out on the [`merge-rm`](killercup/cargo-edit@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 #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: killercup/cargo-edit#690
- Remove unused dependencies: killercup/cargo-edit#415
- Clean up caches: killercup/cargo-edit#647

### Additional information

Fixes #10520.
panhania added a commit to google/fleetspeak-rs that referenced this issue Jun 29, 2023
It was initially removed because [RFC #2906][1] mentioned it is no
longer necessary and the version is inferred. However, the version that
was actually stabilized does not include this change. For more details
details, see the "Changes from RFC" section of the [Cargo issue][2].

[1]: rust-lang/rfcs#2906
[2]: rust-lang/cargo#8415
bors bot pushed a commit to sigp/lighthouse that referenced this issue Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: Michael Sproul <[email protected]>
bors bot pushed a commit to sigp/lighthouse that referenced this issue Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: Michael Sproul <[email protected]>
bors bot pushed a commit to sigp/lighthouse that referenced this issue Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: Michael Sproul <[email protected]>
bors bot pushed a commit to sigp/lighthouse that referenced this issue Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: Michael Sproul <[email protected]>
bors bot pushed a commit to sigp/lighthouse that referenced this issue Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: Michael Sproul <[email protected]>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after sigp#4639 get's merged.
closes sigp#4651


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: Michael Sproul <[email protected]>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
Synchronize dependencies and edition on the workspace `Cargo.toml`

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after sigp#4639 get's merged.
closes sigp#4651

Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: Michael Sproul <[email protected]>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
Synchronize dependencies and edition on the workspace `Cargo.toml`

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after sigp#4639 get's merged.
closes sigp#4651

Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: Michael Sproul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 A-workspaces Area: workspaces C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging a pull request may close this issue.