see rdar://22334382
Created attachment 259414 [details] proposed patch The failure happens when a simulator is already running (due to previous run or some other reasons). Due to that the directory cannot be deleted since the simulator is writing some files to it. In this patch, we are quitting all the running simulator before attempting to delete the logs directory. Tested on local setup.
Comment on attachment 259414 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=259414&action=review > Tools/Scripts/webkitpy/port/ios.py:387 > + self._executive.run_command(['osascript', '-e', 'tell application id "com.apple.iphonesimulator" to quit']) > + quit_delay = 5 > + _log.debug('Waiting {seconds} seconds for any old iOS Simulator instance to quit ...'.format(seconds=quit_delay)) > + time.sleep(quit_delay) > if os.path.isdir(data_path): > shutil.rmtree(data_path) Is it necessary to remove the data directory? I mean, check_sys_deps() will conditionally erase the simulator device using "simctl erase", which will delete the data directory. If it is not necessary then I suggest we have this function be empty (except for the presence of the pass keyword - since Python does not allow a function to have an empty body). If it is necessary to remove the data directory here then can we make use of Simulator.check_simulator_device_and_erase_if_needed() or extract the logic from this function into a shared function that can be called here and in Simulator.check_simulator_device_and_erase_if_needed()? Can we make use of Simulator.wait_until_device_is_in_state() to wait until the booted simulator device is shutdown instead of waiting a hardcoded 5 seconds.
Comment on attachment 259414 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=259414&action=review > Tools/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=148197 Please include the radar URL, <rdar://problem/22334382>, under the Bugzilla bug URL.
Created attachment 259560 [details] Proposed patch - Quit the simulator after the testing is done, so that it is not already running during the next build. - Instead of quitting the simulator during the setup through the code, use kill-old-processes to kill the Simulator (will need another patch for buildbot).
Comment on attachment 259560 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=259560&action=review > Tools/BuildSlaveSupport/kill-old-processes:88 > + "Simulator" #FIXME: Consider moving iOS-Specific tasks to their own list Please put a space character after the '#', substitute "specific" for "Specific" and add a period to the end of this sentence. > Tools/Scripts/webkitpy/port/ios.py:-248 > - # testing_device will fail to boot if it is already booted. We assume that if testing_device > - # is booted that it was booted by the iOS Simulator app (as opposed to simctl). So, quit the > - # iOS Simulator app to shutdown testing_device. > - # FIXME: <rdar://problem/20916140> Switch to using CoreSimulator.framework for launching and quitting iOS Simulator > - self._executive.run_command(['osascript', '-e', 'tell application id "com.apple.iphonesimulator" to quit']) > - Simulator.wait_until_device_is_in_state(testing_device.udid, Simulator.DeviceState.SHUTDOWN) > - Actually, we need this code to handle the scenario where a person launched the iOS Simulator with simulator device testing_device and then subsequently runs run-webkit-test --ios-simulator (this is implied by the comment on lines 242 - 244). So, I suggest that we extract the common code into shared function, say _quitIOSSimulator, that we call from both here and clean_up_test_run(). Maybe something like: def _quitIOSSimulator(self): # FIXME: <rdar://problem/20916140> Switch to using CoreSimulator.framework for launching and quitting iOS Simulator self._executive.run_command(['osascript', '-e', 'tell application id "com.apple.iphonesimulator" to quit'])
(In reply to comment #5) > [...] > def _quitIOSSimulator(self): > # FIXME: <rdar://problem/20916140> Switch to using > CoreSimulator.framework for launching and quitting iOS Simulator > self._executive.run_command(['osascript', '-e', 'tell application id > "com.apple.iphonesimulator" to quit']) We may also want to take to this opportunity to extract the bundle identifier "com.apple.iphonesimulator" into a constant that can be shared by both the quit simulator logic (say, in IOSSimulatorPort._quitIOSSimulator()) and IOSSimulatorPort.setup_test_run().
(In reply to comment #5) > > Tools/Scripts/webkitpy/port/ios.py:-248 > > - # testing_device will fail to boot if it is already booted. We assume that if testing_device > > - # is booted that it was booted by the iOS Simulator app (as opposed to simctl). So, quit the > > - # iOS Simulator app to shutdown testing_device. > > - # FIXME: <rdar://problem/20916140> Switch to using CoreSimulator.framework for launching and quitting iOS Simulator > > - self._executive.run_command(['osascript', '-e', 'tell application id "com.apple.iphonesimulator" to quit']) > > - Simulator.wait_until_device_is_in_state(testing_device.udid, Simulator.DeviceState.SHUTDOWN) > > - > > Actually, we need this code to handle the scenario where a person launched > the iOS Simulator with simulator device testing_device and then subsequently > runs run-webkit-test --ios-simulator (this is implied by the comment on > lines 242 - 244). So, I suggest that we extract the common code into shared > function, say _quitIOSSimulator, that we call from both here and > clean_up_test_run(). Disregard this comment. We also need to move the logic for quitting the simulator from IOSSimulatorPort.check_sys_deps() to IOSSimulatorPort.reset_preferences(). Otherwise, we will hit this bug when a person launches the testing device in iOS Simulator and subsequently runs run-webkit-tests --ios-simulator.
Created attachment 259884 [details] Proposed patch Made the changes as discussed. Quitting the simulator both during the cleanup as well as at the beginning of layout_tests (during the reset_preferences).
Comment on attachment 259884 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=259884&action=review > Tools/ChangeLog:12 > + (IOSSimulatorPort._quitIOSSimulator): common function to quit IOSSimulator. Nit: common => Common IOSSimulator => iOS Simulator > Tools/ChangeLog:14 > + (IOSSimulatorPort.check_sys_deps): No need to quit the simulator here as its now being quit in reset_preferences. Nit: reset_preferences => reset_preferences() > Tools/Scripts/webkitpy/port/ios.py:80 > + SIMULATOR_BUNDLE_ID = "com.apple.iphonesimulator" Nit: ' (single quote) => " (double quote) > Tools/Scripts/webkitpy/port/ios.py:213 > + def _quitIOSSimulator(self): I didn't meant to steer you wrong on the naming of this function. We name functions with underscores between words. So, this function should be named _quit_ios_simulator.
Created attachment 259893 [details] Proposed patch
Comment on attachment 259893 [details] Proposed patch Thank you Aakash for updating the patch.
Comment on attachment 259893 [details] Proposed patch Clearing flags on attachment: 259893 Committed r188942: <http://trac.webkit.org/changeset/188942>
All reviewed patches have been landed. Closing bug.