Skip to content

Conversation

kraenhansen
Copy link
Collaborator

@kraenhansen kraenhansen commented May 8, 2025

Merging this PR will:

  • Add a new ferric-modules package with a ferric bin, which calls into cargo to build dynamic libraries and package them into the prebuild formats expected by react-native-node-api-modules.
  • Add a "ferric-example` library package using napi.rs to define a simple function.
ferric-building.mov

Marking this as draft to get early feedback and because there's a few outstanding todos that need to be resolved before merging.

@kraenhansen kraenhansen self-assigned this May 8, 2025
Copy link

changeset-bot bot commented May 8, 2025

⚠️ No Changeset found

Latest commit: 5978142

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@mani3xis mani3xis left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

try {
return new Set(
cp
.execFileSync("rustup", ["target", "list", "--installed"], {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can grab all the targets and filter out installed ourselves? This might be handy if you would like to separate supported targets from available/installed. I've managed to do something like this here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could base the list of available targets on a call into "rustup" yeah. I would like that once I figure out how to install more of them (so I can test 😬).

"arm64-apple-tvos-sim": "arm64",
"arm64-apple-visionos": "arm64",
"arm64-apple-visionos-sim": "arm64",
} satisfies Record<AppleTriplet, AppleArchitecture>;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about detecting installed Xcode targets and then parsing the CPU arch from that? I'm afraid target availability depends on the SDK version. Maybe we can use some code from here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That could probably work 👍 And that would make it easier to adopt new SDKs in future versions of XCode, right? I mean - it wouldn't require a change to our tool.

One thing that I'd be mindful of is that we don't want building for all apple triplets (using --apple) becoming host machine dependent, as that could lead to xcframeworks with missing framework+dylib for an SDK they had not installed when building. I'd rather error in that case, so we do need to enumerate a list of Apple triplets for that.

'<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">',
'<plist version="1.0">',
" <dict>",
...Object.entries(values).flatMap(([key, value]) => [
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: If you wouldn't mind, I'd be happy to contribute my version (from here) as it serializes arrays, objects and strings + includes a tailing new-line. I've also noticed that Apple favors tab instead of spaces, but that's a minor thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is mostly code I'm moving, I (or you) can follow up with a PR changing that 👍 There is also plist 🙂

LINES.map((line, lineNumber, lines) => {
const ratio = lineNumber / lines.length;
return chalk.rgb(Math.round(250 - 100 * ratio), 0, 0)(line);
}).join("\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 👍

Copy link
Collaborator Author

@kraenhansen kraenhansen May 8, 2025

Choose a reason for hiding this comment

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

I probably spent half of my time building this tool, editing this file 😆

);
}

// TODO: Handle Apple and constructing an XCFramework
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Isn't it done above already? 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hah yes - that's a leftover 😅

): Promise<string[]> {
const result = [];
const darwinLibraries = [];
for (const [target, libraryPath] of libraries) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to be pushy, maybe we can reuse the groupByPlatformArch<>() function? It was handy to collect libs for Xcframework :) If not, then ignore this comment :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd happy to do that as a follow-up 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just looked at the groupByPlatformArch function again. It feels a bit overkill to what this needs (simply extracting the "darwin" libraries from what is known to be a list of target names) - I think it would also take some refactoring to make the types match up.

I'd be happy to merge a PR if you can fit it and even better if we could make use of it somewhere else as well, where the full feature of the function gets used 👍

@kraenhansen kraenhansen force-pushed the kh/ferric branch 3 times, most recently from bf48ac8 to 34c26c2 Compare May 11, 2025 10:43
@kraenhansen kraenhansen marked this pull request as ready for review May 11, 2025 22:40
@kraenhansen kraenhansen merged commit 1250fe5 into main May 11, 2025
3 checks passed
@kraenhansen kraenhansen deleted the kh/ferric branch May 11, 2025 22:48
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.

2 participants