use CMake setting for osx deployment target - #59
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the macOS CI build script to stop explicitly setting/passing CMAKE_OSX_DEPLOYMENT_TARGET, with the intent to rely on the value defined in CMakeLists.txt instead.
Changes:
- Removed the script-side defaulting of
CMAKE_OSX_DEPLOYMENT_TARGET. - Removed
-DCMAKE_OSX_DEPLOYMENT_TARGET=...from thecmake ..configure invocation.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Configure | ||
| cmake .. \ | ||
| -DCMAKE_OSX_DEPLOYMENT_TARGET=${CMAKE_OSX_DEPLOYMENT_TARGET} \ | ||
| -DUIOHOOKDIR=${PWD}/deps/${DEPS} \ | ||
| -DCMAKE_BUILD_TYPE=RelWithDebInfo \ | ||
| ${NODEJS_VERSION_PARAM} \ |
| endif() | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
@copilot we do not use universal mac build? Can you re-evaluate this branch?
There was a problem hiding this comment.
Re-evaluated: since this project never uses universal (multi-arch) builds, CMAKE_OSX_ARCHITECTURES is always a single value (arm64 or x86_64) — never a list like arm64;x86_64. The STREQUAL "arm64" check in CMakeLists.txt is therefore correct and reliable.
Additionally, CMAKE_OSX_DEPLOYMENT_TARGET is set before project() (lines 2–13 vs line 14), so CMake sees it during compiler/toolchain initialization — the timing concern doesn't apply here either.
No changes needed.
set CMAKE_OSX_DEPLOYMENT_TARGET before initial toolchain initialization.