Build napi as a shared library on Android - #183
matthargett wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adjusts how the napi library is built so Android produces a shared libnapi.so, enabling separately dlopen’d native addons to resolve napi_* symbols via dynamic linking.
Changes:
- Build
napiasSHAREDon Android, otherwise keep existing default library-type behavior. - Add rationale in CMake comments about Android/bionic
dlopenand symbol resolution.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
cc @vmoroz @kraenhansen , thanks for generalizing your work into the upstream nodejs CTS! |
…PI_SHARED option)
2e2ad15 to
dd9365b
Compare
…PI_SHARED option)
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Link napi as a shared library (libnapi.so) on Android instead of statically into each consumer, so native addons can be dlopen'd as standalone .node modules and resolve their napi_* imports via a real DT_NEEDED. The host and every addon then share a single napi instance. Other platforms keep static napi.
- Don't hard-force SHARED: add a JSR_NAPI_SHARED CMake option (default ON on Android, OFF elsewhere)
so integrators can keep a static napi via -DJSR_NAPI_SHARED=OFF without patching the project.
- Fix the comment: the non-shared branch keeps add_library(napi ${SOURCES}), which follows the
project's default library type (BUILD_SHARED_LIBS), not necessarily static.
Hermes does not ship a js_native_api_hermes.cc -- its C napi_* functions live in the hermesNapi static library. Building napi as a SHARED library therefore produces a libnapi.so that does not carry those symbols, and everything linking it fails with undefined references (napi_wrap, napi_create_arraybuffer, napi_create_external, ...). The shared-napi default exists so dlopen'd .node addons can resolve napi_* at load; that harness is not built for Hermes, so keep napi static there.
6d06127 to
7eb0800
Compare
…PI_SHARED option)
|
On "no Android build validates the new shared/static napi configuration": upstream CI on fork PRs is held at the workflow-approval gate, which is why only GitGuardian reports here. The full matrix — including Android_V8, Android_JSC and Android_Hermes — runs on the fork twin, rebeckerspecialties#12. Both configurations are exercised there: Android_V8 / Android_JSC build with the default |
Build the
napitarget as a shared library (libnapi.so) on Android only; every other platform keeps the static library.Why
dlopen'd as standalone.nodemodules that resolvenapi_*through a realDT_NEEDED: the modelnodejs/node-api-ctsuses, and what the in-process conformance suite in Add N-API compliance tests #116 needs..sos, and addons can load on demand.lib/<abi>/*.sopackaging: no runtime code download and nodlopenfrom a writable directory, so Play and Quest rules are unaffected. Same mechanism aslibv8android.so.Embedder impact (Android only)
napinow carriesDT_NEEDED libnapi.so. AGPexternalNativeBuildand prefab/AAR package it automatically; a hand-rolled.soallow-list must add it, or the app fails at load withlibrary "libnapi.so" not found.napi_*symbols are exported fromlibnapi.so; exactly one napi instance per process.if(ANDROID)). A shared napi there would be a separate lift: an embedded, signed framework on iOS;napi.dllplus an import library on Windows.Verification
libnapi.sois built and packaged (lib/arm64-v8a/libnapi.so), every consumer links it viaDT_NEEDED, and on an arm64 emulator (API 29, V8) a standalone.nodeaddon withnapi_*as undefined importsdlopens and resolvesnapi_register_module_v1against it.Stack
First of the N-API series: this → #116 (compliance tests) → #189 (N-API v7) → #258 (Worker).