refactor(cargo-wdk): target architecture handling in `BuildAction` to respect other forms of `target` overrides #494

svasista-ms · 2025-09-02T19:46:39Z

This PR includes changes to the build command to respect other forms of target configuration vis config.toml (build.target) and environment (CARGO_BUILD_TARGET) along with the --target CLI option.

Logical Changes

cargo build already supported the above forms of target specification. What we needed was to ensure packaging step uses the same target as build step and uses the correct output location of binaries.

In this PR is we now run cargo build --message-format=json (instead of cargo build) and extract the output location of binaries from the resulting JSON. To get the target arch we run cargo rustc --print right after cargo build --message-format=json and parse the output for the target_arch field. [1] [2] [3].

Test Changes

Added kmdf-driver-with-target-override test fixture in cargo-wdk/tests and the cases in crates/cargo-wdk/tests/build_command_test.rs [4]

CI Workflow Changes

To support the integration test cases that run cargo wdk build in a cross-compilation setting when using Nuget-based WDK, two additional environment variables have been added:

FullVersionNumber: Denotes the complete version number (including QFE) of the WDK and SDK nuget packages that were downloaded [5]
NugetPackagesRoot: The root directory where the nuget packages required to build a driver are downloaded [6]

The variables are set in the install-wdk action and used in build_command_test where some form of target override is tested. Using NugetPackagesRoot and FullVersionNumber the WDKContentRoot env var is set to point to the correct WDK content based on the target architecture.

Closes #435
Closes #348

- Removed the TargetArch enum and simplified target architecture handling in the CLI. - Updated the command execution function to accept an optional working directory. - Enhanced error reporting for command failures, including status codes and stderr output. - Cleaned up the `run` method in the `WdkBuild` struct to prepare for future enhancements. - Improved test assertions for driver packaging tasks and added a utility function to determine target folders based on architecture. - Updated the Cargo configuration for the WDM driver test to specify the target architecture explicitly.

crates/cargo-wdk/src/providers/exec.rs

…des and resolve conflicts

Copilot

Pull Request Overview

This PR refactors the cargo-wdk build command to better respect Cargo's target architecture override mechanisms by removing the TargetArch enum and simplifying architecture handling. Instead of detecting architecture via rustc --print host-tuple, the code now uses cargo rustc -- --print cfg to detect the effective target architecture that Cargo will build for, respecting configuration settings from .cargo/config.toml or environment variables.

Removed TargetArch enum and related CLI architecture detection logic
Updated command execution to support optional working directories and enhanced error reporting
Improved driver packaging task error messages and test utilities

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`crates/cargo-wdk/tests/wdm-driver/.cargo/config.toml`	Added explicit target architecture configuration
`crates/cargo-wdk/tests/build_command_test.rs`	Updated test assertions and added utility for target folder detection
`crates/cargo-wdk/src/providers/wdk_build.rs`	Added placeholder for future WDK build number field
`crates/cargo-wdk/src/providers/mod.rs`	Enhanced command error reporting with status codes and stderr
`crates/cargo-wdk/src/providers/exec.rs`	Added working directory support and improved debug logging
`crates/cargo-wdk/src/cli.rs`	Removed `TargetArch` enum and simplified build command handling
`crates/cargo-wdk/src/actions/mod.rs`	Removed `TargetArch` enum definition
`crates/cargo-wdk/src/actions/build/tests.rs`	Updated test cases to use new architecture handling
`crates/cargo-wdk/src/actions/build/package_task.rs`	Changed driver model parameter to reference and minor formatting cleanup
`crates/cargo-wdk/src/actions/build/mod.rs`	Implemented new target architecture detection and package root path construction
`crates/cargo-wdk/src/actions/build/error.rs`	Added new error types for architecture detection
`crates/cargo-wdk/src/actions/build/build_task.rs`	Updated to use simplified architecture handling

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

crates/cargo-wdk/src/providers/wdk_build.rs

crates/cargo-wdk/src/providers/mod.rs

…ecture and artifact paths for the packaging process - Added tests for building KMDF drivers with explicit target architecture options. - Created a new test fixture for `kmdf-driver-with-target-override` to validate target override scenarios

Copilot

Pull Request Overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

crates/cargo-wdk/tests/build_command_test.rs

crates/cargo-wdk/src/providers/wdk_build.rs

…g logic - Added new error variants to `BuildActionError` and `BuildTaskError` for better error reporting. - Enhanced the `build_and_package` function to handle driver binary paths and target architecture more effectively. - Modified tests to reflect changes in the build process and ensure accurate artifact handling. - Renamed `Profile::Dev` to `Profile::Debug` for clarity and consistency.

Copilot

Pull Request Overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

.github/workflows/local-development-makefile.yaml

crates/cargo-wdk/src/actions/build/mod.rs

crates/cargo-wdk/src/actions/build/build_task.rs

crates/cargo-wdk/tests/build_command_test.rs

- Resolve conflicts - Add tests

Copilot

Pull Request Overview

Copilot reviewed 26 out of 38 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

crates/cargo-wdk/src/actions/build/mod.rs

crates/cargo-wdk/src/actions/build/build_task.rs

crates/cargo-wdk/src/actions/build/mod.rs

Copilot

Pull Request Overview

Copilot reviewed 26 out of 38 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

crates/cargo-wdk/tests/build_command_test.rs

crates/cargo-wdk/src/actions/build/build_task.rs

crates/cargo-wdk/src/actions/build/mod.rs

Copilot

Pull Request Overview

Copilot reviewed 26 out of 38 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

crates/cargo-wdk/src/actions/mod.rs:39

The profile parsing accepts 'debug' but Cargo's standard development profile is called 'dev'. This creates inconsistency with Cargo conventions.

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        match s.to_lowercase().as_str() {
            "debug" => std::result::Result::Ok(Self::Debug),
            "release" => std::result::Result::Ok(Self::Release),
            _ => Err(format!("'{s}' is not a valid profile")),
        }
    }

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

remove retry mechanism for cargo clean in integration tests improve comments for `get_target_dir_for_packaging` fn

…ariants are defined

Copilot

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crates/cargo-wdk/tests/kmdf-driver-with-target-override/Cargo.toml

crates/cargo-wdk/src/actions/build/mod.rs

Copilot · 2025-12-05T08:06:43Z

crates/cargo-wdk/tests/build_command_test.rs

-        run_cargo_clean(&driver_path);
-        let stdout = run_build_cmd(&driver_path);
+#[test]
+fn kmdf_driver_with_target_override_cli_wins() {


The test kmdf_driver_with_target_override_cli_wins sets both CARGO_BUILD_TARGET env var (to X86_64) and --target-arch CLI arg (to ARM64) to verify that CLI wins. However, the test fixture's .cargo/config.toml also has target = "x86_64-pc-windows-msvc". This means there are actually 3 conflicting target specifications, not 2. The test name and logic should clarify that CLI wins over both env and config.toml.

Suggested change

fn kmdf_driver_with_target_override_cli_wins() {

/// This test verifies that the CLI `--target-arch` argument takes precedence over both

/// the `CARGO_BUILD_TARGET` environment variable and the `target` specified in `.cargo/config.toml`.

/// All three sources are set to conflicting values:

/// - `.cargo/config.toml` sets target to x86_64-pc-windows-msvc

/// - `CARGO_BUILD_TARGET` env var is set to either x86_64-pc-windows-msvc or aarch64-pc-windows-msvc

/// - CLI `--target-arch` is set to ARM64

/// The test asserts that the CLI value wins.

fn kmdf_driver_with_target_override_cli_wins_over_env_and_config() {

@svasista-ms Do add a comment stating the three competing sources of config. The comment can be brief. Doesn't have to be as big as the one suggested by copilot

crates/cargo-wdk/src/actions/build/tests.rs

crates/cargo-wdk/src/actions/build/mod.rs

crates/cargo-wdk/tests/build_command_test.rs

crates/cargo-wdk/tests/test_utils/mod.rs

crates/cargo-wdk/src/actions/build/build_task.rs

crates/cargo-wdk/src/actions/build/mod.rs

crates/cargo-wdk/tests/build_command_test.rs

Copilot

Pull request overview

Copilot reviewed 17 out of 18 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/cargo-wdk/tests/build_command_test.rs

…unction

gurry · 2025-12-26T07:44:45Z

crates/cargo-wdk/tests/build_command_test.rs

+    let wdk_content_root = std::env::var("NugetPackagesRoot")
+        .ok()
+        .map(|nuget_package_root| get_nuget_wdk_content_root(target_arch, &nuget_package_root));
+    let env = wdk_content_root
+        .as_deref()
+        .map(|wdk_content_root| [("WDKContentRoot", Some(wdk_content_root))]);


Better to extract this into a function and use it everywhere

Thanks @gurry, I moved the NugetPackagesRoot check to the helper function. Renamed the fn to get_wdk_content_root_from_nuget_packages_root. This fn will return None if either of the required environment variables are not set.

gurry · 2025-12-26T11:26:26Z

❌ Patch coverage is 92.27528% with 55 lines in your changes missing coverage. Please review. ✅ Project coverage is 82.92%.

@svasista-ms looks like the test coverage of the newly added code has some room for improvement. Please have a look

…dd comments

Copilot

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-29T04:26:51Z

crates/cargo-wdk/src/actions/build/mod.rs

+                            "Driver binary's parent directory is missing. Driver binary (.dll) \
+                             path: {}",


The comment uses "Driver binary's parent directory is missing" but the parent directory of any file path should always exist. The real issue is that the path is likely a root path or otherwise invalid. Consider updating the error message to be more accurate about the failure condition.

Suggested change

"Driver binary's parent directory is missing. Driver binary (.dll) \

path: {}",

"Cannot determine parent directory for driver binary; path may be a \

root or otherwise invalid. Driver binary (.dll) path: {}",

Copilot

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-29T10:02:10Z

.vscode/settings.json

    "./tests/wdk-macros-tests/Cargo.toml",
    "./tests/wdk-sys-tests/Cargo.toml",
+
+    // Rust Analyzer is disabled on the following test projects because 


The comment at line 25 should have proper sentence capitalization and punctuation. Change "// Rust Analyzer is disabled on the following test projects because" to "// Rust Analyzer is disabled on the following test projects because:"

Suggested change

// Rust Analyzer is disabled on the following test projects because

// Rust Analyzer is disabled on the following test projects because:

Copilot · 2025-12-29T10:02:11Z

crates/cargo-wdk/src/actions/build/mod.rs

+    ///   is found in the output or its parent folder cannot be determined.
+    /// - `BuildActionError::NotAbsolute` - If the DLL's parent path could not
+    ///   be made absolute.
+    /// - `BuildActionError::CannotDetermineTargetDir` - If a cargo message
+    ///   could not be parsed.


The documentation at lines 411-416 lists the same error variant CannotDetermineTargetDir twice. The first one should be for "no matching DLL file found" and the second for "cargo message parse errors". Consider consolidating these into a single bullet point or making them distinct, such as:

BuildActionError::CannotDetermineTargetDir - If no matching DLL file is found in the output, its parent folder cannot be determined, or a cargo message could not be parsed.

Suggested change

/// is found in the output or its parent folder cannot be determined.

/// - `BuildActionError::NotAbsolute` - If the DLL's parent path could not

/// be made absolute.

/// - `BuildActionError::CannotDetermineTargetDir` - If a cargo message

/// could not be parsed.

/// is found in the output, its parent folder cannot be determined, or a

/// cargo message could not be parsed.

/// - `BuildActionError::NotAbsolute` - If the DLL's parent path could not

/// be made absolute.

Copilot · 2025-12-29T10:02:11Z

crates/cargo-wdk/src/actions/build/mod.rs

+    /// Invokes `cargo rustc -- --print cfg` and finds the `target_arch` value
+    ///
+    /// # Arguments
+    /// * `working_dir` -- Working directory from which the command must be


The double dash in the comment at line 493 should be a single dash for consistency with standard documentation style. Change "* working_dir -- Working directory" to "* working_dir - Working directory".

Suggested change

/// * `working_dir` -- Working directory from which the command must be

/// * `working_dir` - Working directory from which the command must be

github-advanced-security bot found potential problems Sep 2, 2025

View reviewed changes

crates/cargo-wdk/src/providers/exec.rs Fixed Show fixed Hide fixed

svasista-ms changed the title ~~refactor: target architecture handling and command execution~~ refactor(cargo-wdk): target architecture handling in BuildAction to respect other forms of target overrides Sep 4, 2025

svasista-ms added 3 commits September 4, 2025 20:53

fix: skip packaging if target kind is not cdylib

a5c6f70

Merge branch 'main' into support-target-overrides

0da33be

Merge remote-tracking branch 'origin/main' into support-target-overri…

778939a

…des and resolve conflicts

Copilot AI review requested due to automatic review settings September 25, 2025 03:58

Copilot AI reviewed Sep 25, 2025

View reviewed changes

crates/cargo-wdk/src/providers/wdk_build.rs Outdated Show resolved Hide resolved

crates/cargo-wdk/src/providers/mod.rs Outdated Show resolved Hide resolved

svasista-ms added 2 commits September 29, 2025 15:07

Merge branch 'main' into support-target-overrides

61b1f5d

Copilot AI review requested due to automatic review settings September 29, 2025 09:38

Copilot AI reviewed Sep 29, 2025

View reviewed changes

crates/cargo-wdk/tests/build_command_test.rs Outdated Show resolved Hide resolved

crates/cargo-wdk/tests/build_command_test.rs Outdated Show resolved Hide resolved

crates/cargo-wdk/src/providers/wdk_build.rs Outdated Show resolved Hide resolved

svasista-ms added 2 commits September 29, 2025 16:57

fix: update test workflow and test assertions.

b980aa9

Copilot AI review requested due to automatic review settings October 7, 2025 15:05

Copilot AI reviewed Oct 7, 2025

View reviewed changes

svasista-ms added 4 commits October 9, 2025 18:45

fix typos

32dfb50

address Copilot's comments

09170ba

Merge main into support-target-overrides:

1d804c0

- Resolve conflicts - Add tests

check if the order of targets matters for the default target selection.

2e0f588

Copilot AI review requested due to automatic review settings October 16, 2025 14:16

Copilot AI reviewed Oct 16, 2025

View reviewed changes

crates/cargo-wdk/src/actions/build/mod.rs Show resolved Hide resolved

crates/cargo-wdk/src/actions/build/build_task.rs Outdated Show resolved Hide resolved

crates/cargo-wdk/src/actions/build/mod.rs Outdated Show resolved Hide resolved

svasista-ms added 2 commits October 17, 2025 00:09

fix expect in build_command_test

1f9d721

fix lock scope in build_command_test

ca4391f

Copilot AI review requested due to automatic review settings October 16, 2025 19:39

Copilot AI reviewed Oct 16, 2025

View reviewed changes

crates/cargo-wdk/tests/build_command_test.rs Outdated Show resolved Hide resolved

crates/cargo-wdk/src/actions/build/build_task.rs Outdated Show resolved Hide resolved

crates/cargo-wdk/src/actions/build/mod.rs Outdated Show resolved Hide resolved

svasista-ms added 2 commits October 17, 2025 01:52

fix clippy issues

238c3c0

fix tests

31e25ee

Copilot AI review requested due to automatic review settings October 17, 2025 05:04

Copilot AI reviewed Oct 17, 2025

View reviewed changes

fix BuildAction tests

69e661e

remove retry mechanism for cargo clean in integration tests improve comments for `get_target_dir_for_packaging` fn

Copilot AI review requested due to automatic review settings December 5, 2025 08:02

Copilot started reviewing on behalf of svasista-ms December 5, 2025 08:03 View session

Copilot finished reviewing on behalf of svasista-ms December 5, 2025 08:04

fix issue in PackageTaskError, corrected the order in which error v…

70fcc29

…ariants are defined

Copilot AI reviewed Dec 5, 2025

View reviewed changes

gurry reviewed Dec 8, 2025

View reviewed changes

svasista-ms added 2 commits December 8, 2025 18:16

address comments

4f7332a

fix tests

59832bb

svasista-ms requested a review from a team December 9, 2025 12:46

gurry mentioned this pull request Dec 11, 2025

Support target overrides from config.toml or env variable #435

Open

gurry reviewed Dec 19, 2025

View reviewed changes

crates/cargo-wdk/tests/build_command_test.rs Outdated Show resolved Hide resolved

refactor build command tests to simplify environment variable handling

7dceec0

Copilot AI review requested due to automatic review settings December 23, 2025 16:33

Copilot started reviewing on behalf of svasista-ms December 23, 2025 16:33 View session

Copilot AI reviewed Dec 23, 2025

View reviewed changes

crates/cargo-wdk/tests/build_command_test.rs Outdated Show resolved Hide resolved

crates/cargo-wdk/tests/build_command_test.rs Outdated Show resolved Hide resolved

svasista-ms added 2 commits December 24, 2025 22:49

fix: update package directory name formatting in NuGet content root f…

c2b9741

…unction

refactor: disable rust-analyzer on cargo-wdk test fixtures

6131ea3

gurry reviewed Dec 26, 2025

View reviewed changes

Merge branch 'main' into support-target-overrides

f55fee8

Copilot AI review requested due to automatic review settings December 29, 2025 04:24

svasista-ms added 2 commits December 29, 2025 09:54

refactor: rename test for clarity on target override precedence and a…

aa47db1

…dd comments

refactor: simplify WDK content root retrieval from NuGet packages

cf1e0da

Copilot started reviewing on behalf of svasista-ms December 29, 2025 04:24 View session

Copilot AI reviewed Dec 29, 2025

View reviewed changes

address review comments

f1ce41c

Copilot AI review requested due to automatic review settings December 29, 2025 09:59

Copilot started reviewing on behalf of svasista-ms December 29, 2025 09:59 View session

Copilot AI reviewed Dec 29, 2025

View reviewed changes

-fn kmdf_driver_with_target_override_cli_wins() {
+/// This test verifies that the CLI `--target-arch` argument takes precedence over both
+/// the `CARGO_BUILD_TARGET` environment variable and the `target` specified in `.cargo/config.toml`.
+/// All three sources are set to conflicting values:
+/// - `.cargo/config.toml` sets target to x86_64-pc-windows-msvc
+/// - `CARGO_BUILD_TARGET` env var is set to either x86_64-pc-windows-msvc or aarch64-pc-windows-msvc
+/// - CLI `--target-arch` is set to ARM64
+/// The test asserts that the CLI value wins.
+fn kmdf_driver_with_target_override_cli_wins_over_env_and_config() {

		"Driver binary's parent directory is missing. Driver binary (.dll) \
		path: {}",

	// Rust Analyzer is disabled on the following test projects because
	// Rust Analyzer is disabled on the following test projects because:

	/// * `working_dir` -- Working directory from which the command must be
	/// * `working_dir` - Working directory from which the command must be

refactor(cargo-wdk): target architecture handling in BuildAction to respect other forms of target overrides #494

Are you sure you want to change the base?

refactor(cargo-wdk): target architecture handling in BuildAction to respect other forms of target overrides #494

Uh oh!

Conversation

svasista-ms commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Logical Changes

Test Changes

CI Workflow Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

gurry Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

refactor(cargo-wdk): target architecture handling in `BuildAction` to respect other forms of `target` overrides #494

refactor(cargo-wdk): target architecture handling in `BuildAction` to respect other forms of `target` overrides #494

svasista-ms commented Sep 2, 2025 •

edited

Loading

gurry commented Dec 26, 2025 •

edited

Loading