-
Notifications
You must be signed in to change notification settings - Fork 4
Use CMake file API to read shared library target paths #254
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
Conversation
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 CMake file API integration to dynamically read shared library target paths instead of relying on pre-defined directory locations. This fixes issue #253 by introducing a wrapper around the CMake file API to query and read metadata for targets in build directories.
- Adds a new
cmake-file-api.ts
module with comprehensive Zod schemas for parsing CMake file API responses - Updates both Apple and Android platform implementations to use the file API for discovering shared library artifacts
- Replaces hardcoded file discovery logic with dynamic target querying based on CMake metadata
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/cmake-rn/src/cmake-file-api.ts | New module implementing CMake file API wrapper with Zod schemas and query/parsing functions |
packages/cmake-rn/src/cmake-file-api.test.ts | Unit tests for the file API functionality, specifically testing reply index path resolution |
packages/cmake-rn/src/platforms/apple.ts | Updated to use CMake file API instead of directory scanning for shared library discovery |
packages/cmake-rn/src/platforms/android.ts | Updated to use CMake file API instead of directory scanning for shared library discovery |
packages/cmake-rn/src/cli.ts | Added query creation for codemodel-v2 API during project configuration |
packages/cmake-rn/package.json | Added zod dependency for schema validation |
.changeset/evil-vans-love.md | Changeset documenting the patch-level change |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Can you cover a few more exceptional cases to make sure there's feedback up to the CLI with actionable output? A couple of cases
I'm okay to save these for a separate PR but before tagging a release. I'm mostly thinking about support load :) |
I've pushed a commit removing a lot of the schema code I already added. Instead, I'm going to use CoPilot to generate a wrapper around the CMake file API from their docs, as a separate package, similar to https://crates.io/crates/cmake-file-api for Rust. This will hopefully host a much more comprehensive test suite. That being said, we want to test our implementation of the specification, not CMake. |
1715032
to
eae0a78
Compare
eae0a78
to
a7fdfe9
Compare
I've opened #257 now and rebased this PR on top of that. |
f594b7b
to
52ec31d
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
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
// TODO: Make this call async | ||
cp.spawnSync("install_name_tool", [ |
Copilot
AI
Sep 28, 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 TODO comment indicates this synchronous call should be made async, but the function has already been converted to async. Consider using cp.spawn
with promises or a promisified version to maintain consistency with the rest of the async operations.
// TODO: Make this call async | |
cp.spawnSync("install_name_tool", [ | |
// Make this call async | |
await spawn("install_name_tool", [ |
Copilot uses AI. Check for mistakes.
a8e860c
to
4d0b7df
Compare
This PR is stacked on #257 and fixes #253 by introducing use of the CMake file API to read shared library target paths.
Merging this PR will:
cmake-rn
fails if projects emit more than one dynamic library #201 by leaning on the CMake structure to group shared library artifacts.Note
Switches artifact discovery to the CMake File API and updates Android/Apple post-build flows and host helpers to assemble outputs per-target using the new APIs.
cmake-file-api
: add dependency and initialize query (codemodel
v2) insrc/cli.ts
.postBuild
: read targets via CMake File API, group shared libraries by target, and assemble per-library.android.node
outputs using updated host API; includeconfiguration
.postBuild
: read targets via CMake File API, generate frameworks and assemble per-library XCFrameworks (or.apple.node
).createAndroidLibsDirectory
signature tolibraries: { triplet, libraryPath }[]
; adjust implementation.createAppleFramework
async and usespawn
/fs.promises
.createAppleFramework
when building XCFrameworks.Written by Cursor Bugbot for commit 4d0b7df. This will update automatically on new commits. Configure here.