Skip to content

Conversation

@svasista-ms
Copy link
Contributor

@svasista-ms svasista-ms commented Jul 2, 2025

When tested with WDK Version 22621, the following bug was found -

--- stderr D:\a\windows-drivers-rs\windows-drivers-rs\crates\wdk-sys\types-input.h:53:10: fatal error: 'ucx/1.6/ucxclass.h' file not found

https://github.com/microsoft/windows-drivers-rs/actions/runs/15851446771/job/44685702556#step:9:1631

Instead of using version 1.6, the latest version of ucx available in the $WDKContentRoot\Include\10.0.26100.0\km\ucx directory is used.

image

Summary of changes,

  1. ucx_header_path function in impl Config that determines the latest version of UCX header in the directory.
  2. utils::find_max_version_in_directory function to find the max version in a directory that contains folders of the form x.y where x and y are valid u32s.
  3. Extracted windows_sdk_library_path construction into to arch_sdk_library_path for reusability.
  4. Added basic tests

@svasista-ms svasista-ms requested review from a team and wmmc88 July 2, 2025 13:22
Copy link
Contributor

@gurry gurry left a comment

Choose a reason for hiding this comment

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

The "use the UCX header from the latest version" logic implemented here is only a temporary fix right? I assume we're going to make it smart enough later to pick the right header version according to the chosen kit version, especially when we have the relevant enhancements for this in cargo-wdk. Is that understanding correct?

@wmmc88
Copy link
Collaborator

wmmc88 commented Jul 7, 2025

The "use the UCX header from the latest version" logic implemented here is only a temporary fix right? I assume we're going to make it smart enough later to pick the right header version according to the chosen kit version, especially when we have the relevant enhancements for this in cargo-wdk. Is that understanding correct?

It's tracked in #412. The current thinking is default it to latest, but configurable in metadata in the future

@gurry
Copy link
Contributor

gurry commented Jul 8, 2025

It's tracked in #412. The current thinking is default it to latest, but configurable in metadata in the future

Got it. Thanks.

Copy link
Contributor

@gurry gurry left a comment

Choose a reason for hiding this comment

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

A few comments

gurry
gurry previously approved these changes Jul 11, 2025
…mprove error handling, implement from_str for TwoPartVersion type
Copy link
Collaborator

@wmmc88 wmmc88 left a comment

Choose a reason for hiding this comment

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

Approved! left a couple nit comments of where you could add some tracking issue comments. if you want to quickly fix up the tiny things and ping me, ill re-approve

Copy link
Collaborator

@wmmc88 wmmc88 left a comment

Choose a reason for hiding this comment

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

Actually, the Box leak should definitely be fixed before merge. Probably can just make usb_headers fn return a Cow<'static, str>

@gurry
Copy link
Contributor

gurry commented Jul 16, 2025

Yes, not very widely used. But this type will be handy when we implement same logic for other versioned headers in the Kit.

Remember YAGNI

test cases for bindgen_header_contents function to cover usb headers combinations
updated wdk-sys build script accordingly
@wmmc88 wmmc88 requested a review from Copilot July 17, 2025 20:17
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the build scripts to automatically select the latest UCX header version from the WDK content, refactors the header-path logic for reuse, and adds corresponding utility functions and tests.

  • Introduces TwoPartVersion and find_max_version_in_directory to detect the newest “x.y” subdirectory.
  • Refactors Config::headers and bindgen_header_contents to return Result and propagate errors.
  • Extracts sdk_library_path, implements Config::ucx_header, and updates UCX include logic.
  • Adds tests for version parsing, directory scanning, and USB header generation branches.

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/wdk-sys/build.rs Added ? to propagate errors in binding‐generation helper calls
crates/wdk-build/src/utils.rs Defined TwoPartVersion, parsing, directory‐version scan, and tests
crates/wdk-build/src/lib.rs Refactored headers, bindgen_header_contents, extracted helpers
crates/wdk-build/Cargo.toml Added assert_fs as a dev‐dependency
crates/wdk-build/rust-driver-makefile.toml Minor change to .map usage
.github/workflows/codeql.yml Updated WDK installation and cleanup script
Comments suppressed due to low confidence (3)

crates/wdk-build/src/utils.rs:415

  • The doc comment mentions BasicVersion but the function returns TwoPartVersion. Update the documentation to refer to the correct return type.
/// * `Some(BasicVersion)` - The maximum version found

crates/wdk-build/src/utils.rs:422

  • Using .flatten() on an iterator of Result<DirEntry, io::Error> won’t compile because Result doesn’t implement IntoIterator for DirEntry. Consider using .filter_map(Result::ok) to skip entries that failed to read.
        .flatten()

crates/wdk-build/Cargo.toml:40

  • The assert_fs.workspace = true entry under [dev-dependencies] uses invalid TOML syntax and won’t pull in the external crate. It should be something like assert_fs = { version = "<version>", workspace = true } or specify a version key.
assert_fs.workspace = true

wmmc88
wmmc88 previously approved these changes Jul 22, 2025
gurry
gurry previously approved these changes Jul 23, 2025
@gurry gurry disabled auto-merge July 23, 2025 02:33
Copy link
Contributor

@krishnakumar4a4 krishnakumar4a4 left a comment

Choose a reason for hiding this comment

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

I’ve added a few minor comments, primarily on the tests.

@svasista-ms svasista-ms dismissed stale reviews from gurry and wmmc88 via 6f64b26 July 23, 2025 12:55
@gurry gurry enabled auto-merge July 24, 2025 01:57
@gurry gurry added this pull request to the merge queue Jul 24, 2025
Merged via the queue into microsoft:main with commit 1dd4930 Jul 24, 2025
63 checks passed
@gurry gurry deleted the ucx-header branch July 24, 2025 04:35
@leon-xd leon-xd mentioned this pull request Oct 17, 2025
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.

4 participants