[build] Raise macOS minimum deployment target to 13.0#794
Conversation
Stamp the wheel as macosx_13_0_arm64 instead of macosx_11_0_arm64, and pin CMAKE_OSX_DEPLOYMENT_TARGET=13.0 (set before project()) so the compiled binary's minos matches the advertised wheel tag. The old -DOSX_DEPLOYMENT_TARGET=11.0 define was an unused preprocessor macro that never set the real deployment target; it is replaced by the CMake variable.
|
@codex review |
![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: 6e0b87b58c
ℹ️ 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".
| if(NOT DEFINED CMAKE_OSX_DEPLOYMENT_TARGET) | ||
| set(CMAKE_OSX_DEPLOYMENT_TARGET "13.0" CACHE STRING "Minimum macOS deployment target") |
There was a problem hiding this comment.
When a macOS builder sets the standard MACOSX_DEPLOYMENT_TARGET environment variable, this new guard still creates an explicit CMAKE_OSX_DEPLOYMENT_TARGET=13.0 cache entry before project(), so CMake never gets to initialize the variable from the env value. That makes the advertised override path silently ignored for builds that need to raise or lower the deployment target; check $ENV{MACOSX_DEPLOYMENT_TARGET} before setting the default, or seed the cache from it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Opus here: Good catch, fixed in a684072. The guard now also requires $ENV{MACOSX_DEPLOYMENT_TARGET} to be empty before applying the 13.0 default, so CMake gets to seed CMAKE_OSX_DEPLOYMENT_TARGET from the env var during project() as intended. Verified against Darwin-Initialize.cmake: the env seeding is a non-FORCE cache set, so it was indeed a no-op once our pre-project() entry existed.
The previous guard only checked NOT DEFINED CMAKE_OSX_DEPLOYMENT_TARGET, so creating the cache entry before project() preempted CMake's env-var seeding (a non-FORCE cache set in Darwin-Initialize.cmake is a no-op once the entry exists). That silently ignored a builder-provided MACOSX_DEPLOYMENT_TARGET. Also require the env var to be empty before applying our 13.0 default, so an explicit -D or the standard env var both take effect as documented.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Stamp the wheel as macosx_13_0_arm64 instead of macosx_11_0_arm64, and pin CMAKE_OSX_DEPLOYMENT_TARGET=13.0 (set before project()) so the compiled binary's minos matches the advertised wheel tag.
The old -DOSX_DEPLOYMENT_TARGET=11.0 define was an unused preprocessor macro that never set the real deployment target; it is replaced by the CMake variable.
Issue: #
Brief Summary
copilot:summary
Walkthrough
copilot:walkthrough