Clean up workspace dependencies after cargo remove #11242

cassaundra · 2022-10-15T22:41:59Z

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.

rust-highfive · 2022-10-15T22:42:02Z

r? @weihanglo

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

src/cargo/ops/cargo_remove.rs

cassaundra · 2022-10-23T21:56:45Z

Recent push should fix these issues. No rush though, I know you've got a lot on your plate! :)

src/cargo/ops/cargo_remove.rs

epage · 2022-10-24T17:10:41Z

src/cargo/ops/cargo_remove.rs

+        if let Some(parent_manifest) = parent_manifest {
+            cargo_util::paths::write(
+                options.workspace.root_manifest(),
+                parent_manifest.to_string().as_bytes(),
+            )?;
+        }


I feel like this approach is fairly complicated

Managing two different workspace manifests with different lifetimes

Predicting our action as we try to see if something is unused

I feel like this would be a lot cleaner if we just accumulated the removed workspace dependency names and removed them all at once at the end. This would be a gc step working off of what the workspace does in practice and not what we've predicted it will look like.

This still has a lot of the complexity I'm concerned about by trying to predict when a dependency is no longer needed for removal. imo we shouldn't call dependency_in_workspace in the loop but instead just accumulate into to_remove_workspace if dependency_is_workspace. In the follow-up loop we then check dependency_in_workspace and remove from workspace.dependencies.

This still has a lot of the complexity I'm concerned about by trying to predict when a dependency is no longer needed for removal. imo we shouldn't call dependency_in_workspace in the loop but instead just accumulate into to_remove_workspace if dependency_is_workspace. In the follow-up loop we then check dependency_in_workspace and remove from workspace.dependencies.

Oh yes I agree, I didn't mean for the pushed commit to be reviewed yet. I didn't realize you'd be notified. It should be better now.

cassaundra · 2022-10-27T02:20:42Z

I believe @epage will be unavailable for the time being, so @weihanglo do you think you could take over reviewing this?

It'd be nice to get this and #11194 (blocked on this PR) merged before the upcoming 1.66 cutoff so that it lands with cargo remove.

weihanglo · 2022-10-27T03:03:42Z

Absolutely yes! Will review it ASAP.

r? @weihanglo

src/cargo/ops/cargo_remove.rs

epage · 2022-10-28T01:49:37Z

src/bin/cargo/commands/remove.rs

+                    .into_iter()
+                    .map(String::from)
+                    .collect::<Vec<_>>();
+                dep_is_workspace(&d.package_name(), &dep_table, manifest)


Shouldn't this be d.name_in_toml()?

epage · 2022-10-28T01:52:32Z

src/bin/cargo/commands/remove.rs

+    members.iter().any(|(pkg, manifest)| {
+        pkg.dependencies()
+            .iter()
+            .filter(|d| d.name_in_toml() == dep)
+            .map(|d| (d, dep_to_table(d)))
+            .any(|(d, t)| {


nit: for where the dependency was removed, we are relying on the undocumented behavior for when dep_is_workspace does not find the promised dependency. Ideally, we would make this explicit. This could be done either by

Having dep_is_workspace return Option<bool>

Iterate over the dependencies in manifest rather than pkg. LocalManifest should have fairly friendly accessors to help with that

Personally, I would prefer that both happen. If both are done, we could do a .expect() on dep_is_workspace or turn it into an internal error.

Doing the first should be easy. It's important to note that dep_is_workspace will return None on the recently removed dependency (as it hasn't been updated in the Workspace but has in the manifest), so doing unwrap_or(false) would make more sense. That's essentially what is going on right now, although poorly documented.

We could inspect each manifest with LocalManifest rather than Package, but that would require adding support for manually searching through dependencies, dev-dependencies, target.xyz.build-dependencies, etc. Using Package/Dependency was an intentional choice to avoid this. Do you still think it is worth the rewrite?

hat would require adding support for manually searching through dependencies, dev-dependencies, target.xyz.build-dependencies, etc.

Manifest::get_sections will let us iterate over all dependency sections and then we just need to check if the returned table-likes contain the dependency key

epage · 2022-10-28T01:56:43Z

tests/testsuite/cargo_remove/workspace/out/Cargo.toml

+[workspace]
+members = [ "my-member" ]
+
+[package]
+name = "my-package"
+version = "0.1.0"


We should probably have at least one test where we ensure workspace.dependencies.semver is not removed.

Done.

The workspace tests are now as follows:

workspace (previously workspace_virtual): virtual workspace

workspace_non_virtual (previously workspace): non-virtual workspace

workspace_preserved: non-virtual workspace which does not modify [workspace.dependencies]

cassaundra · 2022-10-31T04:12:15Z

The failing tests seem to be spurious and unrelated, although I don't have a way to retry.

epage · 2022-10-31T12:17:47Z

The failing tests seem to be spurious and unrelated, although I don't have a way to retry.

See https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/latest.20nightly.20broken

epage · 2022-11-01T12:44:03Z

CI is fixed and you can see fresh results if you rebase.

cassaundra · 2022-11-01T19:41:16Z

Okay, great! Tests seem to be passing now.

epage · 2022-11-01T20:15:48Z

@bors r+

bors · 2022-11-01T20:15:51Z

📌 Commit 38b23d5 has been approved by epage

It is now in the queue for this repository.

bors · 2022-11-01T20:15:58Z

⌛ Testing commit 38b23d5 with merge 65b2149...

bors · 2022-11-01T21:03:04Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 65b2149 to master...

14 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..810cbad9a123ad4ee0a55a96171b8f8478ff1c03 2022-10-27 15:20:57 +0000 to 2022-11-02 21:04:31 +0000 - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300)

Update cargo 14 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..810cbad9a123ad4ee0a55a96171b8f8478ff1c03 2022-10-27 15:20:57 +0000 to 2022-11-02 21:04:31 +0000 - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300) r? `@ghost`

20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b 2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000 - chore: Upgrade dependencies (rust-lang/cargo#11328) - Clean more aggressively in CI (rust-lang/cargo#11335) - Remove remove_dir_all (rust-lang/cargo#11333) - test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327) - Revert rust-lang/cargo#11183 (rust-lang/cargo#11331) - fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332) - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300)

Update cargo 20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b 2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000 - chore: Upgrade dependencies (rust-lang/cargo#11328) - Clean more aggressively in CI (rust-lang/cargo#11335) - Remove remove_dir_all (rust-lang/cargo#11333) - test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327) - Revert rust-lang/cargo#11183 (rust-lang/cargo#11331) - fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332) - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300) r? `@ghost`

weihanglo · 2022-11-05T11:08:36Z

Should we backport this to 1.66 beta?

epage · 2022-11-05T12:00:18Z

Bah, we didn't make the cut off?

We also have #11194 which is almost done. Probably a topic for the next team meeting of whether

Let these come in the next release
Beta backport these
Mark cargo-remove as unstable in the beta, removing the cargo-rm alias (to not interfere with cargo-edit)

If we are fine with these behavior changes after 1.66 (which I think I am fine with), I lean towards letting them go into 1.67. I tend to treat the bar for backports as high but am open to where others set the bar.

ehuss · 2022-11-08T19:29:40Z

We discussed the options in the meeting, and I think we are comfortable with just continuing with this and #11194 landing in the next release. I think the impact should be small, and is unlikely to be a significant problem for users. That is:

This issue AFAIK just leaves a stale dependency in workspace.dependencies. That shouldn't cause any warnings or errors, and shouldn't be a significant problem for users. This would also require the combination of someone using cargo remove plus workspace inheritance plus removing the last remaining reference. I suspect that combination is a very small population of users.
Clean profile, patch, and replace in cargo remove #11194 may cause errors, but I think the combination of someone using cargo remove and using a patch or profile override is going to be extremely small (if not zero). And the consequence is not terrible, they will just get a warning about them being unused, and I think the user should be able to figure out what is going on. My comment at Import cargo remove into cargo #11099 (comment) about gc'ing unused things wasn't intended to be a high-priority issue, and I think it's find that it was deferred.

rust-highfive assigned weihanglo Oct 15, 2022

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

cassaundra commented Oct 15, 2022

View reviewed changes