-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Xcode 9 supports running multiple simulators #17284
Conversation
local-cli/runIOS/runIOS.js
Outdated
// but we want it to only launch the simulator | ||
} | ||
child_process.spawnSync('xcrun', ['simctl', 'boot', selectedSimulator.udid]); | ||
console.log(selectedSimulator) |
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.
semi: Missing semicolon.
Ping ✨ @hramos almost all green now! (failing tests are not related to my changes I assume) |
local-cli/runIOS/runIOS.js
Outdated
.then((appName) => { | ||
.then((udid) => new Promise((resolve) => { | ||
buildProject(xcodeProject, udid, scheme, args.configuration, args.packager, args.verbose) | ||
.then((appName) => resolve(udid, appName)); |
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 resolve call looks buggy
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.
I kind of agree, wasn't really happy with it either, but couldn't really think of a better way.. So I'm open to suggestions :)
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.
Oh now that I look at it again I think I can simply move the buildProject
call into the first promise. Let me try
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.
I've moved the invocation of buildProject
into the first promise. But now I wonder why it wasn't there in the first place..
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.
Another option would be to add the udid
to the resolve call in buildProject
, but that doesn't feel right either.
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.
Thanks for the PR. Looks really good! However, many people that I've met are still on an older version. For time being, can we check for the version with something like:
$ /usr/bin/xcodebuild -version
and decide to proceed / warn? Alternatively, can we log a message in case of run-ios
failing that it was expected on Xcode < 9?
In the future, we might want to drop it.
Well as far as I know there shouldn't be a problem if they do not give a Altho I'm not completely sure what happens if they do specify one when there's another booted already, and that's hard to test since I'm no longer having Xcode 8. But again, I think we should move forward, Xcode 9 has been around for over 3 months, this might be another good reason to upgrade. |
Oh but this I can do:
|
@shergin Does Facebook use Xcode 8 anymore? That'd help inform whether to keep legacy support around. |
@ide Facebook does not use |
@koenpunt I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
Ping ✨ |
@grabbou FYI; just brought my branch up-to-date with master |
Can you take a look at the failing tests? We recently got all tests passing. If you're sure the failure is unrelated, at least rebase the PR. That will trigger tests to run again. |
move buildProject invocation
to guide users with Xcode < 9 we now throw an informative error that running multiple simulators is only support in Xcode 9 and up
also changed spawnSync to execFileSync, because spawnSync didn't throw
@hramos tests are passing now. js_checks isn't, but I'm pretty sure that's unrelated. |
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.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: PR #17284 (accepted in 2ad3407) introduced a couple of regressions. ~1. There's the code:~ ``` .then((appName) => resolve(selectedSimulator.udid, appName)); /* ... */ .then((udid, appName) => { ``` ~~This makes `appName` to be always `undefined` as per `resolve` accepts only 1 argument. This regression causes issues if an app name differs from a scheme name.~ ~This PR fixes this by wrapping both values in an array.~ This was fixed in 589eae1. 2. The code ``` child_process.execFileSync('xcrun', ['simctl', 'boot', selectedSimulator.udid]); ``` makes a simulator *boot*, but the simulator *doesn't launch*. That's a regression, which forces developers to launch simulators by other means (by running a number of elaborate console commands, by running Xcode, or by running a simulator manually). This PR reverts that part of changes. Create a blank project with a name that differs from scheme name. Try to `react-native run-ios` in it. See that a simulator is launched and installing succeeds. Without this changes simulator wouldn't launch, and installing step would fail because of app name mismatch. [CLI][BUGFIX][local-cli/runIOS/runIOS.js] - Fix running on multiple simulators feature regressions Closes #18711 Differential Revision: D7535150 Pulled By: hramos fbshipit-source-id: 5c714231e9977c0c829b6f8c793497cd31cd46b5
Summary: PR facebook#17284 (accepted in 2ad3407) introduced a couple of regressions. ~1. There's the code:~ ``` .then((appName) => resolve(selectedSimulator.udid, appName)); /* ... */ .then((udid, appName) => { ``` ~~This makes `appName` to be always `undefined` as per `resolve` accepts only 1 argument. This regression causes issues if an app name differs from a scheme name.~ ~This PR fixes this by wrapping both values in an array.~ This was fixed in 589eae1. 2. The code ``` child_process.execFileSync('xcrun', ['simctl', 'boot', selectedSimulator.udid]); ``` makes a simulator *boot*, but the simulator *doesn't launch*. That's a regression, which forces developers to launch simulators by other means (by running a number of elaborate console commands, by running Xcode, or by running a simulator manually). This PR reverts that part of changes. Create a blank project with a name that differs from scheme name. Try to `react-native run-ios` in it. See that a simulator is launched and installing succeeds. Without this changes simulator wouldn't launch, and installing step would fail because of app name mismatch. [CLI][BUGFIX][local-cli/runIOS/runIOS.js] - Fix running on multiple simulators feature regressions Closes facebook#18711 Differential Revision: D7535150 Pulled By: hramos fbshipit-source-id: 5c714231e9977c0c829b6f8c793497cd31cd46b5
The default value of --simulator should be null, or the booted device never be used. |
Summary: PR facebook#17284 (accepted in 2ad3407) introduced a couple of regressions. ~1. There's the code:~ ``` .then((appName) => resolve(selectedSimulator.udid, appName)); /* ... */ .then((udid, appName) => { ``` ~~This makes `appName` to be always `undefined` as per `resolve` accepts only 1 argument. This regression causes issues if an app name differs from a scheme name.~ ~This PR fixes this by wrapping both values in an array.~ This was fixed in 589eae1. 2. The code ``` child_process.execFileSync('xcrun', ['simctl', 'boot', selectedSimulator.udid]); ``` makes a simulator *boot*, but the simulator *doesn't launch*. That's a regression, which forces developers to launch simulators by other means (by running a number of elaborate console commands, by running Xcode, or by running a simulator manually). This PR reverts that part of changes. Create a blank project with a name that differs from scheme name. Try to `react-native run-ios` in it. See that a simulator is launched and installing succeeds. Without this changes simulator wouldn't launch, and installing step would fail because of app name mismatch. [CLI][BUGFIX][local-cli/runIOS/runIOS.js] - Fix running on multiple simulators feature regressions Closes facebook#18711 Differential Revision: D7535150 Pulled By: hramos fbshipit-source-id: 5c714231e9977c0c829b6f8c793497cd31cd46b5
Summary: Since Xcode 9 you can run multiple simultaneously. And since I believe React Native advocates using the latest version of Xcode, we can safely remove this constraint. Updated the unit tests. Furthermore it can be found in the [Xcode release notes](https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/WhatsNewXcode/xcode_9/xcode_9.html#//apple_ref/doc/uid/TP40004626-CH8-SW12) that multiple simulators are now supported. This can be tested with the CLI by running `react-native run-ios` twice, but with a different `--simulator` flag, e.g.; react-native run-ios --simulator "iPhone SE" react-native run-ios --simulator "iPhone X" [IOS] [ENHANCEMENT] [local-cli/runIOS/findMatchingSimulator.js] - Allow running multiple simulators Closes facebook/react-native#17284 Differential Revision: D7102790 Pulled By: hramos fbshipit-source-id: 750e7039201e28a1feda2bec1e78144fd9deff98
Motivation
Since Xcode 9 you can run multiple simultaneously. And since I believe React Native advocates using the latest version of Xcode, we can safely remove this constraint.
Test Plan
Updated the unit tests. Furthermore it can be found in the Xcode release notes that multiple simulators are now supported.
This can be tested with the CLI by running
react-native run-ios
twice, but with a different--simulator
flag, e.g.;Release Notes
[IOS] [ENHANCEMENT] [local-cli/runIOS/findMatchingSimulator.js] - Allow running multiple simulators