Upgrade to Electron 43 - #22
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the build configuration to target Electron 43 headers and align CI tooling accordingly, ensuring native module builds use the correct runtime headers and Node.js toolchain.
Changes:
- Fixes macOS deployment target variable name so arm64 defaults can be applied.
- Bumps Electron header/runtime version to
v43.2.0in both CMake and CI. - Updates GitHub Actions macOS build to use Node.js
24.x.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| CMakeLists.txt | Fixes macOS deployment target variable typo and updates default runtime headers version to Electron v43.2.0. |
| ci/build-osx.sh | Adjusts how macOS deployment target is passed into CMake during CI builds. |
| .github/workflows/build.yml | Updates CI runtime headers version and Node.js version used for builds. |
Suppressed comments (1)
CMakeLists.txt:15
CMAKE_OSX_DEPLOYMENT_TARGETis set afterproject(). Now that the variable name typo is fixed, this block may still not reliably affect compiler/linker flags becauseCMAKE_OSX_DEPLOYMENT_TARGETis typically consumed during language/toolchain initialization (triggered byproject()/enable_language()). Consider setting it beforeproject()so the deployment target is applied consistently (especially important if CI stops passing-DCMAKE_OSX_DEPLOYMENT_TARGET).
cmake_minimum_required(VERSION 3.5)
project(node_window_rendering)
if(APPLE)
if (NOT CMAKE_OSX_ARCHITECTURES)
set(CMAKE_OSX_ARCHITECTURES "${CMAKE_HOST_SYSTEM_PROCESSOR}")
endif()
if (NOT CMAKE_OSX_DEPLOYMENT_TARGET)
if ("${CMAKE_OSX_ARCHITECTURES}" STREQUAL "arm64")
set(CMAKE_OSX_DEPLOYMENT_TARGET "11.0")
else()
set(CMAKE_OSX_DEPLOYMENT_TARGET "10.15")
endif()
endif()
endif()
💡 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=10.15 \ | ||
| -DCMAKE_OSX_DEPLOYMENT_TARGET=${CMAKE_OSX_DEPLOYMENT_TARGET} \ | ||
| -DCMAKE_BUILD_TYPE=RelWithDebInfo \ | ||
| -DCMAKE_INSTALL_PREFIX=${FULL_DISTRIBUTE_PATH}/node-window-rendering \ |
There was a problem hiding this comment.
@copilot can you re-evaluate this branch? I addressed this defect
There was a problem hiding this comment.
Re-reviewed this branch: acd1d0b removes the empty -DCMAKE_OSX_DEPLOYMENT_TARGET= override from ci/build-osx.sh, so CMake now falls back to the architecture-based defaults in CMakeLists.txt (10.15 for x86_64, 11.0 for arm64). No further change is needed here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
ci/build-osx.sh:36
CMAKE_OSX_DEPLOYMENT_TARGETis not set anywhere in this script or the workflow env, so this expands to an empty-DCMAKE_OSX_DEPLOYMENT_TARGET=argument. That can populate the CMake cache with an empty deployment target and prevent the default logic inCMakeLists.txtfrom taking effect. Only pass this-Dflag when the variable is non-empty.
-DCMAKE_OSX_DEPLOYMENT_TARGET=${CMAKE_OSX_DEPLOYMENT_TARGET} \
No description provided.