Skip to content

Upgrade to Electron 43 - #22

Open
sandboxcoder wants to merge 2 commits into
streamlabsfrom
rno/upgrade-electron43
Open

Upgrade to Electron 43#22
sandboxcoder wants to merge 2 commits into
streamlabsfrom
rno/upgrade-electron43

Conversation

@sandboxcoder

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.0 in 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_TARGET is set after project(). Now that the variable name typo is fixed, this block may still not reliably affect compiler/linker flags because CMAKE_OSX_DEPLOYMENT_TARGET is typically consumed during language/toolchain initialization (triggered by project() / enable_language()). Consider setting it before project() 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.

Comment thread ci/build-osx.sh
Comment on lines 34 to 38
# 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 \

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_TARGET is 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 in CMakeLists.txt from taking effect. Only pass this -D flag when the variable is non-empty.
-DCMAKE_OSX_DEPLOYMENT_TARGET=${CMAKE_OSX_DEPLOYMENT_TARGET} \

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants