fix: prevent RUSTFLAGS from leaking into downstream cargo-make rust-script builds#629
fix: prevent RUSTFLAGS from leaking into downstream cargo-make rust-script builds#629wmmc88 wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents downstream cargo make consumers of wdk-build from inheriting caller RUSTFLAGS (e.g. -D warnings) into rust-script compilation of embedded scripts by ensuring wdk-build is treated as a non-path dependency when loaded from the registry.
Changes:
- Update
load_wdk_build_makefile()to copy+rewrite embeddedrust-scriptmanifests fromwdk-build = { path = "." }towdk-build = "X.Y.Z"whenwdk-buildis resolved from a non-path source. - Preserve existing symlink behavior when
wdk-buildis a workspace/path dependency to keep local dev flows intact. - Remove redundant
version = "...“fields fromrust-scriptembedded dependency specs and add an explanatory note in the main makefile.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/wdk-build/src/cargo_make.rs | Implements registry-vs-path behavior (copy+rewrite vs symlink) when materializing makefiles into target/. |
| crates/wdk-build/rust-driver-makefile.toml | Removes redundant embedded dependency versions and documents the downstream rewrite behavior. |
| crates/wdk-build/rust-driver-sample-makefile.toml | Removes redundant embedded dependency versions from sample tasks. |
Comments suppressed due to low confidence (2)
crates/wdk-build/src/cargo_make.rs:934
- The rewrite uses a simple
replacewithout validating that any occurrences were actually patched. If the makefile format changes (spacing/comments/features), this will silently leave thepath = "."dependency intact and the original lint-leak issue will persist. Consider checking whetherpatched_contentdiffers frommakefile_contentand returning aConfigError(or at least logging) when no replacement occurred.
let version = &wdk_build_package.version;
let patched_content = makefile_content.replace(
r#"wdk-build = { path = "." }"#,
&format!("wdk-build = \"{version}\""),
);
crates/wdk-build/src/cargo_make.rs:927
load_wdk_build_makefilenow sometimes writes a patched copy intotarget/instead of always creating/updating a symlink. The rustdoc/comments above this function (and the publicload_*_makefilewrappers) still describe the behavior as always symlinking; they should be updated to reflect the new copy+rewrite path for registry dependencies.
if wdk_build_package.source.is_some() {
// wdk-build is a registry (or git) dependency. Rewrite path dependencies in
// the makefile's rust-script embedded manifests to version-only dependencies so
// that cargo treats wdk-build as a registry dep and applies --cap-lints. This
// prevents the caller's RUSTFLAGS (e.g. -D warnings) from leaking into the
// published wdk-build crate's compilation.
let makefile_content = std::fs::read_to_string(&rust_driver_makefile_toml_path)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/wdk-build/src/cargo_make.rs
Outdated
| if wdk_build_package.source.is_some() { | ||
| // wdk-build is a registry (or git) dependency. Rewrite path dependencies in | ||
| // the makefile's rust-script embedded manifests to version-only dependencies so | ||
| // that cargo treats wdk-build as a registry dep and applies --cap-lints. This | ||
| // prevents the caller's RUSTFLAGS (e.g. -D warnings) from leaking into the | ||
| // published wdk-build crate's compilation. | ||
| let makefile_content = std::fs::read_to_string(&rust_driver_makefile_toml_path) | ||
| .map_err(|source| IoError::with_path(&rust_driver_makefile_toml_path, source))?; | ||
|
|
||
| let version = &wdk_build_package.version; | ||
| let patched_content = makefile_content.replace( | ||
| r#"wdk-build = { path = "." }"#, | ||
| &format!("wdk-build = \"{version}\""), | ||
| ); | ||
|
|
||
| // Only write if content changed or file doesn't exist, to avoid unnecessary | ||
| // rebuilds from rust-script cache invalidation | ||
| let should_write = if destination_path.exists() { | ||
| let existing_content = std::fs::read_to_string(&destination_path) | ||
| .map_err(|source| IoError::with_path(&destination_path, source))?; | ||
| existing_content != patched_content | ||
| } else { | ||
| true | ||
| }; | ||
|
|
||
| if should_write { | ||
| if destination_path.exists() { | ||
| std::fs::remove_file(&destination_path) | ||
| .map_err(|source| IoError::with_path(&destination_path, source))?; | ||
| } | ||
| std::fs::write(&destination_path, patched_content) | ||
| .map_err(|source| IoError::with_path(&destination_path, source))?; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This change introduces new behavior (copying and patching the makefile based on cargo_metadata source) but there are no tests covering the rewrite decision or the resulting file content. Since this module already has unit tests, consider adding tests (e.g., using temp dirs + a small fixture makefile, and injecting/stubbing metadata) to lock in registry-vs-path behavior and ensure the wdk-build dep is rewritten as intended.
This issue also appears on line 930 of the same file.
crates/wdk-build/src/cargo_make.rs
Outdated
| if wdk_build_package.source.is_some() { | ||
| // wdk-build is a registry (or git) dependency. Rewrite path dependencies in | ||
| // the makefile's rust-script embedded manifests to version-only dependencies so | ||
| // that cargo treats wdk-build as a registry dep and applies --cap-lints. This | ||
| // prevents the caller's RUSTFLAGS (e.g. -D warnings) from leaking into the | ||
| // published wdk-build crate's compilation. | ||
| let makefile_content = std::fs::read_to_string(&rust_driver_makefile_toml_path) | ||
| .map_err(|source| IoError::with_path(&rust_driver_makefile_toml_path, source))?; | ||
|
|
||
| let version = &wdk_build_package.version; | ||
| let patched_content = makefile_content.replace( | ||
| r#"wdk-build = { path = "." }"#, | ||
| &format!("wdk-build = \"{version}\""), | ||
| ); |
There was a problem hiding this comment.
wdk_build_package.source.is_some() groups registry and git dependencies together, but rewriting to wdk-build = "{version}" will break consumers using a git dependency (Cargo will try to resolve that version from the registry). Consider detecting source kind (e.g., registry+... vs git+...) and only performing the version-only rewrite for registry sources; for git sources, either preserve the symlink behavior or rewrite to a git dependency in the embedded manifests.
This issue also appears on line 921 of the same file.
| //! # `load_rust_driver_makefile()`, this path dependency is automatically | ||
| //! # rewritten to a version-only registry dependency. This ensures cargo | ||
| //! # applies `--cap-lints allow` to wdk-build, preventing the caller's | ||
| //! # RUSTFLAGS from leaking into the published crate's compilation. |
There was a problem hiding this comment.
The NOTE says the path = "." dependency is rewritten when loaded by downstream consumers, but the implementation only rewrites when wdk-build is resolved from a non-path source (registry/git). Downstream users can also depend on wdk-build via a local path, in which case the rewrite intentionally does not happen. Consider clarifying the note to mention it only applies when wdk-build is a registry dependency.
| //! # `load_rust_driver_makefile()`, this path dependency is automatically | |
| //! # rewritten to a version-only registry dependency. This ensures cargo | |
| //! # applies `--cap-lints allow` to wdk-build, preventing the caller's | |
| //! # RUSTFLAGS from leaking into the published crate's compilation. | |
| //! # `load_rust_driver_makefile()`, and `wdk-build` is resolved from a | |
| //! # non-path source (e.g., registry or git), this path dependency is | |
| //! # automatically rewritten to a version-only registry dependency. This | |
| //! # ensures cargo applies `--cap-lints allow` to wdk-build, preventing the | |
| //! # caller's RUSTFLAGS from leaking into the published crate's compilation. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #629 +/- ##
==========================================
+ Coverage 77.42% 78.63% +1.21%
==========================================
Files 24 24
Lines 4851 5117 +266
Branches 4851 5117 +266
==========================================
+ Hits 3756 4024 +268
+ Misses 978 976 -2
Partials 117 117 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4df0dc5 to
9a89265
Compare
…t builds Unset RUSTFLAGS for cargo-make and cargo-wdk steps in build.yaml to prevent -D warnings from leaking into rust-script compilation of wdk-build. This is a temporary workaround. The proper fix is in microsoft/windows-drivers-rs#629 which rewrites path deps to version-only deps in load_wdk_build_makefile() for registry consumers. Root cause: wdk-build = { path = '.' } in rust-driver-makefile.toml makes wdk-build a path dependency even when loaded from the registry cache via symlink. Cargo does not apply --cap-lints allow to path deps, so RUSTFLAGS leaks through. Tracked in ADO Bug #5396440.
9a89265 to
3522897
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/wdk-build/src/cargo_make.rs
Outdated
| let version = &wdk_build_package.version; | ||
| let patched_content = makefile_content.replace( | ||
| r#"wdk-build = { path = "." }"#, | ||
| &format!("wdk-build = \"{version}\""), |
There was a problem hiding this comment.
The rewritten dependency uses wdk-build = "{version}", which is a caret requirement and may allow rust-script to resolve a newer compatible 0.5.x than the wdk-build version that provided this makefile. To keep the embedded scripts and the loaded makefile in lockstep (and make downstream behavior deterministic/offline-friendly), consider rewriting to an exact requirement like wdk-build = "={version}" instead.
| &format!("wdk-build = \"{version}\""), | |
| &format!("wdk-build = \"={version}\""), |
| let is_registry_source = wdk_build_package | ||
| .source | ||
| .as_ref() | ||
| .is_some_and(|s| s.repr.starts_with("registry+") || s.repr.starts_with("sparse+")); | ||
|
|
||
| if is_registry_source { | ||
| // wdk-build is a registry dependency. Rewrite path dependencies in the | ||
| // makefile's rust-script embedded manifests to version-only dependencies | ||
| // so that cargo treats wdk-build as a registry dep and applies | ||
| // --cap-lints. This prevents the caller's RUSTFLAGS (e.g. -D warnings) | ||
| // from leaking into the published wdk-build crate's compilation. | ||
| let makefile_content = std::fs::read_to_string(&rust_driver_makefile_toml_path) | ||
| .map_err(|source| IoError::with_path(&rust_driver_makefile_toml_path, source))?; | ||
|
|
||
| let version = &wdk_build_package.version; | ||
| let patched_content = makefile_content.replace( | ||
| r#"wdk-build = { path = "." }"#, | ||
| &format!("wdk-build = \"{version}\""), | ||
| ); | ||
|
|
||
| if patched_content == makefile_content { | ||
| warn!( | ||
| "No wdk-build path dependency found to rewrite in {makefile_name:?}. The makefile \ | ||
| format may have changed." | ||
| ); | ||
| } | ||
|
|
||
| // Only write if content changed or file doesn't exist, to avoid | ||
| // unnecessary rebuilds from rust-script cache invalidation | ||
| let should_write = if destination_path.exists() { | ||
| let existing_content = std::fs::read_to_string(&destination_path) | ||
| .map_err(|source| IoError::with_path(&destination_path, source))?; | ||
| existing_content != patched_content | ||
| } else { | ||
| true | ||
| }; | ||
|
|
||
| if should_write { | ||
| if destination_path.exists() { | ||
| std::fs::remove_file(&destination_path) | ||
| .map_err(|source| IoError::with_path(&destination_path, source))?; | ||
| } | ||
| std::fs::write(&destination_path, patched_content) | ||
| .map_err(|source| IoError::with_path(&destination_path, source))?; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This new registry-vs-non-registry behavior (copy+rewrite vs symlink) is not covered by tests, but this module already has unit tests for other helpers. Consider extracting the rewrite decision/content patching into a small pure helper and adding tests that (a) registry sources produce rewritten content and (b) non-registry sources preserve the symlink/update behavior.
3522897 to
a28a18b
Compare
…t builds Unset RUSTFLAGS for cargo-make and cargo-wdk steps in build.yaml to prevent -D warnings from leaking into rust-script compilation of wdk-build. Temporary workaround until microsoft/windows-drivers-rs#629 is merged and a new wdk-build is published. Tracked in ADO Bug #5396440.
Unset RUSTFLAGS for the cargo-make step in build.yaml to prevent -D warnings from leaking into rust-script compilation of wdk-build. Only cargo-make is affected since it uses rust-script to compile inline build scripts where wdk-build is a path dependency (via symlink), bypassing cargo's --cap-lints. cargo-wdk and cargo install are normal cargo operations with proper --cap-lints handling and do not need this workaround. Temporary workaround until microsoft/windows-drivers-rs#629 is merged and a new wdk-build is published. Tracked in ADO Bug #5396440.
## Changes ### 1. Fix nightly clippy + rustfmt failures Edits to pass new nightly `clippy::unnecessary_trailing_comma` [lint](https://rust-lang.github.io/rust-clippy/master/index.html?search=clippy%3A%3Aunnecessary_trailing_comma#unnecessary_trailing_comma) and nightly rustfmt import reordering. - Remove 4 unnecessary trailing commas in `format!`/`println!` macros (`general/echo/kmdf/exe/src/main.rs`) - Reorder underscore-prefixed imports to match nightly rustfmt sorting (`device.rs`, `queue.rs`, `driver.rs`) **Failed CI run:** https://github.com/microsoft/Windows-rust-driver-samples/actions/runs/22519532366 ### 2. Workaround RUSTFLAGS leaking into cargo-make/cargo-wdk builds Unset `RUSTFLAGS` for `cargo make`, `cargo wdk install`, and `cargo wdk build` steps in `build.yaml`. This prevents `-D warnings` from leaking into `rust-script` compilation of `wdk-build` (which is treated as a path dependency via symlink, bypassing cargo's `--cap-lints allow`). Temporary workaround until microsoft/windows-drivers-rs#629 is merged and a new `wdk-build` is published.
…cript builds When downstream consumers (e.g. Windows-rust-driver-samples) run cargo make, RUSTFLAGS=-D warnings leaks into rust-script compilation of wdk-build because the makefile dep specs use path = '.' which makes wdk-build a path dependency. Cargo does not apply --cap-lints to path dependencies, so -D warnings applies directly to the published crate, causing upstream lint issues to break downstream builds. Fix: load_wdk_build_makefile() now detects whether wdk-build is a registry dependency (via package.source from cargo_metadata). When it is, the makefile is copied with path deps rewritten to version-only deps, ensuring cargo applies --cap-lints allow. When wdk-build is a local path dep, the existing symlink behavior is preserved. Also removes the unused version field from all 14 rust-script dep specs in the makefiles, since path always takes precedence when both are specified.
a28a18b to
bcdf058
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// The version of `wdk-build` from which the file comes is determined by the | ||
| /// working directory of the process that invokes this function. For example, | ||
| /// if this function is ultimately executing in a `cargo_make` `load_script`, | ||
| /// the files will come from the `wdk-build` version that is in the | ||
| /// `Cargo.lock` file, and not the `wdk-build` version specified in the | ||
| /// `load_script`. |
There was a problem hiding this comment.
The updated doc comment explains the new copy+rewrite behavior, but the # Errors list for this function (below) still describes only symlink-related IO errors and doesn’t mention that NoWdkBuildCrateDetected can be returned. Please align the error docs with the current behavior (read/write/symlink + missing wdk-build).
| let wdk_build_package = &wdk_build_package_matches[0]; | ||
|
|
||
| let rust_driver_makefile_toml_path = wdk_build_package | ||
| .manifest_path | ||
| .parent() |
There was a problem hiding this comment.
rust_driver_makefile_toml_path is used for both rust-driver and rust-driver-sample makefiles (it’s derived from the generic makefile_name). Renaming it to something neutral like makefile_path/source_makefile_path would avoid confusion as this function grows.
Problem
When downstream consumers of
wdk-build(e.g.microsoft/Windows-rust-driver-samples) runcargo make,RUSTFLAGS=-D warningsleaks intorust-scriptcompilation of the inline build scripts. This causes warnings in the publishedwdk-buildcrate to become hard errors, breaking downstream CI.Example failure (from Windows-rust-driver-samples Build CI):
Root Cause
The rust-script tasks in
rust-driver-makefile.tomlspecifywdk-build = { path = ".", version = "0.5.1" }. Cargo always usespathwhen both are specified, ignoringversion. Whenload_rust_driver_makefile()symlinks the makefile from the registry cache,path = "."resolves to the registry source via--base-path, makingwdk-builda path dependency. Cargo does not apply--cap-lints allowto path dependencies, soRUSTFLAGS=-D warningsapplies directly to the published crate.Normal
cargo buildis unaffected becausewdk-buildis resolved as a registry dependency there.Fix
load_wdk_build_makefile()now checkspackage.sourcefromcargo_metadata:source.is_some()): Copies the makefile and rewriteswdk-build = { path = "." }towdk-build = "X.Y.Z"(version from metadata). This ensures cargo treats it as a registry dep and applies--cap-lints allow.source.is_none()): Preserves existing symlink behavior so local development works correctly.Removed unused
versionfield from all 14 rust-script dep specs inrust-driver-makefile.tomlandrust-driver-sample-makefile.toml, sincepathalways takes precedence. Added a comment explaining the automatic rewrite byload_rust_driver_makefile().Scenarios
package.sourceextend, notload_*)Some(registry+...)--cap-lintsNone