Merge cargo-rm into cargo

A scratchpad like #568

Ordered list:

Update CLI flags to match the merged cargo-add
Add --target support (Add --target flag to cargo-rm #711)
Dry-run support
First letter of warnings/errors should be lower case
Change CRATE in help to DEP to clarify that we want the dependency name and not the crate name (in case of a rename)
Create merge-rm branch
Refactor to match cargo's directory structure
Port tests to snapbox (like refactor(tests): Reduce reliance on CARGO_IS_TEST #659)
Port tests to cargo-test-support
Refactor out of cargos ops/cargo_add/manifest.rs into a place for reuse
- Possibilities: ops/cargo_edit or edit and make it public
Port to cargo's internals
Merge the command (like in feat: Import cargo-add into cargo rust-lang/cargo#10472)
Update documentation (like in Document cargo-add rust-lang/cargo#10578)
Update completions (like in Completion support for cargo-add rust-lang/cargo#10577)

Currently:

cargo-rm 0.9.0
Remove a dependency from a Cargo.toml manifest file

USAGE:
    cargo rm [OPTIONS] <CRATE>...

ARGS:
    <CRATE>...    Crates to be removed

OPTIONS:
    -B, --build                   Remove crate as build dependency
    -D, --dev                     Remove crate as development dependency
    -h, --help                    Print help information
        --manifest-path <PATH>    Path to the manifest to remove a dependency from
    -p, --package <PKGID>         Package id of the crate to remove this dependency from
    -q, --quiet                   Do not print any output in case of success
    -V, --version                 Print version information
    -Z <FLAG>                     Unstable (nightly-only) flags

Only operates on one crate

The positional argument is list of dependency keys to remove

Cleans up features when done

Out of scope:

cargo remove should clean up removed dependencies from the cache #647

Too immature

RFC: cargo rm --unused #415

Move to cargo:

[feature] cargo rm remove deps with a dash instead of an underscore #690

Fix before merging

Optionally have set-version update Cargo.lock #698

(Python) poetry remove

Dry run support

(Javascript) yarn remove

Supports wildcards

(Javascript) pnpm remove

(Go) go get

go get foo@none to remove

(Julia) pkg rm

(Ruby) bundle remove

(Dart) dart pub remove

@epage

Hello! Per some conversations with @epage, I will be working on this feature for the time being. I'll start leaving my notes on this issue as well.

Expanded list of prior art:

(Python) poetry remove

Supports dry run

(JavaScript) yarn remove

Supports wildcards

(JavaScript) pnpm remove

(Go) go get

go get foo@none to remove

(Julia) pkg rm

Supports --all to remove all dependencies

(Ruby) bundle remove

(Dart) dart pub remove

Supports dry run

(Lua) luarocks remove

Supports force remove

(.NET) Uninstall-Package

Supports dry run
Supports removal of dependencies
Supports force remove (disregards dependencies)

(Haxe) haxelib remove

(Racket) raco pkg remove

Supports dry run
Supports force remove (disregards dependencies)
Supports demotion to weak dependency (sort of a corollary of force remove)

Happen to know what distinguishes a force-remove from a remove?

I'm also a bit baffled as to why some tools provide an --all. Seems unlikely someone will go nuclear with their dependencies like that.

Happen to know what distinguishes a force-remove from a remove?

If another dependency depends on the package to be removed, it seems the default behavior in these programs would be to raise an error. A force remove disregards that error, and removes the package anyway.

I'm pretty confident this behavior wouldn't exist in Cargo, since removing a dependended-upon crate from the manifest would still keep the dependency in the graph, just not as a top-level dependency. That being said, it would be good to know whether there are any edge cases in which removing a dependency could result in a dependency resolution issue.

I'm also a bit baffled as to why some tools provide an --all. Seems unlikely someone will go nuclear with their dependencies like that.

I thought it was weird too! My intuition here is that it could be useful in some sort of interactive environment, like a REPL. Looking at the original PR for --all in Julia (JuliaLang/Pkg.jl#2432), it seems the flag was added to several other subcommands at the same time, so it's probably just a fun artifact that arose out of that.

That being said, it would be good to know whether there are any edge cases in which removing a dependency could result in a dependency resolution issue.

Right now, we always remove even if it is a known breaking change. I wonder if we the following cases should require force-remove

Removing an optional dependency that is exposed as a feature
Removing a public dependency (RFC only at this time, its not implemented)

Removing an optional dependency that is exposed as a feature

That makes sense to me.

Removing a public dependency (RFC only at this time, its not implemented)

Good to know. I suppose if any progress is made on that front, we could revisit this?

Good to know. I suppose if any progress is made on that front, we could revisit this?

I'm assuming cargo remove will be merged first, so we'd just make a note on the public dependency tracking issue that this needs to be resolved.

Do you think breaking changes should be considered opt-in (with --force) or opt-out (with something like --no-breaking)? Not exactly sure what the user would expect out of this situation.

For that matter, adding a dependency can in some cases result in a breaking change, so I worry that it runs the risk of setting a precedent that we can't actually uphold.

Do you think breaking changes should be considered opt-in (with --force) or opt-out (with something like --no-breaking)? Not exactly sure what the user would expect out of this situation.

If we have the feature at all, I feel like it should be --force. Opting in to not break feels a little weird and generally when preventing harm you do it by default. I'm not entirely sold on the feature though. I'd say go implement it or not as you see fit and we'll include it in the proposal to merge the command for discussing.

adding a dependency can in some cases result in a breaking change

How so?

adding a dependency can in some cases result in a breaking change

How so?

It's definitely more of a hard-to-detect edge case, but from the SemVer docs:

It is usually safe to add new dependencies, as long as the new dependency does not introduce new requirements that result in a breaking change. For example, adding a new dependency that requires nightly in a project that previously worked on stable is a major change.

Starting to work on merge-rm now.

A quick thought: could we switch the test styles before dropping into the merge-rm branch?

For example, that would let us fix #698 before we fork development if that is considered needed

A quick thought: could we switch the test styles before dropping into the merge-rm branch?

That's fine! I will update the task list. I'll let you know if I have any questions.

rust-lang/cargo#10902 is moving forward, so we should do similar (part of #698)

Also, should we GC workspace.dependencies when they are no longer used? See rust-lang/rfcs#2906 for more context

rust-lang/cargo#10902 is moving forward, so we should do similar (part of #698)

Okay! Sounds good. Is that something that we should do before or after merge-rm do you think? I see you're using cargo's cargo::resolve_ws for that.

Also, should we GC workspace.dependencies when they are no longer used? See rust-lang/rfcs#2906 for more context

By this, do you mean having the workspace.dependencies entry be removed after no workspace members depend on it? I can see where that'd be useful for some people, but I think that might be too aggressive of a change in that the user might not expect something to be changed outside of the specific crate they're working in.

What do you think about a --workspace-like flag for add and remove that operates specifically on workspace.dependencies?

Okay! Sounds good. Is that something that we should do before or after merge-rm do you think? I see you're using cargo's cargo::resolve_ws for that.

Whereever possible, we should make changes before forking so people can use them immediately and there is less "going dark". In this case, you can use cargo metadata for it as well, see https://github.com/killercup/cargo-edit/pull/756/files

By this, do you mean having the workspace.dependencies entry be removed after no workspace members depend on it? I can see where that'd be useful for some people, but I think that might be too aggressive of a change in that the user might not expect something to be changed outside of the specific crate they're working in.

What do you think about a --workspace-like flag for add and remove that operates specifically on workspace.dependencies?

--workspace generally means "apply to all workspace members" and I'd rather than not use it in a different context and confuse people.

In general, we have to weigh

Not having the functionality
Having it without a flag
Having it with a flag

Flags come with a usability cost. The --help output grows and users have to dig through more to find what they actually want. We are effectively presenting a choice to a user and have to make sure they have all the information for making a decision.

So with that, we need to consider if the feature is worth the cost of a flag. In some cases, it might be better to not have the functionality at all. In other cases, it might be better to make it unconfigurable.

In this particular case, some things to keep in mind

We already automatically remove the dependency from feature lists
The original workspace RFC called for it to be a hard error to have a workspace.dependencies entry that isn't referenced. We dropped it due to not being worth the implementation cost.
Users generally are using SCM these days which will help them identify changes and allow them to revert them if needed.

Other options

We do a shell_note when we remove it
We do a shell_note telling the user they can remove it

btw workspace.dependencies doesn't get stablized for another 5 weeks. We are not currently setup for testing nightly features, so depending on our timeline, this might be in merge-rm rather than master.

That makes sense to me. Since this will probably be in merge-rm anyway, we could make that something to solicit feedback on later.

@epage

Title: Import cargo-remove into cargo

What does this PR try to resolve?

This PR merges cargo remove from cargo-edit into cargo.

Motivation

General approval from community, see Add cargo-add (from cargo-edit) to cargo proper rust-lang/cargo#5586 and Add cargo-rm (from cargo-edit) to cargo proper rust-lang/cargo#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 rust-lang/cargo#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). 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

$ 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

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.

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

This is following some prep work done in Some work has already begun on this feature in rust-lang/cargo#11059.

Work on this feature was carried out on the 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
- Supports dry run
JavaScript: yarn remove
- Supports wildcards
JavaScript: pnpm remove
Go: go get
- go get foo@none to remove
Julia: pkg rm
- Supports --all to remove all dependencies
Ruby: bundle remove
Dart: dart pub remove
- Supports dry run
Lua: luarocks remove
- Supports force remove
.NET: Uninstall-Package
- Supports dry run
- Supports removal of dependencies
- Supports force remove (disregards dependencies)
Haxe: haxelib remove
Racket: raco pkg remove
- 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), 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, 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 Tracking issue for Deduplicate Cargo workspace information RFC rust-lang/cargo#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: [feature] cargo rm remove deps with a dash instead of an underscore #690
Remove unused dependencies: RFC: cargo rm --unused #415
Clean up caches: cargo remove should clean up removed dependencies from the cache #647

Additional information

Fixes #10520.

I've posted a draft of the PR body above. There's still some stuff I want to add/polish, but I thought I'd post it sooner rather than later in case you have any feedback.

Thanks for posting this!

Some quick thoughts

The motivation section should be focused on end-users. "Minimal overhead to implement" and "Precedent set with cargo-add to introduce cargo-edit subcommands to cargo" are more implementation focused. The first might better fit in talking about why it isn't a draw back. The latter doesn't quite fit into the discussion. Something being in cargo-edit doesn't necessarily mean it should be merged. For example. I'm unsure of cargo-set-version
Another potential motivation is helping users clean up their manifests. For example, a user might forget to delete an optional dependency from their features list, get an error, and have to go back. This is now automatic
I don't see the Drawback section carried over. Particularly consider the impact to a resourced scrapped team to be taking this on as a drive-by contributor
The behavior section should call out
- That it focuses on the dependency name, rather than the crate name, like cargo add
- That it will automatically remove itself from the feature list
Should the lock file stuff be moved to behavior? Also, not entirely sure we need all of that in here.
Should the rename be called out in the behavior section instead?
Some other future possibilities
- Adding in a cargo rm alias
- Garbage collecting workspace dependencies when they are no longer used
Because insta-stablizatrion isn't required due to the rename, are you expecting to have this be unstable or do you want to add reasons why it should still be insta-stablized?

Defer section

Docs
GC?

Docs deferral

Simplify the review by decoupling the code and documentation conversations
cargo-add specific: too much behavior was changing during the review
documentation changes can only happen in the cargo repo but all other review feedback was handled in the cargo-edit repo

I've updated the PR draft body above and started staging changes in a cargo fork (compare changes). I still have yet to do a final pass on both of these (I have to head out shortly), but they should be in a final-ish state. Let me know if you have any questions. Many thanks! :)

I've made some tweaks directly to the draft. Good to go!

Will need to backport changes from rust-lang/cargo#11194 when complete.

No, we'll be deprecating cargo rm in cargo-edit and removing it at a later point.

We wanted to start dev in master because we wanted to get things into people's hands ASAP and to collect feedback but the value of backporting changes isn't worth the effort especially when the command will be disappearing soon.

Navigation Menu

Repository navigation

Merge cargo-rm into cargo #682

3 remaining items

What does this PR try to resolve?

Motivation

Drawbacks

Behavior

Help output

Example usage

How should we test and review this PR?

Prior art

Insta-stabilization

Deferred work

Future Possibilities

Additional information

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Merge cargo-rm into cargo #682

Description

Activity

epage commented on Mar 29, 2022

epage commented on Mar 29, 2022

epage commented on Mar 29, 2022

cassaundra commented on Jun 10, 2022

cassaundra commented on Jun 10, 2022

epage commented on Jun 10, 2022

cassaundra commented on Jun 12, 2022

epage commented on Jun 13, 2022

cassaundra commented on Jun 16, 2022

epage commented on Jun 16, 2022

cassaundra commented on Jun 16, 2022

epage commented on Jun 16, 2022

cassaundra commented on Jun 16, 2022

3 remaining items

cassaundra commented on Jul 24, 2022

epage commented on Jul 25, 2022

cassaundra commented on Jul 25, 2022

epage commented on Jul 27, 2022

cassaundra commented on Aug 16, 2022

epage commented on Aug 16, 2022

cassaundra commented on Aug 16, 2022

cassaundra commented on Sep 14, 2022

What does this PR try to resolve?

Motivation

Drawbacks

Behavior

Help output

Example usage

How should we test and review this PR?

Prior art

Insta-stabilization

Deferred work

Future Possibilities

Additional information

cassaundra commented on Sep 14, 2022

epage commented on Sep 15, 2022

epage commented on Sep 15, 2022

cassaundra commented on Sep 15, 2022

epage commented on Sep 16, 2022

cassaundra commented on Oct 9, 2022

epage commented on Oct 10, 2022

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions