The combination of simctl install and simctl launch creates race conditions when using multiple simulators. This should both mitigate the effect of these race conditions and provide adequate information and logging for debugging when they do occur.
Created attachment 300155 [details] Patch
Comment on attachment 300155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300155&action=review > Tools/ChangeLog:3 > + Work around for simctl install/simctl launch bugs Work around => Workaround Is this title still correct? > Tools/ChangeLog:7 > + Please explain the bug that this patch is addressing, how it is addressing it (by adding a workaround), and what the workaround is. > Tools/Scripts/webkitpy/xcode/simulator.py:272 > + output = self._host.executive.run_command(['xcrun', 'simctl', 'listapps', self.udid]) > + if '/{}";\n'.format(os.path.basename(app_path)) in output: > + return True > + raise RuntimeError('Could not find {} installed on {}'.format(app_path, self.udid)) We should not need to do this. simctl install should guarantee that the app was installed upon return. If not, then this is a bug with "simctl install" and we should file it. > Tools/Scripts/webkitpy/xcode/simulator.py:284 > + # FIXME: This seems to be a bug in CoreSimulator, at least partially Please file a bug and reference the bug URL here. > Tools/Scripts/webkitpy/xcode/simulator.py:293 > + counter = 0 It would be more idiomatic to break out the loop here instead of setting counter to 0 and checking the loop condition again.
Comment on attachment 300155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300155&action=review >> Tools/ChangeLog:3 >> + Work around for simctl install/simctl launch bugs > > Work around => Workaround > > Is this title still correct? I believe so. At least from a usage perspective, the bug involves using these too commands very close together, it's not just a bug in one or the other.
Created attachment 300164 [details] Patch
Comment on attachment 300164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300164&action=review > Tools/ChangeLog:3 > + Workaround for simctl install/simctl launch bugs As far as I can tell we are working around a single bug here, <rdar://problem/30273973>. If there is a second bug then please explain it and/or reference its bug URL. If there is only one bug then please update the title of this bug and update this ChangeLog line. > Tools/ChangeLog:7 > + As I mentioned in comment 2, "please explain the bug that this patch is addressing, how it is addressing it (by adding a workaround), and what the workaround is." When explaining the bug it would be useful to note the error message that we are seeing when the bug occurs. > Tools/Scripts/webkitpy/xcode/simulator.py:278 > + # FIXME: This is a workaround for <rdar://problem/30273973>, Racey failure of simctl launch Very Minor: Please use complete sentences that end with a period. > Tools/Scripts/webkitpy/xcode/simulator.py:279 > + def error_handler(error): Minor: This is OK as-is. I would have defined this as a staticmethod named _log_debug_error() to avoid creating a function on each invocation of install_app. Hopefully a person will realize that this logging function is unused and will remove it when it comes time to remove the workaround assuming no other code makes use of it. > Tools/Scripts/webkitpy/xcode/simulator.py:283 > + for x in range(0, 3): Minor: This is OK as-is. Note that range() will allocate a list. I prefer your old approach of using a while loop. Alternatively we can use xrange() [1] that returns an iterator to be more efficient. Hopefully this workaround is short lived. [1] <https://docs.python.org/2/library/functions.html#xrange> > Tools/Scripts/webkitpy/xcode/simulator.py:291 > + ) > + match = re.match(r'(?P<bundle>[^:]+): (?P<pid>\d+)\n', output) > + if match: > + break Very Minor: This is OK as-is. Unlike your previous approach of special casing the workaround logic to a known failure message the approach in this patch will cause us to try to launch the app three times regardless of the simctl failure, logging the thrown exception to the debug log. I hope this workaround is short lived. Otherwise we should likely adopt your former approach and be more precise when we apply the workaround.
Created attachment 300180 [details] Patch
Comment on attachment 300180 [details] Patch Clearing flags on attachment: 300180 Committed r211405: <http://trac.webkit.org/changeset/211405>
All reviewed patches have been landed. Closing bug.