feat: separate bindgen runs to enable inclusion of mutually-exclusive headers#654
feat: separate bindgen runs to enable inclusion of mutually-exclusive headers#654leon-xd wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restructures wdk-sys bindings generation so mutually-exclusive WDK headers can be supported by running bindgen in separate translation units per API subset, while preserving the existing “flat” public namespace.
Changes:
- Reworks
crates/wdk-sys/build.rsto generate base + per-subset{types,constants,functions}in separate bindgen passes, then aggregatestypes.rs/constants.rs. - Introduces
wdk_build::derives::{DerivesMap, BaseDerivesCallback}and tests, to preserve derive parity when base types are blocklisted in later bindgen runs. - Updates dependencies (e.g.,
bindgen0.72.1) and adjustswdk-sysmodules to rely on generated imports instead of localuse crate::types::*.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/wdk-sys/build.rs | New multi-pass bindgen pipeline, include tracking + blocklisting, and aggregation of base/subset outputs. |
| crates/wdk-sys/src/usb.rs | Removes use crate::types::* from wrapper; adds lint allowance for potentially-empty bindings. |
| crates/wdk-sys/src/storage.rs | Removes types glob import from wrapper module (now expected to be injected by generated code). |
| crates/wdk-sys/src/spb.rs | Removes types glob import from wrapper module (now expected to be injected by generated code). |
| crates/wdk-sys/src/parallel_ports.rs | Removes types glob import; fixes allow-message to refer to Parallel Ports. |
| crates/wdk-sys/src/hid.rs | Removes types glob import from wrapper; adds lint allowance for potentially-empty bindings. |
| crates/wdk-sys/src/gpio.rs | Removes types glob import from wrapper module (now expected to be injected by generated code). |
| crates/wdk-sys/src/constants.rs | Removes types glob import from wrapper module (aggregation now provides the needed scope). |
| crates/wdk-sys/Cargo.toml | Adds regex dependency used by build script for escaping allow/blocklist patterns. |
| crates/wdk-build/src/lib.rs | Exposes derives module (doc-hidden) for build-script consumption. |
| crates/wdk-build/src/derives.rs | Implements derive-set parsing + bindgen callback to answer blocklisted_type_implements_trait. |
| crates/wdk-build/src/bindgen.rs | Sets size_t_is_usize(true) on the default bindgen builder. |
| crates/wdk-build/tests/derives.rs | Integration tests for DerivesMap::from_file and representative bindgen output shapes. |
| crates/wdk-build/Cargo.toml | Adds bitflags + syn for derive parsing implementation. |
| Cargo.toml | Bumps bindgen to 0.72.1; adds bitflags workspace dep. |
| Cargo.lock | Lockfile updates for bindgen bump and new deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Generates `base_types.rs` and harvests the include set visited along the | ||
| /// way. | ||
| /// | ||
| /// The returned path feeds `DeriveMap::from_file` so API-subset passes can |
| Ok(Err(thread_error)) => { | ||
| return Err(thread_error.into().context(format!( | ||
| r#""{thread_name}" thread failed to exit successfully"# | ||
| ))); |
| if exported_types.insert(type_name.clone()) { | ||
| writeln!( | ||
| types_aggregator, | ||
| "pub use {api_subset_name}_types::{type_name};" | ||
| ) | ||
| .expect("writing to String is infallible"); | ||
| } else { | ||
| trace!( | ||
| "Skipping duplicate type {type_name} from {api_subset_name}_types (already \ | ||
| re-exported)" | ||
| ); | ||
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #654 +/- ##
==========================================
+ Coverage 79.45% 80.55% +1.10%
==========================================
Files 26 27 +1
Lines 5500 5967 +467
Branches 5500 5967 +467
==========================================
+ Hits 4370 4807 +437
- Misses 1001 1016 +15
- Partials 129 144 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .expect("Bindings should succeed to generate"); | ||
|
|
||
| let output_file_path = out_path.join("base_types.rs"); | ||
| fs::write(&output_file_path, bindings.to_string()) |
There was a problem hiding this comment.
Any reason for moving away from write_to_file(&output_file_path) which would have been more efficient?
| // Clone the set out under the lock rather than trying to take ownership of | ||
| // the `Arc` — bindgen may retain internal references to the callback | ||
| // (e.g. via libclang-sys), so `Arc::into_inner` is not guaranteed to | ||
| // succeed. The set is small and this runs once per build. |
There was a problem hiding this comment.
Maybe explicitly dropping bindgen_builder here will cause it to give up its Arc reference and then we could do Arc::into_inner().
Note: this is a draft PR. I will be on vacation from 5/3 through 5/10. There is still significant cleanup work to be done before this is published as a PR ready for evaluation.
Overview
This is the second of two PRs dedicated to solving the issue of our current inability to include mutually exclusive headers in our coverage. See #652 for the first PR. This PR is based off of the
derives-librarybranch of my fork and will pull changes as they are made to that branch. Fixes #516.The reason why
wdk-syswas unable to generate bindings for mutually exclusive headers is primarily because of the way we were generating types and constants. In order to generate bindings exported in a flat layer (e.g. developers including types viause wdk_sys::{some_base_type, some_gpio_type, some_usb_type}rather thanuse wdk_sys::{some_base_type, gpio::some_gpio_type, usb::some_usb_type}), we threw all headers regardless of API subset into a single translation unit forbindgento generate bindings for. This worked for the narrow set of API subsets that we enabled, as none of the headers were mutually exclusive.Unfortunately, this was not a sustainable method. There are several PRs introducing new API subsets (see #337, #595, #359, #450) that are blocked because some newly included header broke compatibility with another subset due to a mutually exclusive header.
cargospecifies that features are not meant to be mutually exclusive themselves. Hence, supporting anall-featuresbuild is an invariant when adding new features.Broadly, this PR rewires
wdk_sys/build.rsin order to generate types, constants, and functions per API subset in individual translation units. This generates bindgen output for every item of{constants, types, functions} X ({base} U <set of api subsets enabled>). In order to maintain backwards compatibility, we wire all constants and types files into a single layer forconstants.rsandtypes.rs.After this change, mutually exclusive headers are not an obstacle. Rather, it is an indicator that these headers belong in different features for respective API subsets.
Specific design decisions/issues
Avoiding duplicate bindings
There is a significant amount of duplication in a naive pass of
bindgenfor any given API subset and the base types. This is avoided usingbindgen'sblocklistfunctionality. I ordered the generation strictly: we first runbindgenon the base types and collect which files it traversed. We pass this list of files into thebindgenblocklist for all API subset generation, preventing base types, constants, and functions from being duplicated in API subsets.Derive parity
As noted in #652,
bindgenassumes that any blocklisted types are unable to derive any of Rust's core traits. In order to keep API subset types that depended on a base type with the same set of trait implementations, we create aDerivesMapby parsing the generated base types before any API subset types are generated. We receive a list of trait implementations that each generated type is able to satisfy, and pass this ontobindgenviablocklisted_type_implements_trait, allowingbindgento derive the correct traits for every type.Avoiding collisions between subsets
bindgengenerates a host of helper structs that are the same per generated file. In addition, multiple API subsets can generate similarly named types. These cannot be re-exported per subset. In order to prevent exporting conflicting types we maintain a set of types that we run into during API subset types generation andpub useeach individual type if that type name has not been exported yet. Cross-subset name conflicts are currently resolved first-come-first-serve. If this becomes problematic in practice, we can add customizable precedence rules.Currently there are no collisions in any generated constants. We re-export them via a
pub use *. A future colliding constant would surface as an 'ambiguous reference' error at user compile time.Verification
I tested this with UMDF, KMDF, and WDM configs enabled. I validated against
microsoft/mainusing a bindings comparison tool. The tool itself needs validation which I plan to do on return, but its initial output shows full parity on traitimplementations across all bindings.
I also timed the build process with this change to ensure there was not a significant build time regression. As it stands I am currently able to build
wdk_syswithall_featuresin ~45 seconds, compared tomicrosoft/main's 60 seconds. I would not trust this number as the build script is subject to change with future revisions for this PR.