feat(build-common): embed Windows VERSIONINFO resource in cdylib builds - #2446
Open
gleocadie wants to merge 3 commits into
Open
feat(build-common): embed Windows VERSIONINFO resource in cdylib builds#2446gleocadie wants to merge 3 commits into
gleocadie wants to merge 3 commits into
Conversation
datadog_profiling_ffi.dll ships with no VERSIONINFO resource, so Windows Installer's file-replacement logic on upgrade falls back to comparing created-vs-modified timestamps instead of comparing versions. That let an MSI in-place upgrade leave a stale libdatadog v20.0.0 DLL on disk next to freshly-upgraded v25.0.0 native modules, and a fatal AV inside a static initializer (LibraryConfig ABI mismatch) on next startup. Add `build_common::embed_windows_version_info`, a `winresource`-backed helper that populates FileVersion/ProductVersion from the libdatadog release version (build_common's own CARGO_PKG_VERSION, since it declares `version.workspace = true` -- deliberately not the calling crate's, since leaf FFI crates like libdd-profiling-ffi pin their own far-more-slowly bumped semver and would make the version compare identical release over release), plus CompanyName/ProductName/OriginalFilename/FileDescription/ LegalCopyright. Wire it into libdd-profiling-ffi's build.rs. The helper no-ops on non-Windows targets, detected via CARGO_CFG_TARGET_OS rather than #[cfg(windows)]: build scripts always compile for the host, so a compile-time cfg would reflect the host rather than whatever target Cargo is cross-compiling for. It also never hard-fails the build: cross- compiling to a Windows target from Linux/macOS CI without a resource compiler (llvm-rc / *-windres) on PATH degrades to a cargo:warning instead. Gated behind a `winresource` feature on build_common so crates that only need header generation don't pull in the new dependency.
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce0daf0704
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+117
to
+121
| let patch_str = patch_str | ||
| .split(|c: char| !c.is_ascii_digit()) | ||
| .next() | ||
| .unwrap_or(patch_str); | ||
| let patch: u16 = patch_str.parse().ok()?; |
There was a problem hiding this comment.
When a prerelease MSI such as 43.0.0-rc.1 is followed by 43.0.0, this strips the suffix and gives both DLLs the identical fixed file version 43.0.0.0; Windows Installer therefore does not consider the final DLL newer and can retain the prerelease binary. This is a supported release input (scripts/create-release.sh:20 accepts prerelease suffixes), so the packed version needs a monotonic prerelease-to-final representation rather than collapsing both releases to the same value.
Useful? React with 👍 / 👎.
Collaborator
There was a problem hiding this comment.
I'm not sure this is a big deal? We don't typically ship anything with a suffix to customers?
Contributor
Author
There was a problem hiding this comment.
exactly. not a big deal
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 9eb373b | Docs | View more details | Give us feedback! |
ekump
approved these changes
Sep 2, 2026
ekump
left a comment
ekump
left a comment
Collaborator
There was a problem hiding this comment.
a couple of non-blocking questions / suggestions. LGTM
Comment on lines
+117
to
+121
| let patch_str = patch_str | ||
| .split(|c: char| !c.is_ascii_digit()) | ||
| .next() | ||
| .unwrap_or(patch_str); | ||
| let patch: u16 = patch_str.parse().ok()?; |
Collaborator
There was a problem hiding this comment.
I'm not sure this is a big deal? We don't typically ship anything with a suffix to customers?
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
Collaborator
There was a problem hiding this comment.
Is there any value in having a windows-only test to cover the VERSIONINFO changes? Or if we incorporate my panic suggestion at build time, is that enough?
Contributor
Author
There was a problem hiding this comment.
your suggestion looks good, I'll incorporate it.
Co-authored-by: Edmund Kump <edmund.kump@datadoghq.com>
BenchmarksComparisonBenchmark execution time: 2026-09-03 16:09:41 Comparing candidate commit 9eb373b in PR branch Found 4 performance improvements and 12 performance regressions! Performance is the same for 153 metrics, 10 unstable metrics.
|
Contributor
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
datadog_profiling_ffi.dllships with no VERSIONINFO resource, so Windows Installer's file-replacement logic on upgrade falls back to comparing created-vs-modified timestamps instead of comparing versions. That let an MSI in-place upgrade leave a stale libdatadog v20.0.0 DLL on disk next to freshly-upgraded v25.0.0 native modules, and a fatal AV inside a static initializer (LibraryConfig ABI mismatch) on next startup.Add
build_common::embed_windows_version_info, awinresource-backed helper that populates FileVersion/ProductVersion from the libdatadog release version (build_common's own CARGO_PKG_VERSION, since it declaresversion.workspace = true-- deliberately not the calling crate's, since leaf FFI crates like libdd-profiling-ffi pin their own far-more-slowly bumped semver and would make the version compare identical release over release), plus CompanyName/ProductName/OriginalFilename/FileDescription/ LegalCopyright. Wire it into libdd-profiling-ffi's build.rs.Motivation
This crash report https://app.datadoghq.com/error-tracking/issue/593381a8-a5ba-11f1-bb8f-da7ad0900002?query=source%3Adotnet+service%3Ainstrumentation-telemetry-data%2A+%40tags.crash_datadog%3Atrue+%40library_version.major%3A3+-%40tags.crash_profiler%3Atrue+-%40tags.crash_asm%3Atrue+-%40tags.crash_runtime_metrics%3Atrue+%40library_version.minor%3A%3E%3D31+issue.age%3A%3C%3D3600000&from_ts=1788149319000&to_ts=1788235719000&live=false&monitor_id=236533719&monitor_sub_type=.new%28%29&link_source=monitor_notif shows a failed upgrade:
An old version of .NET APM was installed using MSI. After some time, the customer used a newer version of the MSI. But the
datadog_profiling_ffi.dllwas not changed because it was missing theVERSIONINFOresource. Since the default overwrite rule is version based, it will compare the created vs modified timestamps. Since the file was not modified, it's kept.Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.