fix(cargo-wdk): workspace resolution in build and clean actions to match cargo - #706
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates cargo-wdk’s build and clean flows to better match Cargo’s workspace resolution behavior (Issue #477), especially when invoked from within a workspace subdirectory.
Changes:
- Pass the
Metadataprovider intoCleanActionand usecargo metadatato detect workspaces before falling back to “emulated workspace” scanning. - Add a
NotAbsoluteerror variant for clean to mirror build’s handling of unexpected non-absolute paths. - Add/adjust
CleanActionunit tests for the new workspace-detection behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/cargo-wdk/src/cli.rs | Wires Metadata into CleanAction::new(...) so clean can query cargo metadata. |
| crates/cargo-wdk/src/actions/clean/mod.rs | Adds workspace detection via cargo metadata before emulated workspace scanning. |
| crates/cargo-wdk/src/actions/clean/error.rs | Adds NotAbsolute error variant for workspace root resolution failures. |
| crates/cargo-wdk/src/actions/build/mod.rs | Adds workspace-root inference attempt when Cargo.toml is not in the current directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let owns_working_dir = cargo_metadata.workspace_packages().iter().any(|p| { | ||
| p.manifest_path | ||
| .parent() | ||
| .and_then(|path| absolute(path.as_std_path()).ok()) | ||
| .is_some_and(|package_dir| package_dir.starts_with(&self.working_dir)) | ||
| }); | ||
|
|
||
| if owns_working_dir { | ||
| let workspace_root = absolute(cargo_metadata.workspace_root.as_std_path()) | ||
| .map_err(|e| { | ||
| CleanActionError::NotAbsolute( | ||
| cargo_metadata.workspace_root.clone().into(), | ||
| e, | ||
| ) | ||
| })?; | ||
| debug!( | ||
| "Working directory {} lies inside the workspace rooted at {}; running cargo \ | ||
| clean from workspace root", | ||
| self.working_dir.display(), | ||
| workspace_root.display() | ||
| ); | ||
| return self.run_cargo_clean(&workspace_root); | ||
| } |
| let owns_working_dir = cargo_metadata.workspace_packages().iter().any(|p| { | ||
| p.manifest_path | ||
| .parent() | ||
| .and_then(|path| absolute(path.as_std_path()).ok()) | ||
| .is_some_and(|package_dir| package_dir.starts_with(&self.working_dir)) | ||
| }); | ||
|
|
||
| if owns_working_dir { | ||
| let workspace_root = absolute(cargo_metadata.workspace_root.as_std_path()) | ||
| .map_err(|e| { | ||
| BuildActionError::NotAbsolute( | ||
| cargo_metadata.workspace_root.clone().into(), | ||
| e, | ||
| ) | ||
| })?; | ||
| debug!( | ||
| "Working directory {} lies inside the workspace rooted at {}; running build \ | ||
| from workspace root", | ||
| self.working_dir.display(), | ||
| workspace_root.display() | ||
| ); | ||
| return self.run_from_workspace_root(&workspace_root); | ||
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #706 +/- ##
==========================================
+ Coverage 82.18% 82.62% +0.44%
==========================================
Files 25 26 +1
Lines 6287 6516 +229
Branches 6287 6516 +229
==========================================
+ Hits 5167 5384 +217
- Misses 989 998 +9
- Partials 131 134 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…and `clean` actions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
crates/cargo-wdk/src/actions/mod.rs:149
find_workspace_rootalways returnscargo_metadata.workspace_root, which for a workspace member subdirectory (e.g.member/src/) will typically be the workspace root, not the nearest package root. InBuildAction::runthis causescargo wdk buildfrom inside a member subtree to build/package every workspace member (because it later runs from the workspace root), which diverges fromcargo build’s “nearest Cargo.toml” behavior and can significantly increase build time.
absolute(cargo_metadata.workspace_root.as_std_path()).ok()
crates/cargo-wdk/src/actions/mod.rs:147
- The
is_emulated_workspaceearly-return makesfind_workspace_rootreturnNoneeven whencargo metadatasuccessfully identifies a real workspace, solely because the current directory contains non-memberCargo.tomlchildren. That preserves “emulated workspace” behavior, but it also meanscargo wdk build/cleancan still diverge fromcargoinside an actual workspace (cargo would ignore those excluded projects and operate on the workspace it found). If the PR goal is to “match cargo”, consider gating this behavior behind an explicit flag or removing it when a workspace is detected.
This issue also appears on line 149 of the same file.
let is_emulated_workspace = dirs.iter().any(|entry| {
entry.is_dir
&& fs.exists(&entry.path.join("Cargo.toml"))
&& absolute(&entry.path).is_ok_and(|child_dir| {
!member_dirs
.iter()
.any(|member| member.starts_with(&child_dir))
})
});
if is_emulated_workspace {
return None;
}
crates/cargo-wdk/src/actions/build/mod.rs:178
- New workspace-root resolution logic in
BuildAction::runisn’t covered by the existing build action unit tests (there don’t appear to be tests for the “no Cargo.toml in cwd but inside a workspace” path, or for ensuring a member subdirectory builds only the selected package vs the whole workspace). This path is central to #477 and can regress behavior/perf without a test.
// Standalone driver/driver workspace support
if self.fs.exists(&self.working_dir.join("Cargo.toml")) {
return self.run_from_workspace_root(&self.working_dir);
}
let dirs = self.fs.read_dir_entries(&self.working_dir)?;
if let Some(workspace_root) = super::find_workspace_root(
self.metadata,
self.fs,
&self.working_dir,
&dirs,
self.locked,
self.features,
) {
debug!(
"Working directory {} lies inside the workspace rooted at {}; running build from \
workspace root",
self.working_dir.display(),
workspace_root.display()
);
return self.run_from_workspace_root(&workspace_root);
}
…esolution # Conflicts: # crates/cargo-wdk/src/actions/mod.rs # crates/cargo-wdk/src/cli.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (4)
crates/cargo-wdk/src/actions/mod.rs:36
trim_start_matches(r"\\?\")will break verbatim UNC paths like\\?\UNC\server\share\...by producing an invalid path (UNC\server\share\...). That means workspace resolution viacargo metadatacan still fail for network shares. Handle the\\?\UNC\prefix explicitly (mapping it back to a standard UNC path) and avoid string conversion when no verbatim prefix is present.
let working_dir_trimmed: PathBuf = working_dir
.to_string_lossy()
.trim_start_matches(r"\\?\")
.into();
crates/cargo-wdk/src/actions/mod.rs:24
- The doc comment for
find_workspace_rootis incomplete/misleading: the function also returnsNonewhen the working directory is treated as an "emulated workspace" (non-member child projects), and it usesdirsfor that decision. Consider documenting that exception so callers understand when a workspace root will not be returned.
/// Resolves the root of the workspace for a working directory that has no
/// `Cargo.toml` of its own.
crates/cargo-wdk/src/actions/build/mod.rs:204
- This path now runs
cargo metadatainfind_workspace_rootand then immediately callsrun_from_workspace_root, which runscargo metadataagain (viaget_cargo_metadataat actions/build/mod.rs:381 andrun_from_workspace_rootat :292). For large workspaces, this doubles the metadata cost for the common "cwd without Cargo.toml" case. Consider refactoring to reuse the first metadata result (e.g., have the helper return the parsed metadata or a workspace_root + workspace_packages) to avoid the second invocation.
let dirs = self.fs.read_dir_entries(&self.working_dir)?;
if let Some(workspace_root) = super::find_workspace_root(
self.metadata,
self.fs,
crates/cargo-wdk/src/actions/clean/mod.rs:105
- This new workspace-root probe changes the documented control flow of
CleanAction::run(the doc comment above still says that when there is noCargo.tomlwe always treat the directory as an emulated workspace). Since the doc comment isn’t in a changed hunk, consider at least adding a local note here and updating the higher-level docs in the same PR.
let dirs = self.fs.read_dir_entries(&self.working_dir)?;
if let Some(workspace_root) = super::find_workspace_root(
self.metadata,
self.fs,
TBD
Resolves #477