-
Notifications
You must be signed in to change notification settings - Fork 4
Build for simulators by default #118
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.
do we expect that each new platform supported would be created in-repo as a new file? given the type-code embedded in the name, is it worth having a base interface (TargetPlatform
?) that includes platformIsSupported
, with concrete implementations (AppleTargetPlatform
, AndroidTargetPlatform
).
@matthargett I think that's a great question and something I don't have an immediate answer for. I could definitely do a follow-up PR with a refactor to separate out a type that needs implementation per platform 👍 |
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 adds default build targets for iOS simulator and Android emulator in both cmake-rn
and ferric
when no targets/triplets are specified, and provides helper functions to detect platform support.
- Default targets/triplets for Apple and Android are added when none are passed.
- Helper functions
isAndroidSupported
andisAppleSupported
are introduced. - User messages are updated to inform about default behavior instead of exiting.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/ferric/src/build.ts | Added default Android and Apple simulator targets and support checks |
packages/cmake-rn/src/cli.ts | Introduced default triplets for emulator/simulator and updated user messages |
packages/cmake-rn/src/apple.ts | Added isAppleSupported helper |
packages/cmake-rn/src/android.ts | Added isAndroidSupported helper |
Comments suppressed due to low confidence (4)
packages/cmake-rn/src/cli.ts:134
- [nitpick] The default-triplet logic is duplicated in both
cmake-rn
andferric
. Consider extracting it into a shared utility to reduce duplication.
if (triplets.size === 0) {
packages/ferric/src/build.ts:135
- [nitpick] The info message warns about defaults but doesn't list the actual targets chosen. Including
[...]
of default targets would make it clearer which targets are being used.
console.error(
packages/cmake-rn/src/cli.ts:133
- There are no tests verifying the new default-triplet behavior. Adding unit tests to cover scenarios where no triplets are provided would help prevent regressions.
if (triplets.size === 0) {
packages/cmake-rn/src/android.ts:98
- The function uses
fs.existsSync
butfs
is not imported. Addimport fs from 'fs';
at the top of the file.
export function isAndroidSupported() {
} | ||
if (isAppleSupported()) { | ||
if (process.arch === "arm64") { | ||
targets.add("aarch64-apple-ios-sim"); |
Copilot
AI
Jun 12, 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.
Only the ARM64 simulator is added for Apple; consider also adding x86_64-apple-ios-sim
when process.arch === "x64"
to cover Intel-based simulators.
targets.add("aarch64-apple-ios-sim"); | |
targets.add("aarch64-apple-ios-sim"); | |
} else if (process.arch === "x64") { | |
targets.add("x86_64-apple-ios-sim"); |
Copilot uses AI. Check for mistakes.
Merging for now, to keep things going. Please review in retrospect. |
Merging this PR will:
cmake-rn
andferric
to build for targets / triplets needed for iOS simulator and Android emulator.