Skip to content

feat(cargo-wdk): add --stampinf-args passthrough to customize stampinf options - #733

Open
Shravan Vasista (svasista-ms) wants to merge 8 commits into
microsoft:mainfrom
svasista-ms:feat/729-stampinf-args
Open

feat(cargo-wdk): add --stampinf-args passthrough to customize stampinf options#733
Shravan Vasista (svasista-ms) wants to merge 8 commits into
microsoft:mainfrom
svasista-ms:feat/729-stampinf-args

Conversation

@svasista-ms

@svasista-ms Shravan Vasista (svasista-ms) commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

cargo-wdk currently hardcodes its stampinf invocation during packaging, so there is no way to customize the options for use cases like pinning DriverVer (-d * and -v * stamp the current date and time, making every build produce a different INF) or to stamp a Provider.

This PR adds --stampinf-args to the cargo wdk build command similar to other tools.

The following switches: | -f,  -a, -c, -k, & -u | are derived by cargo-wdk at build time. So, they are rejected at parse time.

Resolves #729

Screenshots

  • Help Output:
after-01-help
  • Build with custom stampinf arguments:
after-03-pinned
  • Rejects reserved switches (both /x and -x are considered as stampinf accepts both
after-04-reserved

Copilot AI lite review requested due to automatic review settings September 1, 2026 11:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-args CLI option with reserved-switch validation (-f, -a, -c, -k, -u).
  • Thread the resolved stampinf arguments through BuildActionPackageTask and incorporate them into the stampinf command, allowing caller-supplied -d/-v to 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.

Comment thread crates/cargo-wdk/src/cli.rs
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.54369% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.38%. Comparing base (3655880) to head (bc3d720).

Files with missing lines Patch % Lines
crates/cargo-wdk/src/actions/build/package_task.rs 98.58% 2 Missing ⚠️
crates/cargo-wdk/src/cli.rs 98.41% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread crates/cargo-wdk/src/cli.rs Outdated
Comment thread crates/cargo-wdk/src/cli.rs Outdated
Comment thread crates/cargo-wdk/src/cli.rs Outdated
Comment thread crates/cargo-wdk/src/cli.rs Outdated
Comment thread crates/cargo-wdk/tests/build_command_test.rs Outdated
Comment thread crates/cargo-wdk/README.md
- rename `switches` to `args`
- drop the functional test for reserved args
- fix README
Copilot AI review requested due to automatic review settings September 4, 2026 08:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 intermediate Vec<&str> just to append wdf_version_flags. You can avoid the extra allocation and simplify the code by extending args directly from the iterator.
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@gurry Gurinder Singh (gurry) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR LGTM except for these comments

Comment thread crates/cargo-wdk/README.md
Comment thread crates/cargo-wdk/src/cli.rs Outdated
Comment thread crates/cargo-wdk/src/cli.rs
Comment thread crates/cargo-wdk/src/cli.rs Outdated
Copilot AI review requested due to automatic review settings September 8, 2026 08:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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-args reserved-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

@svasista-ms

Copy link
Copy Markdown
Contributor Author

Verified the unclosed backticks. It is being rendered correctly:

after-07-backticks

All backticks are closed.

Comment thread crates/cargo-wdk/README.md Outdated
Co-authored-by: Gurinder Singh <frederick.the.fool@gmail.com>
Signed-off-by: Shravan Vasista <svasista@microsoft.com>
Copilot AI review requested due to automatic review settings September 9, 2026 09:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

Comment thread crates/cargo-wdk/src/cli.rs
Co-authored-by: Gurinder Singh <frederick.the.fool@gmail.com>
Signed-off-by: Shravan Vasista <svasista@microsoft.com>
Copilot AI review requested due to automatic review settings September 9, 2026 16:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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_defaults helper, the expectation closure compares &[&str] (args[2..]) directly to Vec<String> (expected[..]), which is a type mismatch and will not compile. Compare &str values instead (e.g., via Iterator::eq against expected.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

Comment thread crates/cargo-wdk/src/actions/build/package_task.rs Outdated
Comment thread crates/cargo-wdk/src/cli.rs Outdated
Copilot AI review requested due to automatic review settings September 9, 2026 17:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Copilot AI review requested due to automatic review settings September 10, 2026 06:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 a String, 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 pass kmdf_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 temporary String inside the expected args slice, which should fail to compile. Store the version in a local binding and use kmdf_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 temporary String in the expected args slice. Use a local String binding and pass kmdf_version.as_str() instead.
                "driver.cat",
                "-k",
                &default_kmdf_version(),
                "-v",
  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/wdk-build/rust-driver-makefile.toml

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Comment thread crates/cargo-wdk/src/actions/build/package_task.rs Outdated
Copilot AI review requested due to automatic review settings September 10, 2026 07:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Comment thread crates/cargo-wdk/README.md Outdated
Copilot AI review requested due to automatic review settings September 10, 2026 11:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 -k and -u unconditionally, but run_stampinf only adds -k for KMDF and -u for UMDF (neither for WDM). Consider updating the wording to reflect that -k/-u are 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/-u are 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

Comment thread crates/cargo-wdk/README.md
Comment thread crates/cargo-wdk/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support stampinf passthrough in cargo-wdk

4 participants