feat: add PE resource version + metadata generation#661
Conversation
… var as the same as an absent env var, update documentation, update and add unit tests
There was a problem hiding this comment.
Pull request overview
This PR adds automatic generation and compilation of Windows PE VERSIONINFO resources for driver binaries built with wdk-build, so that file version and descriptive metadata are embedded directly into the produced .sys / .dll artifacts (not just the .inf).
Changes:
- Introduces
resource_compilemodule to generate a.rc, compile it viarc.exeto a.res, and emit linker args to embed it into the driver binary. - Extends
ConfigErrorandConfig::configure_binary_buildto run the version-resource compilation step during build script configuration. - Updates WDK metadata parsing so
[package.metadata.wdk.*]sections (e.g.,version-resource) can coexist while still strictly validating thedriver-modelconfiguration, plus adds unit tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/wdk-build/src/resource_compile.rs | New module implementing version/metadata resolution, .rc generation, rc.exe invocation, and link-arg emission, with unit tests. |
| crates/wdk-build/src/metadata/mod.rs | Narrows WDK configuration deserialization to metadata.wdk.driver-model while permitting other metadata.wdk.* sections; adds tests. |
| crates/wdk-build/src/lib.rs | Exposes the new module, adds a ConfigError variant, and invokes resource compilation from configure_binary_build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #661 +/- ##
==========================================
+ Coverage 79.93% 81.89% +1.95%
==========================================
Files 26 27 +1
Lines 5633 6627 +994
Branches 5633 6627 +994
==========================================
+ Hits 4503 5427 +924
- Misses 1002 1050 +48
- Partials 128 150 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…tness for different dev environment setups
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alan632 <aln.noda7@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alan632 <aln.noda7@gmail.com>
… typos.toml used in rc.exe args
…se crate only), fix lint issues
…ary rerun directive (CI builds are fresh), fix formatting in .typos.toml
- add pass through of source error into ConfigError's output
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alan632 <aln.noda7@gmail.com>
| // Invoke rc.exe (expected to be on PATH via eWDK prompt or cargo-wdk setup) | ||
| let include_paths = resource_include_paths(config)?; | ||
|
|
||
| let mut cmd = Command::new("rc.exe"); |
There was a problem hiding this comment.
Was it an intentional choice not to use pre-existing crates for RC file generation and compilation, such as winresource, winres, or embed_resource, or were there other reasons?
There was a problem hiding this comment.
It wasn't an intentional choice not to use an existing crate, just something carried over after looking at someone else's previous work (#243).
From my quick look it looks like winresource supersedes winres (winres doesn't support Rust >= v1.61). I think the benefit winresource brings is it handles the .rc file content generation automatically but we would still need the toolchain/resource path creation for it. And, embed_resource requires both (it needs a pre-generated .rc file). I'll take a further look into these crates but initially I'm thinking the extra complexity with integrating them doesn't gain us that much compared with the extra dependencies we'd pull in?
There was a problem hiding this comment.
just chiming in here. When I assigned this to Alan, I did intentionally tell Alan that the existing crates I surveyed previously either had long-standing bugs that affected their reliability, were unmaintained, or otherwise did not satisfy our requirements. Our goal here is to piggy-back off our existing metadata configuration system to avoid needing to manually an rc file that duplicates information we've provided elsewhere.
- add a handler to deal with multiple authors cleanly - move env_var_non_empty to utils.rs - fix a cyclic error types with ResourceCompileError
| fn create_include_subdirectories(include_directory: &Path, subdirectories: &[&str]) { | ||
| for subdirectory in subdirectories { | ||
| fs::create_dir_all(include_directory.join(subdirectory)).unwrap(); | ||
| } |
| [files] | ||
| extend-exclude = ["**/Cargo.lock", "**/target/**"] | ||
|
|
||
| # Allowlist short words that collide with the typos dictionary but are |
There was a problem hiding this comment.
nit: scope this down as much as possible since this could be a common typo. Something like this could work:
[type.resource_compile]
extend-glob = ["resource_compile.rs"]
extend-words = {
# rc.exe /fo (output file) switch
fo = "fo"
}| // Invoke rc.exe (expected to be on PATH via eWDK prompt or cargo-wdk setup) | ||
| let include_paths = resource_include_paths(config)?; | ||
|
|
||
| let mut cmd = Command::new("rc.exe"); |
There was a problem hiding this comment.
just chiming in here. When I assigned this to Alan, I did intentionally tell Alan that the existing crates I surveyed previously either had long-standing bugs that affected their reliability, were unmaintained, or otherwise did not satisfy our requirements. Our goal here is to piggy-back off our existing metadata configuration system to avoid needing to manually an rc file that duplicates information we've provided elsewhere.
| /// `CARGO_PKG_VERSION` env var (emitted by cargo) | ||
| /// if env var is not present or empty. | ||
| fn resolve_version() -> Result<DriverVersion, ResourceCompileError> { | ||
| let version_str = env_var_non_empty(VERSION_ENV_VAR).map_or_else( |
There was a problem hiding this comment.
VERSION_ENV_VAR corresponds with STAMPINF_VERSION that was previously added in order to get around cargo-wdk currently not having a flexible customization system. That system is being developed right now. I think perhaps we should avoid tying more functionality to that temporary workaround.
| for (macro_name, macro_value) in [ | ||
| ("VER_FILEDESCRIPTION_STR", format!("\"{file_description}\"")), | ||
| ("VER_PRODUCTNAME_STR", format!("\"{product_name}\"")), | ||
| ("VER_FILEVERSION", version.as_rc_numeric()), |
There was a problem hiding this comment.
What about PRODUCTVERSION? do both get set by default in a C driver?
| DriverConfig::Umdf(_) => ("VFT_DLL", "VFT2_UNKNOWN"), | ||
| }; | ||
|
|
||
| let mut rc = String::with_capacity(1024); |
There was a problem hiding this comment.
Since you're writing into a string anyways, I think this chunk would be a lot more readable and easier to grok the RC file contents if it did a template with string replacement similar to
.|
|
||
| let rc = generate_rc_content(version, &metadata, &config); | ||
|
|
||
| assert!(rc.contains("#pragma code_page(65001)")); |
There was a problem hiding this comment.
Not necessarily required for this PR to merge, but I think some snapshot testing here would be better since the output is fairly complex. It would be worth looking into insta or snapbox
| @@ -0,0 +1,1645 @@ | |||
| // Copyright (c) Microsoft Corporation | |||
There was a problem hiding this comment.
I think there should be an integration test asserting the version derived here matches the version in the inf. You can probably add that test coverage by piggybacking on some of the existing integration tests in cargo-wdk
| fn resolve_version() -> Result<DriverVersion, ResourceCompileError> { | ||
| let version_str = env_var_non_empty(VERSION_ENV_VAR).map_or_else( | ||
| || { | ||
| env::var("CARGO_PKG_VERSION").map_err(|_| ResourceCompileError::MetadataError { |
There was a problem hiding this comment.
what is the current behavior here if there is not version defined (since cargo made version field optional)? Does it error out or does it default to 0.0.0?
| let metadata = cargo_metadata::MetadataCommand::new() | ||
| .manifest_path(&manifest_path) | ||
| .no_deps() | ||
| .exec() | ||
| .map_err(|e| ResourceCompileError::MetadataError { | ||
| detail: format!("cargo metadata failed: {e}"), | ||
| })?; |
Summary
Currently driver PE resource metadata and versioning do not get populated (the version in the *.inf does) and makes issue and post-mortem troubleshooting/tracing difficult.
This PR adds the functionality to populate the PE resource metadata automatically.
wdk-build/src/lib.rsconfigure_binary_buildto initiate *.rc generation, compilation to *.res, and linker directive emissionwdk-build/src/resource_compile.rscompile_version_resourcecalled fromconfigure_binary_buildSTAMPINF_VERSION, falling back toCARGO_PKG_VERSIONset by Cargorc.exeto compile to a *.res and emits linker directiveswdk-build/src/metadata/mod.rs[package.metadata.wdk.version-resource]), keeps strict serde validation of Wdk/DriverConfig (eg.[package.metadata.wdk.driver-model])Validation
Local testing with in production driver in the following scenarios: missing version-resource metadata, populated version-resource metadata, and
STAMPINF_VERSIONset.Tested with sample drivers kmdf/wdm/umdf.
WDM sample driver with
[package.metadata.wdk.version-resource]info filled in:UMDF sample driver without
[package.metadata.wdk.version-resource]info: