feat(cargo-wdk): add --stampinf-args passthrough to customize stampinf options - #733
feat(cargo-wdk): add --stampinf-args passthrough to customize stampinf options#733Shravan Vasista (svasista-ms) wants to merge 8 commits into
--stampinf-args passthrough to customize stampinf options#733Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new cargo wdk build --stampinf-args <ARGS> passthrough to let callers customize stampinf invocation during packaging (e.g., pinning DriverVer / date for reproducible builds), while rejecting switches that cargo-wdk derives and supplies itself.
Changes:
- Introduce
--stampinf-argsCLI option with reserved-switch validation (-f,-a,-c,-k,-u). - Thread the resolved
stampinfarguments throughBuildAction→PackageTaskand incorporate them into thestampinfcommand, allowing caller-supplied-d/-vto override defaults. - Add unit + functional tests covering passthrough behavior and reserved-switch rejection, and update README help output.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/cargo-wdk/src/cli.rs | Adds --stampinf-args parsing/validation and forwards it into build action params. |
| crates/cargo-wdk/src/actions/build/package_task.rs | Incorporates caller-provided stampinf args into the packaging stampinf invocation and adds tests for ordering/overrides. |
| crates/cargo-wdk/src/actions/build/mod.rs | Threads stampinf_args through BuildAction and into PackageTaskParams. |
| crates/cargo-wdk/src/actions/build/tests.rs | Updates build action test setup for the new stampinf_args parameter. |
| crates/cargo-wdk/tests/build_command_test.rs | Adds end-to-end tests for --stampinf-args behavior and validation. |
| crates/cargo-wdk/README.md | Documents the new --stampinf-args option in the help output snippet. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #733 +/- ##
==========================================
+ Coverage 82.93% 83.38% +0.44%
==========================================
Files 25 25
Lines 6685 6872 +187
Branches 6685 6872 +187
==========================================
+ Hits 5544 5730 +186
Misses 1029 1029
- Partials 112 113 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- rename `switches` to `args` - drop the functional test for reserved args - fix README
There was a problem hiding this comment.
🟢 Approval recommended
The passthrough is correctly plumbed end-to-end with clear reserved-arg validation and solid unit/functional test coverage for the new behavior.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-wdk/src/actions/build/package_task.rs:421
args.append(&mut ...collect())allocates an intermediateVec<&str>just to appendwdf_version_flags. You can avoid the extra allocation and simplify the code by extendingargsdirectly from the iterator.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Gurinder Singh (gurry)
left a comment
There was a problem hiding this comment.
The PR LGTM except for these comments
There was a problem hiding this comment.
🔵 Needs a closer look
The new reserved-switch validation error message formatting is currently incorrect (missing closing backtick) and should be fixed to avoid failing the newly added CLI error-string assertion tests.
Review details
Suppressed comments (1)
crates/cargo-wdk/src/cli.rs:268
- The
--stampinf-argsreserved-switch error message is missing a closing backtick around the generated list, so it renders as... supplies the-f,-a, ...-u args itself` (note the unmatched final backtick). Formatting the list explicitly avoids this and improves readability.
format!(
"`--stampinf-args` must not contain `{arg}`; cargo-wdk supplies the `-{}` \
args itself",
STAMPINF_RESERVED_ARGS.join("`, `-")
),
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Gurinder Singh <frederick.the.fool@gmail.com> Signed-off-by: Shravan Vasista <svasista@microsoft.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The --stampinf-args reserved-switch error message is missing a closing backtick, which makes the output inconsistent and causes the new unit test expectation to fail.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-wdk/src/cli.rs:268
- The reserved-switch rejection message is missing a closing backtick after the generated reserved-args list (it currently formats as
...-f,-a,-c,-k,-u args itself). This is harder to read and also contradicts the unit test that expects-u` to be backticked.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Gurinder Singh <frederick.the.fool@gmail.com> Signed-off-by: Shravan Vasista <svasista@microsoft.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are compile-blocking issues in the newly added stampinf_args() error path and in the updated PackageTask tests/argument construction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
crates/cargo-wdk/src/actions/build/package_task.rs:976
- In the new
stampinf_args_works_with_defaultshelper, the expectation closure compares&[&str](args[2..]) directly toVec<String>(expected[..]), which is a type mismatch and will not compile. Compare&strvalues instead (e.g., viaIterator::eqagainstexpected.iter().map(String::as_str)).
// Skip the `-f <inf path>` prefix, whose path is environment
// specific.
cmd == "stampinf" && args[2..] == expected[..]
})
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The feature is fully wired end-to-end, includes both unit and functional test coverage for expected behaviors, and updates documentation accordingly.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new unit tests in package_task.rs take references to temporary String values (&default_kmdf_version()), which should not compile and must be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
crates/cargo-wdk/README.md:116
- This line has trailing whitespace at the end, which can cause lint/style checks to fail and creates noisy diffs. Please remove the trailing space.
crates/cargo-wdk/src/actions/build/package_task.rs:1024 default_kmdf_version()returns aString, and taking a reference to it inside the&[ ... ]slice (&default_kmdf_version()) creates a reference to a temporary that will be dropped immediately. This should not compile (temporary value dropped while borrowed). Bind the version to a local variable first and passkmdf_version.as_str()into the expected args slice.
This issue also appears in the following locations of the same file:
- line 1035
- line 1057
crates/cargo-wdk/src/actions/build/package_task.rs:1038
- Same issue as above:
&default_kmdf_version()takes a reference to a temporaryStringinside the expected args slice, which should fail to compile. Store the version in a local binding and usekmdf_version.as_str()in the slice.
"driver.cat",
"-k",
&default_kmdf_version(),
"-d",
crates/cargo-wdk/src/actions/build/package_task.rs:1060
- Same lifetime issue:
&default_kmdf_version()is a reference to a temporaryStringin the expected args slice. Use a localStringbinding and passkmdf_version.as_str()instead.
"driver.cat",
"-k",
&default_kmdf_version(),
"-v",
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
c0251c6 to
cb525e9
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The feature is fully wired end-to-end with targeted CLI/package logic and accompanying tests, and the remaining feedback is a minor optimization.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The implementation and test coverage for the new passthrough look complete, with only a minor documentation whitespace nit noted.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-wdk/README.md:116
- Line has a trailing space after the final period, which creates an unintended hard line break in Markdown and can cause noisy diffs in the future; remove the trailing whitespace.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new docs/error messaging claim cargo-wdk always “supplies” -k/-u, but those switches are conditional on driver model (not used for WDM), so wording should be corrected to avoid misleading users.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-wdk/src/cli.rs:246
- The doc comment says cargo-wdk "derives" and supplies
-kand-uunconditionally, butrun_stampinfonly adds-kfor KMDF and-ufor UMDF (neither for WDM). Consider updating the wording to reflect that-k/-uare conditional on the driver model, even though they are still reserved in the CLI.
This issue also appears on line 269 of the same file.
crates/cargo-wdk/src/cli.rs:272
- This error message states cargo-wdk "supplies" all reserved args, but
-k/-uare only supplied for KMDF/UMDF packaging (not WDM). Rewording avoids misleading users about when these switches are actually added.
format!(
"`--stampinf-args` must not contain `{arg}`; cargo-wdk supplies the \
{reserved_args} args itself"
),
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite

cargo-wdkcurrently hardcodes itsstampinfinvocation during packaging, so there is no way to customize the options for use cases like pinningDriverVer(-d *and-v *stamp the current date and time, making every build produce a different INF) or to stamp aProvider.This PR adds
--stampinf-argsto thecargo wdk buildcommand similar to other tools.The following switches:
| -f, -a, -c, -k, & -u |are derived bycargo-wdkat build time. So, they are rejected at parse time.Resolves #729
Screenshots
stampinfarguments:/xand-xare considered asstampinfaccepts both