Emit prebuilds per target, next to their sources - #413
Draft
kraenhansen wants to merge 1 commit into
Draft
Conversation
A project declaring multiple addons wrote every prebuild into a single
output directory, named after the CMake target. Both the location and the
name are now derived per target from the CMake File API:
- The output directory defaults to {targetSourceDir}/build/{configuration},
where the new {targetSourceDir} placeholder expands to the target's own
source directory. A single-addon project reports "." and so resolves to
the same path as before.
- The prebuild is named after the artifact on disk (the target's
OUTPUT_NAME) rather than the target name, so a target renamed to avoid a
clash within the project still produces the name the JS require expects.
Together this keeps a prebuild where the Babel plugin and auto-linking
resolve it from, and reduces --namespaced-targets to an internal concern.
Also fixes, in the same area:
- gyp-to-cmake emitted OUTPUT_NAME regardless of --namespaced-targets, due
to an always-truthy condition, and never emitted it for Apple framework
targets, which CMake names after it.
- The Apple build ran a full "cmake --build" once per shared library,
concurrently against one build tree, and called "xcodebuild -list" (a
synchronous spawn) once per library per triplet.
- xcodebuild invocations now run in sequence per build directory, as
concurrent invocations against a single Xcode project are not reliable.
- postBuild looked for "<target name>.framework" while createAppleFramework
names it after the artifact, so the two diverged under namespacing.
- --concurrency accepted any value, and did not implement the documented
fallback to 1 under --verbose. Max listeners is now derived from it.
- verify-prebuilds globbed a directory the prebuilds had moved out of, so
it passed by finding nothing. It now covers tests/ too and requires a
non-zero count.
- The root example project globbed recursively, which both missed examples
copied in after configure and would add a nested project twice. It is
now generated from the same script pipeline.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KfKQDvEkxNtkSE4aaF9yG8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #363, targeting
kh/multi-addon-projectsso the diff shows only the changes on top of it. Every change here touches code #363 introduces, so it does not stand alone againstmain.This takes a run at the open question in #363 — "I need to figure out how to handle the final output" — and fixes a set of defects found while reading that diff.
The output question
The flat output directory breaks the consumer side in two ways, and both have to be addressed together:
plugin.ts, andfindNodeAddonForBindingsinpath-utils.ts), so a prebuild at the package root is invisible to it.getLibraryNamederives the library name from the prebuild's path within its package, and the auto-linker renames the.sotolib${libraryName}.so. The plugin derives the same name from the require path. Move or rename the prebuild and the two disagree.Both are now derived per target from the CMake File API, rather than from the target name:
{targetSourceDir}/build/{configuration}. The new{targetSourceDir}placeholder expands totarget.paths.source, resolved against--source. A project declaring a single addon at the top level reports., so it resolves to the same path as before.OUTPUT_NAME— rather than the CMake target name.Together these reduce
--namespaced-targetsto a purely internal concern: it disambiguates target names within the project, and nothing downstream moves or changes name.Grouping stays keyed by CMake target name, which CMake guarantees unique within a project; the artifact name is deliberately not used as the key, since every addon may well build an
addon.node.Verified against real CMake
Configuring the generated root project (13 sub-projects) and running the new resolver over the File API output:
./build/RelWithDebInfois on thebindingslookup path, and the basenames are the original addon names, sorequire('bindings')('hello')resolves again. A single-addon project was separately confirmed to reportpaths.source === "."and emit to exactly the path it does today.Fixes
gyp-to-cmake
OUTPUT_NAMEwas emitted regardless of--namespaced-targets— the condition testedactualTargetName, which is always a non-empty string.OUTPUT_NAMEwas never set for Apple framework targets, which CMake names after it. Without it the framework, and the prebuild assembled from it, was named after the namespaced target. This is the change visible intests/*/CMakeLists.txt.cmake-rn / Apple
postBuildlooked for`${target name}.framework`whilecreateAppleFrameworknames it after the artifact. Under namespacing these diverge and the assert fires.build()ran a fullcmake --buildonce per shared library, concurrently against the same build tree — its arguments never depended on the library.listXcodeProject()is a synchronous spawn and was called once per library per triplet. Now once per build directory, and async.xcodebuildinvocations run in sequence per build directory; concurrent invocations against a single Xcode project and its derived data are not reliable."Building for multiple targets is not supported yet"guard, which made--targetunusable for the multi-addon case this work exists to serve.cmake-rn / CLI
--concurrencyaccepted anythingparseIntreturned, passingNaN/0intopLimit. Now rejected with a proper Commander usage error.--verbose, which the help text already claimed.EventEmitter.defaultMaxListenersis derived from the concurrency instead of being raised to 500. Each spawned child attaches threeprocesslisteners and removes them on exit, so the live count tracks concurrent children — whichpLimitalready bounds.node-addon-examples
verify-prebuildsglobbedexamples/, which the prebuilds had moved out of, so it passed by finding nothing. It now coverstests/too and asserts a non-zero count, so that failure mode cannot recur silently.CMakeLists.txtusedfile(GLOB_RECURSE ...), which is evaluated once at configure time (missing examples copied in afterwards) and, being recursive, wouldadd_subdirectory()both a parent and a nested project — three allow-list entries incopy-examples.mtsare commented out as "Brings its own CMake project 👀". It is now generated byscripts/generate-root-project.mts, which stops recursing at the firstCMakeLists.txton a path. Being generated fromexamples/(gitignored), the file is now gitignored too.findCMakeProjects/findCMakeProjectsRecursively, dead sincebuild-examples.mtswas deleted.Testing
Run on Linux:
npm run build,npx eslint .,prettier --check .— clean. (eslintreports 4 pre-existing errors inapps/test-app/App.tsx, unrelated and reproducible with these changes stashed.)npm test --workspace gyp-to-cmake— 30 pass, including new--namespaced-targetscoverage, of which there was none.npm test --workspace cmake-rn— 11 pass, including new coverage for the output-path resolution and artifact naming.npm test --workspace react-native-node-api— 43 pass, 4 fail. The 4 are permission-bit tests that cannot pass as root; nothing inpackages/hostis touched here.weak-node-apiconfig to confirm it configures without duplicate targets.Not verified: the Apple changes. This worker is Linux, and per
AGENTS.mdnative builds are not bootstrapped here, so thexcodebuildsequencing, the hoistedcmake --build, and the framework-path fix are reasoned from the code rather than run. The end-to-end check that matters isnpm test --workspace @react-native-node-api/node-addon-exampleson macOS, plus the Test app (iOS/Android) CI jobs — currentlyskippedon #363 because it is a draft.Opened as a draft to match #363.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KfKQDvEkxNtkSE4aaF9yG8
Generated by Claude Code