-
Notifications
You must be signed in to change notification settings - Fork 40
fix: missing -sdk for getting build settings; running incompatible destinations #378
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
90e3a3d to
065e5cd
Compare
| const artifactName = await formatArtifactName({ | ||
| platform: 'ios', | ||
| traits: [ | ||
| args.destination?.[0] ?? 'simulator', |
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.
this was bug producing e.g. ios-generic/platform=iOS-Debug-{hash}, which would never resolve correctly for a remote artifact. cc @mdjastrzebski
| args: { | ||
| destination: [getGenericDestination(platformName, 'simulator')], | ||
| ...args, | ||
| }, | ||
| args, | ||
| destination: 'simulator', |
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.
@mdjastrzebski instead of manipulating args, I'm passing the destination as a 'simulator' | 'device' here and further resolve the genericDestination and sdk from that
| if (device.type !== deviceOrSimulator) { | ||
| throw new RnefError( | ||
| `Selected device "${device.name}" is not a ${deviceOrSimulator}. | ||
| Please select available ${deviceOrSimulator} device: | ||
| ${devices | ||
| .filter(({ type }) => type === deviceOrSimulator) | ||
| .map(({ name }) => `• ${name}`) | ||
| .join('\n')}` | ||
| ); | ||
| } |
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.
| function getSimulatorPlatformSDK(platform: ApplePlatform): PlatformSDK { | ||
| switch (platform) { | ||
| case 'ios': | ||
| return 'iphonesimulator'; | ||
| case 'macos': | ||
| return 'macosx'; | ||
| case 'tvos': | ||
| return 'appletvsimulator'; | ||
| case 'visionos': | ||
| return 'xrsimulator'; | ||
| } | ||
| } | ||
|
|
||
| function getDevicePlatformSDK(platform: ApplePlatform): PlatformSDK { | ||
| switch (platform) { | ||
| case 'ios': | ||
| return 'iphoneos'; | ||
| case 'macos': | ||
| return 'macosx'; | ||
| case 'tvos': | ||
| return 'appletvos'; | ||
| case 'visionos': | ||
| return 'xr'; | ||
| } | ||
| } |
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.
brought these back
| reactNativePath: string; | ||
| artifactName: string; | ||
| binaryPath?: string; | ||
| destination: 'simulator' | 'device'; |
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.
typedef DestinationType = simu | device

Summary
It's necessary to pass
-sdkalongside singular-destinationso that older Xcode versions can resolve the platform name properly. Otherwise it would useiphoneosinstead ofiphonesimulatorand make the resulting app path incorrect.Fixes #371
On top of that this PR improves the way running on iOS is handled when selecting
--destination:simulator, we calculate artifact name with simulator traits and only try to run on simulatorsdevice, we calculate artifact name with device traits and only try to run on devicesTest plan