Merge cargo-rm into cargo

[epage]

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)

[epage]

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

[epage]

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

[epage]

(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

[cassaundra]

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.

[cassaundra]

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)

[epage]

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.

[cassaundra]

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.

[epage]

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)

[cassaundra]

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?

[epage]

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.

[cassaundra]

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.

[epage]

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?

[cassaundra]

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.

[cassaundra]

When #711 is merged, the --target flag can be checked off the to-do list.

[cassaundra]

After #712 is merged, I will open another pull request for cargo rm dry run.

[cassaundra]

I'm going to start keeping track of my own task list here:

• 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)
• Port tests to snapbox (like refactor(tests): Reduce reliance on CARGO_IS_TEST #659)
• Create merge-rm branch
• Refactor to match cargo's directory structure
• Port tests to cargo-test-support
• Optionally have set-version update Cargo.lock #698
• 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)

[cassaundra]

Starting to work on merge-rm now.

[epage]

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

[cassaundra]

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.

[epage]

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

[cassaundra]

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?

[epage]

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.

[cassaundra]

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

[cassaundra]

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

$ 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.

1. feat: Import cargo-add into cargo rust-lang/cargo#10472
2. Document cargo-add rust-lang/cargo#10578
3. Completion support for cargo-add rust-lang/cargo#10577

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.

[cassaundra]

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.

[epage]

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?

[epage]

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

[cassaundra]

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! :)

[epage]

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

[cassaundra]

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

[epage]

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.