Repository navigation
Add a unique build revision number to 'prerelease' versions - #9545
andrewlock wants to merge 6 commits into
Conversation
andrewlock
added
the
area:builds
03b200d to
6bc8fd1
Compare
5b6f5f2 to
e8fd6c1
Compare
andrewlock
changed the title
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: e8fd6c10fc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if(_dd_is_prerelease STREQUAL "true") | ||
| set(DD_VERSION_REVISION 1) | ||
| else() | ||
| set(DD_VERSION_REVISION 0) | ||
| endif() |
There was a problem hiding this comment.
This independently duplicates the false/true → 0/1 mapping already defined in tracer/build/_build/version.props, contrary to MAINT-1.2. If the revision scheme changes—for example, to the contemplated revision 2—MSBuild and CMake can silently produce different four-part versions, defeating the native/managed version-mismatch protection. Define the mapping once and make both build systems consume it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Ironically, I did that before, and the 🤖 took offense, and suggested this way. Rather than flip-flop for eternity, I'm inclined to leave it as-is, and address it if/when the mapping gets more complex than true = 1
BenchmarksBenchmark execution time: 2026-10-29 14:44:27 Comparing candidate commit 9dba82d in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 71 metrics, 1 unstable metrics, 7 known flaky benchmarks, 119 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9545) and master. ✅ No regressions detected |
![datadog-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
MSBuild evaluations and native preprocessor checks supported consistent prerelease revision values across managed and native components. No reportable defect was identified; full build and runtime behavior remain unverified.
🤖 Bits Code Review · Commit e8fd6c1
There was a problem hiding this comment.
tl;dr; on the changes here seeing as cmake sucks to read
- minor refactoring of the error flow when reading
DD_VersionPrefixfails - also read
DD_IsPrerelease - set
DD_VERSION_REVISIONbased on its value
|
|
||
| <PropertyGroup> | ||
| <AssemblyVersion>$(ProductVersion)</AssemblyVersion> | ||
| <FileVersion>$(ProductVersion)</FileVersion> |
There was a problem hiding this comment.
e8fd6c1 to
9dba82d
Compare
tracer/src/Directory.Build.props now sets AssemblyVersion next to Version, and the projects that set their own Version clear it, so the SDK derives it from their Version again. Clearing it (rather than setting it to $(Version)) keeps the SDK's suffix stripping, so a future -beta Version still produces a valid AssemblyVersion.
9dba82d to
a69251a
Compare
There was a problem hiding this comment.
More details
Managed version evaluation and native version-macro checks support the intended revision split between releases and prereleases. Full build validation remains limited by the unavailable .NET 11 SDK.
🤖 Bits Code Review · Commit 9dba82d
|
Moving back to draft for now, because I think a better approach is just to use the sem-ver version for comparisons, and to report the full sem-ver version in general |
Summary of changes
When a build is a prerelease build, bump the revision (4th number in 4-part numbers) to
1Reason for change
We are going to make it easier for people to ship unverified dev-builds to customers. These will require adding a unique suffix to a build. However, using a unique X.Y.Z version (as we have in the past) adds a lot of friction. The main reason we have that requirement today is that we cannot easily distinguish in all cases between a tracer that only differs in the prerelease tag, because some places only use the version numbers (e.g. assembly versions, therefore native code too etc).
With this change, a non-prerelease release will always have revision
0, while pre-release versions (of all types) will have a revision1. This lets us bail in cases of version mismatch, and also ensures things like not coliding in the GAC etc (a minor concern in practice).Note
Open question - should we use a different revision for "real 'prerelease' release" and "someone shipped a dev nightly". We could easily use version '2' for those, for example. Question for a future PR!
Implementation details
Mostly just a ternary: if
IsPrerelease = true ? 1 : 0. The majority of the work is replacing all the places that assume the revision will always be 0, and threading it through everywhere we need to.Test coverage
Covered by existing, plus did some manual builds to compare.
Other details
Stacked on
https://datadoghq.atlassian.net/browse/APMLP-2011