-
Notifications
You must be signed in to change notification settings - Fork 7
Use apple frameworks #274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use apple frameworks #274
Conversation
55e2dcd to
b1e5821
Compare
cbe98e8 to
b49d0eb
Compare
b49d0eb to
71c1fa8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements Apple framework support as the default option for building Node-API addons on Apple platforms, addressing issue #258. The changes enable CMake to generate proper Apple frameworks instead of bare dynamic libraries, and refactor the build system to give platforms more control over the build process.
Key changes:
- Makes Apple frameworks the default build output by adding CMake framework properties to generated CMakeLists.txt files
- Refactors
cmake-rnplatform interfaces to allow platforms to directly spawn build commands rather than just returning arguments - Fixes weak-node-api build to produce frameworks on Apple platforms
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/node-addon-examples/tests/buffers/CMakeLists.txt | Updates project name and adds Apple framework configuration with option to disable |
| packages/node-addon-examples/tests/async/CMakeLists.txt | Updates project name and adds Apple framework configuration with option to disable |
| packages/host/weak-node-api/CMakeLists.txt | Converts weak-node-api to build as an Apple framework |
| packages/host/src/node/prebuilds/apple.ts | Adds versioned framework parameter to createAppleFramework function |
| packages/host/src/node/path-utils.ts | Adds dereferenceDirectory utility function for resolving symlinks |
| packages/host/src/node/path-utils.test.ts | Adds test coverage for dereferenceDirectory function |
| packages/host/src/node/index.ts | Exports dereferenceDirectory function |
| packages/gyp-to-cmake/src/transformer.ts | Adds appleFramework option and generates framework-specific CMake properties |
| packages/gyp-to-cmake/src/cli.ts | Adds --no-apple-framework flag and improves project name generation from package.json |
| packages/gyp-to-cmake/package.json | Adds pkg-dir and read-pkg dependencies |
| packages/ferric/src/build.ts | Updates createAppleFramework call with TODO for versioned frameworks |
| packages/cmake-rn/src/weak-node-api.ts | Extends getWeakNodeApiPath to accept "apple" triplet |
| packages/cmake-rn/src/platforms/types.ts | Refactors Platform interface to replace configure/buildArgs with configure/build methods |
| packages/cmake-rn/src/platforms/apple.ts | Implements new platform interface with xcodebuild support for frameworks |
| packages/cmake-rn/src/platforms/android.ts | Implements new platform interface for Android builds |
| packages/cmake-rn/src/helpers.ts | Extracts toDefineArguments helper function |
| packages/cmake-rn/src/cli.ts | Refactors to use new platform interface methods |
| packages/cmake-rn/package.json | Adds zod dependency |
| } else { | ||
| lines.push( | ||
| ...setTargetPropertiesLines({ | ||
| PREFIX: "", |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PREFIX property is set to an empty string instead of the quoted empty string \"\" that's used in the framework branch (line 158). This inconsistency will cause CMake to interpret the value differently. Both branches should use '\"\"' for consistency.
| PREFIX: "", | |
| PREFIX: '""', |
| recursive: true, | ||
| verbatimSymlinks: true, | ||
| }); | ||
| await fs.promises.unlink(tempPath); |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using unlink() on a directory will fail. Since tempPath is a directory (created from dirPath), this should use fs.promises.rmdir(tempPath, { recursive: true }) or fs.promises.rm(tempPath, { recursive: true }) instead.
| await fs.promises.unlink(tempPath); | |
| await fs.promises.rm(tempPath, { recursive: true, force: true }); |
packages/gyp-to-cmake/src/cli.ts
Outdated
| return name | ||
| .replace(/^@/g, "") | ||
| .replace(/\//g, "--") | ||
| .replace(/[^a-zA-Z0-9_]/g, "_"); |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex replacements will convert hyphens to underscores on line 31, but the comment in escapeBundleIdentifier suggests double underscores should become dots. This transformation is inconsistent with the bundle identifier escaping logic. Consider whether hyphens should be preserved or if this should align with escapeBundleIdentifier's approach.
| .replace(/[^a-zA-Z0-9_]/g, "_"); | |
| .replace(/[^a-zA-Z0-9_-]/g, "_"); |
| }, | ||
| buildArgs() { | ||
| async build({ spawn, triplet }, { build, target, configuration }) { | ||
| // const outputPath = path.join(buildPath, `${triplet.replace(/;/g, "_")}-out`) |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commented-out code should be removed rather than left in the codebase. If this was intended for future use, it should be captured in a TODO comment with context.
| // const outputPath = path.join(buildPath, `${triplet.replace(/;/g, "_")}-out`) |
packages/cmake-rn/src/cli.ts
Outdated
| ); | ||
|
|
||
| // Perform post-build steps for each platform in sequence | ||
| // console.log("📦 Writing prebuilds to:", prettyPath(out)); |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commented-out debugging statement should be removed from the production code.
| // console.log("📦 Writing prebuilds to:", prettyPath(out)); |
bf7c417 to
f116caa
Compare
f116caa to
36cb9a0
Compare
This fixes #258 by making this this only option.
Merging this PR will:
gyp-to-cmakeemit framework enabled shared object libraries passed the--apple-frameworkoption.cmake-rninternals to give platforms more control, giving them the ability to spawn child processes themselves instead of simply returning arguments, when configuring and building projects.weak-node-apiframework must use theVersions/Current/Resources/Info.plistconvention (rather than justInfo.plist) on macOS platforms #273 by adopting this when buildingweak-node-api.Missing docs and I might also be missing option 1 from #262 (copying xcframeworks as is).