clean: Add --workspace support

[osiewicz]

Co-authored-by: dino dinojoaocosta@gmail.com

What does this PR try to resolve?

Fixes #14720.

How to test and review this PR?

We tested this change manually on Ruff repository. We've also added a test¹.

Footnotes

1. During testing we've found that Workspace::members returns path dependencies of workspace members as workspace members; this means that cargo clean --workspace will clean up artifacts of path dependencies as well. We are not sure if that's a good behaviour and would love to get some more guidance on it. ↩

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[epage]

Note we rather have atomic commits for when things are merged rather than having small review feedback steps, see https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request

[weihanglo]

1. During testing we've found that Workspace::members returns path dependencies of workspace members as workspace members; this means that cargo clean --workspace will clean up artifacts of path dependencies as well. We are not sure if that's a good behaviour and would love to get some more guidance on it. ↩

This is a documented behavior, so they should be pruned as well.

All path dependencies residing in the workspace directory automatically become members. Additional members can be listed with the members key, which should be an array of strings containing directories with Cargo.toml files.

See also

cargo/src/cargo/core/workspace.rs

Lines 860 to 914 in 745aae6

	fn find_path_deps(
	&mut self,
	manifest_path: &Path,
	root_manifest: &Path,
	is_path_dep: bool,
	) -> CargoResult<()> {
	let manifest_path = paths::normalize_path(manifest_path);
	if self.members.contains(&manifest_path) {
	return Ok(());
	}
	if is_path_dep && self.root_maybe().is_embedded() {
	// Embedded manifests cannot have workspace members
	return Ok(());
	}
	if is_path_dep
	&& !manifest_path.parent().unwrap().starts_with(self.root())
	&& self.find_root(&manifest_path)? != self.root_manifest
	{
	// If `manifest_path` is a path dependency outside of the workspace,
	// don't add it, or any of its dependencies, as a members.
	return Ok(());
	}
	if let WorkspaceConfig::Root(ref root_config) =
	*self.packages.load(root_manifest)?.workspace_config()
	{
	if root_config.is_excluded(&manifest_path) {
	return Ok(());
	}
	}
	debug!("find_path_deps - {}", manifest_path.display());
	self.members.push(manifest_path.clone());
	let candidates = {
	let pkg = match *self.packages.load(&manifest_path)? {
	MaybePackage::Package(ref p) => p,
	MaybePackage::Virtual(_) => return Ok(()),
	};
	self.member_ids.insert(pkg.package_id());
	pkg.dependencies()
	.iter()
	.map(|d| (d.source_id(), d.package_name()))
	.filter(|(s, _)| s.is_path())
	.filter_map(|(s, n)| s.url().to_file_path().ok().map(|p| (p, n)))
	.map(|(p, n)| (p.join("Cargo.toml"), n))
	.collect::<Vec<_>>()
	};
	for (path, name) in candidates {
	self.find_path_deps(&path, root_manifest, true)
	.with_context(|| format!("failed to load manifest for dependency `{}`", name))
	.map_err(|err| ManifestError::new(err, manifest_path.clone()))?;
	}
	Ok(())
	}

[osiewicz]

I see, thank you!

[epage]

@rfcbot fcp merge T-cargo

This adds a --workspace flag to cargo clean to select all workspace members for having their artifacts deleted. This can help in some CI situations as workspace members can add bloat cache sizes while also being the least likely to be benefited from caching. In the PR authors case, they want to conditionally call this based on disk space pressure. They aren't uploading caches but having stateful build machines without job affinity, so the cache can blow up from different jobs of different types cycling through the machines and they found this is a good way to reduce that pressure.

The style of "operate on all dependencies by default, select packages among anything within the dependency tree" model that this has mirrors what we have for cargo update which also has a --workspace flag.

[rust-rfcbot]

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

• @0xPoe
• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[epage]

We likely don't need to wait the 10 days. If nothing else, we can always revert.

View changes since this review