Store Apple prebuilds as .apple.node instead of .xcframework
#52
Conversation
|
kraenhansen
added
Apple 🍎
| export function replaceWithNodeExtension(modulePath: string) { | ||
| return path.format({ | ||
| ...path.parse(modulePath), | ||
| base: undefined, | ||
| ext: ".node", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Deleting this since the test was failing and it turned out it wasn't actually called from anywhere.
| const xcframeworkExtensionOption = new Option( | ||
| "--xcframework-extension", | ||
| "Don't rename the xcframework to .apple.node" | ||
| ).default(false); |
There was a problem hiding this comment.
I'm adding this option, since we're using this tool to build weak-node-api and we're manually referencing the output artifact via vendored_frameworks (no linking involved).
There was a problem hiding this comment.
Nit: I'm thinking whether this renaming should be something that the users needs to opt-in, instead of asking not to do it. Moreover, I'm a little afraid that the .apple.node extension might suggest that this will also work out-of-the-box on macOS (hinting that it might be compatible with "old-school" N-API addons & node.js). macOS can load dylibs directly and doesn't require packaging into Xcframeworks, and we are currently doing it only for mobile devices, right?
kraenhansen
force-pushed
the
kh/weak-node-api-injection
branch
from
629a063 to
74b48e1
Compare
kraenhansen
force-pushed
the
kh/apple-prebuilds-renamed
branch
from
51e41d9 to
1848720
Compare
| const xcframeworkExtensionOption = new Option( | ||
| "--xcframework-extension", | ||
| "Don't rename the xcframework to .apple.node" | ||
| ).default(false); |
There was a problem hiding this comment.
Nit: I'm thinking whether this renaming should be something that the users needs to opt-in, instead of asking not to do it. Moreover, I'm a little afraid that the .apple.node extension might suggest that this will also work out-of-the-box on macOS (hinting that it might be compatible with "old-school" N-API addons & node.js). macOS can load dylibs directly and doesn't require packaging into Xcframeworks, and we are currently doing it only for mobile devices, right?
| Cargo.lock | ||
|
|
||
| /*.xcframework/ | ||
| /*.apple.node/ |
There was a problem hiding this comment.
Nit: Shouldn't .node be reserved for binary files (shared libraries)? Aren't we breaking this assumption here, by making it a directory? In the Node-API docs I've found this quote (emphasis mine):
The string passed to
require()is the name of the target inbinding.gypresponsible for creating the.nodefile.
There was a problem hiding this comment.
We are definitely walking a fine line here and I agree it could cause confusion.
The "problem" is that one of the use-cases we want to support is taking an existing library (with source-code packaged in) and building the Apple and Android binaries for it.
If we were true to the spirit of the docs you referenced, we would have to override the ".node" file which was already included in the package and we couldn't use that for iOS and Android simultaneously. We would have to "swap out" the .node file for the right (platform+arch) at build time, just before it's copied into the app bundle. As an example, I know this is also what a tool like electron-builder is doing when it's building the final app bundle.
☝️ I feel that approach is not the right one, and I would like the prebuilds for different platforms and architectures to co-exist on the filesystem.
To my understanding, Node.js has no standardized way to publish multiple .node files (for multiple platforms / architectures) - we could spent some more time digging into the output formats of tools like:
(slightly unrelated, I found this extension point in node-gyp-build where a platform can implement require.addon https://github.com/prebuild/node-gyp-build/blob/6822ec52423a2b3ed48ef8960a9fe05902e9e1a3/index.js#L3 ... somewhat similar to our requireNodeAddon 🙂)
There was a problem hiding this comment.
Could you help me formulate a problem statement?
@shirakaba also wrote about this in this thread on discord: https://discord.com/channels/426714625279524876/1343407685462130780
kraenhansen
force-pushed
the
kh/weak-node-api-injection
branch
from
74b48e1 to
9578c63
Compare
kraenhansen
force-pushed
the
kh/apple-prebuilds-renamed
branch
from
1848720 to
8c83d1c
Compare
kraenhansen
force-pushed
the
kh/apple-prebuilds-renamed
branch
from
555a2a6 to
a61f9fa
Compare
2 participants
(stacked on #49)
Merging this PR will close #42 by:
This makes it easier for a developer to tell that the xcframework is meant for Node-API auto-linking and it is renamed to
.xframeworkwhen copied into the host package as part of auto-linking.