fix(cmake): preserve Windows build OS metadata#35405
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request updates cmake/options.cmake to robustly handle the BUILD_VER_OSTYPE cache variable, preventing it from being cleared or shadowed during configuration runs. Feedback suggests applying the same pattern to BUILD_VER_CPUTYPE, which suffers from the exact same shadowing and first-run clearing issue.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| set(BUILD_VER_DATE "" CACHE STRING "Version date override (e.g. %Y-%m-%d %H:%M:%S %z)") | ||
| set(BUILD_VER_CPUTYPE "" CACHE STRING "CPU type override") | ||
| set(BUILD_VER_OSTYPE "" CACHE STRING "OS type override") | ||
| if(DEFINED CACHE{BUILD_VER_OSTYPE} AND NOT "$CACHE{BUILD_VER_OSTYPE}" STREQUAL "") |
There was a problem hiding this comment.
The variable BUILD_VER_CPUTYPE suffers from the exact same shadowing and first-run clearing issue as BUILD_VER_OSTYPE. In cmake/platform.cmake, BUILD_VER_CPUTYPE is initialized as a normal variable (e.g., SET(BUILD_VER_CPUTYPE "x64")). Then, in cmake/options.cmake (line 15), it is declared as a cache variable: set(BUILD_VER_CPUTYPE "" CACHE STRING "CPU type override"). On the first configure run, this clears the normal variable, resulting in an empty BUILD_VER_CPUTYPE in version.cmake. On subsequent runs, the normal variable set by platform.cmake shadows any user-provided cache override (e.g., -DBUILD_VER_CPUTYPE=arm64). To resolve this, please apply the same robust pattern to BUILD_VER_CPUTYPE as well: if(DEFINED CACHE{BUILD_VER_CPUTYPE} AND NOT "$CACHE{BUILD_VER_CPUTYPE}" STREQUAL "") set(BUILD_VER_CPUTYPE "$CACHE{BUILD_VER_CPUTYPE}") else() set(BUILD_VER_CPUTYPE "${BUILD_VER_CPUTYPE}" CACHE STRING "CPU type override" FORCE) endif()
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
Addressed the reviewer suggestion by applying the same cache/normal-variable preservation pattern to Validation run locally: A real Windows configure also reached the version block with |
Description
Preserve the platform-derived build CPU/OS metadata when
options.cmakeregistersBUILD_VER_CPUTYPEandBUILD_VER_OSTYPEas cache overrides.What Problem This Solves
The top-level CMake load order is:
platform.cmakecomputes platform metadata such asBUILD_VER_CPUTYPE=x64andBUILD_VER_OSTYPE=Windows.options.cmakethen declared the same names as empty cache strings, andversion.cmakecould fall back to generic defaults when those values were empty.That could make a Windows configure report contradictory metadata:
The cache entries still need to support user overrides such as
-DBUILD_VER_OSTYPE=CustomOSand-DBUILD_VER_CPUTYPE=arm64, so the fix cannot simply hardcode platform values later inversion.cmake.Changes
options.cmakenow handles both build metadata cache variables in two cases:platform.cmakeare used as the cache defaults.Evidence
A minimal CMake script that simulates the relevant variable/cache sequence now preserves platform defaults and explicit overrides:
A real Windows configure reached the version block with corrected defaults:
The same configure with explicit overrides preserved the user values:
Both configure attempts later stopped in the external dependency stage because the local checkout does not have the cached prebuilt RocksDB artifact; the corrected
cpu:andos:metadata is printed before that unrelated stop.Possible call chain / impact
This change is limited to build metadata initialization. It does not alter platform detection, compiler flags, dependency selection, or user-provided non-empty
BUILD_VER_CPUTYPE/BUILD_VER_OSTYPEoverrides.Checklist
Please check the items in the checklist if applicable.