Clean profile, patch, and replace in cargo remove #11194

cassaundra · 2022-10-08T00:51:11Z

What does this PR try to resolve?

This PR is part of the continued work on cargo remove (#11099, see deferred work).

After a successful removal of a dependency, clean up the profile, patch, and replace sections to remove all references to it.

Note the GC process was expanded to clean up not just references to the dependencies just removed, but also references of all dependencies. This was because there's not an easy way to determine which dependencies correspond to the given TOML keys, without either 1) figuring that out before the removal (therefore having to predict the behavior), or 2) returning that information from the remove function (somewhat unorthodox for an op).

How should we review and test this PR?

Verify that the implementation makes sense and that the tests are sufficient.

rust-highfive · 2022-10-08T00:51:14Z

r? @epage

(rust-highfive has picked a reviewer for you, use r? to override)

src/cargo/util/toml_mut/manifest.rs

Clean up workspace dependencies after cargo remove ### What does this PR try to resolve? After successful removal of an inherited dependency from a workspace member, clean up the root workspace manifest. This PR is part of the continued working on cargo remove (#11099, see deferred work). ### How should we test and review this PR? Make sure the tests cover all possible use cases. After posting this PR, I will post a short self-review regarding some design concerns. ### Additional information #11194 is currently blocked on this feature.

cassaundra · 2022-11-04T00:38:35Z

No longer blocked on #11242, so I've (hopefully) finished up the work on this feature. Here's what's new:

replace GC moved out to the same level as workspace.dependencies GC. Both now share some code, although I think this could be cleaned up some more.
replace GC now takes into account version requirements. It should also operate on the actual package name (not the TOML name).
cargo_remove::gc_replace test is now based on a workspace to test this functionality.

src/cargo/util/toml_mut/manifest.rs

src/bin/cargo/commands/remove.rs

cassaundra · 2022-11-08T19:33:37Z

Okay, I fixed a these issues last night, but I found another bug pertaining to the replace section: since get_dependency_versions takes a TOML key and returns associated Dependencys, dependencies with a package field get ignored. I'll see if I can find a solution for this and commit then.

cassaundra · 2022-11-09T08:00:27Z

After more investigation, it turns out profile and patch also have similar issues. The profile override also needs to support the package ID spec as well.

cassaundra · 2022-11-10T08:56:50Z

Issues have been fixed. A few notes to help with review:

Workspace dependencies work off of TOML keys, whereas patch, profile, and replace match on package names.
Replace and patch overrides take package ID spec, so need to be matched both on package name and version.

src/cargo/ops/cargo_remove.rs

src/bin/cargo/commands/remove.rs

tests/testsuite/cargo_remove/gc_profile/in/Cargo.toml

bors · 2022-11-15T00:32:52Z

☔ The latest upstream changes (presumably #11375) made this pull request unmergeable. Please resolve the merge conflicts.

src/bin/cargo/commands/remove.rs

cassaundra · 2022-11-16T06:23:20Z

Changes made.

I also noticed that workspace dependencies might not be properly handled by the likes of spec_has_match/source_has_match, so I modified the workspace.dependencies pass to replace each instance of these with the corresponding definition.

src/bin/cargo/commands/remove.rs

After a successful removal of a dependency, clean up the profile, patch, and replace sections to remove all references to the dependency.

epage · 2022-11-19T02:46:34Z

@bors r+

bors · 2022-11-19T02:46:36Z

📌 Commit 299252c has been approved by epage

It is now in the queue for this repository.

bors · 2022-11-19T02:46:49Z

⌛ Testing commit 299252c with merge 43689d3...

bors · 2022-11-19T03:47:16Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 43689d3 to master...

7 commits in eb5d35917b2395194593c9ca70c3778f60c1573b..ba607b23db8398723d659249d9abf5536bc322e5 2022-11-17 22:08:43 +0000 to 2022-11-22 20:52:39 +0000 - Fix failure to parse rustc's JSON output if it is too nested (rust-lang/cargo#11368) - Add suggestions when `cargo add` multiple packages (rust-lang/cargo#11186) - Update mod.rs (rust-lang/cargo#11395) - Fix typo `try use` -> `try to use` (rust-lang/cargo#11394) - Add warning when `cargo tree -i <spec>` can not find packages (rust-lang/cargo#11377) - Clean profile, patch, and replace in cargo remove (rust-lang/cargo#11194) - chore: Upgrade miow (rust-lang/cargo#11391)

Update cargo 7 commits in eb5d35917b2395194593c9ca70c3778f60c1573b..ba607b23db8398723d659249d9abf5536bc322e5 2022-11-17 22:08:43 +0000 to 2022-11-22 20:52:39 +0000 - Fix failure to parse rustc's JSON output if it is too nested (rust-lang/cargo#11368) - Add suggestions when `cargo add` multiple packages (rust-lang/cargo#11186) - Update mod.rs (rust-lang/cargo#11395) - Fix typo `try use` -> `try to use` (rust-lang/cargo#11394) - Add warning when `cargo tree -i <spec>` can not find packages (rust-lang/cargo#11377) - Clean profile, patch, and replace in cargo remove (rust-lang/cargo#11194) - chore: Upgrade miow (rust-lang/cargo#11391)

rust-highfive assigned epage Oct 8, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2022

epage reviewed Oct 9, 2022

View reviewed changes

src/cargo/util/toml_mut/manifest.rs Outdated Show resolved Hide resolved

src/cargo/util/toml_mut/manifest.rs Outdated Show resolved Hide resolved

src/cargo/util/toml_mut/manifest.rs Outdated Show resolved Hide resolved

cassaundra mentioned this pull request Oct 9, 2022

Merge cargo-rm into cargo killercup/cargo-edit#682

Open

14 tasks

cassaundra force-pushed the cargo-remove-gc branch from e82c234 to 081d656 Compare October 9, 2022 22:44

cassaundra commented Oct 9, 2022

View reviewed changes

src/cargo/util/toml_mut/manifest.rs Outdated Show resolved Hide resolved

cassaundra force-pushed the cargo-remove-gc branch 2 times, most recently from 7ee5928 to b0b86a3 Compare October 9, 2022 22:57

epage added S-blocked and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2022

epage mentioned this pull request Oct 10, 2022

Import cargo remove into cargo #11099

Merged

5 tasks

cassaundra mentioned this pull request Oct 15, 2022

Clean up workspace dependencies after cargo remove #11242

Merged

cassaundra force-pushed the cargo-remove-gc branch from b0b86a3 to dd1cd3a Compare November 4, 2022 00:33

epage reviewed Nov 4, 2022

View reviewed changes

src/cargo/util/toml_mut/manifest.rs Outdated Show resolved Hide resolved

epage reviewed Nov 4, 2022

View reviewed changes

src/bin/cargo/commands/remove.rs Outdated Show resolved Hide resolved

epage removed the S-blocked label Nov 5, 2022

cassaundra force-pushed the cargo-remove-gc branch from dd1cd3a to b292a75 Compare November 10, 2022 08:56

epage reviewed Nov 10, 2022

View reviewed changes

bors added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Nov 15, 2022

cassaundra force-pushed the cargo-remove-gc branch 3 times, most recently from c05bd27 to 5d3ed21 Compare November 15, 2022 09:21

epage approved these changes Nov 15, 2022

View reviewed changes

src/bin/cargo/commands/remove.rs Outdated Show resolved Hide resolved

src/bin/cargo/commands/remove.rs Outdated Show resolved Hide resolved

src/bin/cargo/commands/remove.rs Show resolved Hide resolved

cassaundra force-pushed the cargo-remove-gc branch from 5d3ed21 to c3ba61c Compare November 16, 2022 06:23

epage reviewed Nov 16, 2022

View reviewed changes

src/bin/cargo/commands/remove.rs Show resolved Hide resolved

cassaundra force-pushed the cargo-remove-gc branch from c3ba61c to 2056f8d Compare November 18, 2022 04:54

Clean profile, patch, and replace in cargo remove

299252c

After a successful removal of a dependency, clean up the profile, patch, and replace sections to remove all references to the dependency.

epage force-pushed the cargo-remove-gc branch from 2056f8d to 299252c Compare November 18, 2022 15:34

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Nov 19, 2022

bors merged commit 43689d3 into rust-lang:master Nov 19, 2022

weihanglo mentioned this pull request Nov 23, 2022

Update cargo rust-lang/rust#104752

Merged

ehuss added this to the 1.67.0 milestone Dec 14, 2022

epage mentioned this pull request Jul 31, 2023

cargo rm unexpectedly removes patches #12419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean profile, patch, and replace in cargo remove #11194

Clean profile, patch, and replace in cargo remove #11194

cassaundra commented Oct 8, 2022 •

edited by epage

Loading

rust-highfive commented Oct 8, 2022

cassaundra commented Nov 4, 2022

cassaundra commented Nov 8, 2022

cassaundra commented Nov 9, 2022

cassaundra commented Nov 10, 2022

bors commented Nov 15, 2022

cassaundra commented Nov 16, 2022

epage commented Nov 19, 2022

bors commented Nov 19, 2022

bors commented Nov 19, 2022

bors commented Nov 19, 2022

Clean profile, patch, and replace in cargo remove #11194

Clean profile, patch, and replace in cargo remove #11194

Conversation

cassaundra commented Oct 8, 2022 • edited by epage Loading

What does this PR try to resolve?

How should we review and test this PR?

rust-highfive commented Oct 8, 2022

cassaundra commented Nov 4, 2022

cassaundra commented Nov 8, 2022

cassaundra commented Nov 9, 2022

cassaundra commented Nov 10, 2022

bors commented Nov 15, 2022

cassaundra commented Nov 16, 2022

epage commented Nov 19, 2022

bors commented Nov 19, 2022

bors commented Nov 19, 2022

bors commented Nov 19, 2022

cassaundra commented Oct 8, 2022 •

edited by epage

Loading