| Summary: | Use simctl instead of LayoutTestRelay | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | ap, commit-queue, dbates, glenn, lforschler | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version: | Other | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | iPhone / iPad | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=166660 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Description
Jonathan Bedard
2016-12-15 16:10:13 PST
Created attachment 297242 [details]
Patch
Created attachment 297244 [details]
Patch
Created attachment 297250 [details]
Patch
Created attachment 297325 [details]
Patch
Attachment 297325 [details] did not pass style-queue:
ERROR: Tools/WebKitTestRunner/ios/TestControllerIOS.mm:35: Bad include order. Mixing system and custom headers. [build/include_order] [4]
Total errors found: 1 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
The most recent patch keeps the LayoutTestRelay directory, there is a bug in webkit-patch which prevents these changes from being run through EWS. https://bugs.webkit.org/show_bug.cgi?id=165953 is tracking this bug, but this patch really needs an EWS check, so for the time being, LayoutTestRelay will remain integrated. Created attachment 297333 [details]
Patch
Attachment 297333 [details] did not pass style-queue:
ERROR: Tools/WebKitTestRunner/ios/TestControllerIOS.mm:35: Bad include order. Mixing system and custom headers. [build/include_order] [4]
Total errors found: 1 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 297341 [details]
Patch
Attachment 297341 [details] did not pass style-queue:
ERROR: Tools/WebKitTestRunner/ios/TestControllerIOS.mm:35: Bad include order. Mixing system and custom headers. [build/include_order] [4]
Total errors found: 1 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
An additional note: these changes have been fully tested on Windows as well, and do not break server_process on the Windows port. Dan Bates and I discussed this patch in person. Removing the review flag as it will be split into multiple patches. Created attachment 298112 [details]
Patch
Created attachment 298113 [details]
Patch
Attachment 298112 [details] will be used as the final patch, but due to https://bugs.webkit.org/show_bug.cgi?id=165953, EWS cannot be applied to it. For the time being, attachment 298113 [details] has all of the changes except the actual removal of the Tool/LayoutTestRelay directory. Created attachment 298114 [details]
Patch
Comment on attachment 298114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298114&action=review I am still reading through the patch. Here are some notes so far. > Tools/Scripts/webkitpy/port/darwin.py:178 > + def extractBundleID(self, app): Nit: The name of this function should use words separated by underscores instead of CamelCase. > Tools/Scripts/webkitpy/port/darwin.py:179 > + return self._executive.run_command(['/usr/libexec/PlistBuddy', '-c', 'Print CFBundleIdentifier', app + '/Info.plist']).rstrip() Take app to be the path to a Mac app bundle, say Safari.app. Then PlistBuddy will return a nonzero exit status because the file "path to Safari.app"/Info.plist does not exit. Either we make this work for all Darwin ports or we should move this function to the iOS Simulator port. > Tools/Scripts/webkitpy/port/driver.py:618 > + dump_tool] Nit: The ']' should be on its own line that is left aligned with the start of the name of the variable the array is assigned to (as was done prior to this change). > Tools/Scripts/webkitpy/port/ios.py:216 > def _build_driver(self): > built_tool = super(IOSSimulatorPort, self)._build_driver() > - built_relay = self._build_relay() > - return built_tool and built_relay > + return built_tool Please remove this method as it is functionally equivalent to its base class implementation. > Tools/Scripts/webkitpy/port/server_process.py:55 > +class ServerProcessBase(object): We prefer to have one class per file to keep files small and focused. Please move this class to another file, say server_process_base.py. > Tools/Scripts/webkitpy/port/server_process.py:67 > self._pid = None Is it necessary to keep this instance variable? > Tools/Scripts/webkitpy/port/server_process.py:81 > + def _grabData(self, deadline): Can we come up with a better name for this function? I feel the name of this function, including the word "grab", is more vague that the prefix "_wait_for_data_and_update_buffers" used on the platform-specific functions being replaced by this function. Maybe _platform_wait_for_data_and_update_buffers would be a more descriptive name? > Tools/Scripts/webkitpy/port/server_process.py:448 > + if self._stdin: > + os.close(self._stdin) > + if self._stdout: > + os.close(self._stdout) > + if self._stderr: > + os.close(self._stderr) > + self._stdin = None > + self._stdout = None > + self._stderr = None I would write this as: if self._stdin: os.close(self._stdin) self._stdin = None if self._stdout: os.close(self._stdout) self._stdout = None if self._stderr: os.close(self._stderr) self._stderr = None > Tools/Scripts/webkitpy/port/server_process.py:457 > + def __unlink_make_fifo(self, path): > + try: > + os.unlink(path) > + except OSError as e: > + if os.path.exists(path): > + raise OSError("Could not unlink '" + path + "', the target path is a file not a pipe.") > + os.mkfifo(path, 0666) I understand the temptation to combine removal of the FIFO and creation of the FIFO into a common function. This has the disadvantage that we do not clean up the FIFO files on process termination. I suggest that we move the os.unlink() logic to SimulatorProcess._reset() and move os.mkfifo() logic to SimulatorProcess._start() so that we always remove the FIFO files on process termination. On another note, I know that you are using the same umask of 0666 for the FIFOs as LayoutTestRelay used. Is it necessary to be so permissive? We do not need to fix this up now. Comment on attachment 298114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298114&action=review >> Tools/Scripts/webkitpy/port/darwin.py:179 >> + return self._executive.run_command(['/usr/libexec/PlistBuddy', '-c', 'Print CFBundleIdentifier', app + '/Info.plist']).rstrip() > > Take app to be the path to a Mac app bundle, say Safari.app. Then PlistBuddy will return a nonzero exit status because the file "path to Safari.app"/Info.plist does not exit. Either we make this work for all Darwin ports or we should move this function to the iOS Simulator port. I think we should make this work for all Darwin ports. It should be pretty trivial anyways. >> Tools/Scripts/webkitpy/port/server_process.py:67 >> self._pid = None > > Is it necessary to keep this instance variable? Yes. The _pid is different from the _proc and is not unique to ServerProcess. >> Tools/Scripts/webkitpy/port/server_process.py:457 >> + os.mkfifo(path, 0666) > > I understand the temptation to combine removal of the FIFO and creation of the FIFO into a common function. This has the disadvantage that we do not clean up the FIFO files on process termination. I suggest that we move the os.unlink() logic to SimulatorProcess._reset() and move os.mkfifo() logic to SimulatorProcess._start() so that we always remove the FIFO files on process termination. > > On another note, I know that you are using the same umask of 0666 for the FIFOs as LayoutTestRelay used. Is it necessary to be so permissive? We do not need to fix this up now. Thinking about this, no. At the very least the stdin queue can be 644, and it is possible that (assuming the simulators are the same user as the scripts) that 600 will work. I'll try those and update the patch. Created attachment 298480 [details]
Patch
Created attachment 298485 [details]
Patch
Comment on attachment 298485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298485&action=review Discussed in person with Dan Bates. Will be noting some of the high-level comments here. > Tools/Scripts/webkitpy/port/simulator_process.py:35 > + Give SimulatorProcess a _proc which resembles the _proc used by server_process and moved function involving _proc which are sufficiently similar to server_process_base > Tools/Scripts/webkitpy/xcode/simulator.py:639 > + Move these function to the device class. Comment on attachment 298485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298485&action=review > Tools/Scripts/webkitpy/port/darwin.py:178 > + def extract_bundle_ID(self, app): Maybe a better name for this method would be app_identifier_from_bundle? This would match the name of the analogous Perl function we have in webkitdirs.pm. > Tools/Scripts/webkitpy/port/driver.py:625 > -# FIXME: this should be abstracted out via the Port subclass somehow. > class IOSSimulatorDriver(Driver): > def cmd_line(self, pixel_tests, per_test_args): > cmd = super(IOSSimulatorDriver, self).cmd_line(pixel_tests, per_test_args) > - relay_tool = self._port.relay_path > dump_tool = cmd[0] > dump_tool_args = cmd[1:] > - product_dir = self._port._build_path() > relay_args = [ > - '-developerDir', self._port.developer_dir, > - '-udid', self._port.device_id_for_worker_number(self._worker_number), > - '-productDir', product_dir, > - '-app', dump_tool, > + self._port.device_id_for_worker_number(self._worker_number), > + dump_tool, > ] > - return [relay_tool] + relay_args + ['--'] + dump_tool_args > + return relay_args + ['--'] + dump_tool_args > > def _setup_environ_for_driver(self, environment): > environment['DEVELOPER_DIR'] = self._port.developer_dir This class is not very meaningful anymore. As far as I can tell it only differs from the behavior of its base class in that it sets the environment variable DEVELOPER_DIR to the path to the Xcode developer directory and constructs an invalid command line as a means to pass the UDID of the device to use to the ServerProcess object. I do not believe the former is necessary now that we are moving to use simctl to interact with the Simulator instead of the CoreSimulator APIs and the latter is an abuse of the purpose of the Driver.cmd_line() method to return a valid command line to invoke so as to pass additional data to an instantiated ServerProcess. One way to avoid such an abuse is to modify the ServerProcess constructor to take a worker number (is there a better way?). > Tools/Scripts/webkitpy/port/simulator_process.py:34 > +class SimulatorProcess(ServerProcessBase): Towards using more of the ServerProcessBase functionality and avoid the need to duplicate its logic in this derived class I suggest that we define a Popen-like class, say SimulatorPopen, and have SimulatorProcess._start() instantiate an instance of it and store it in ServerProcessBase._proc. > Tools/Scripts/webkitpy/port/simulator_process.py:121 > + def poll(self): > + if not self.is_started(): > + return None > + try: > + temp = os.read(self._stderr, 128) > + if temp: > + self._error += temp > + else: > + return True > + except OSError as e: > + pass > + except: > + return True > + return False We should implement poll() such that it checks that the simulator process actually running as opposed to indirectly checking this condition by trying to read from stderr. We can make use of a kqueue and kqueue events to implement the polling behavior efficiently. See <https://docs.python.org/2/library/select.html> for more details. Assuming we go with the approach of using SimulatorPopen to represent a Popen-like object then such polling logic should be in SimulatorPopen.poll() and we can remove this method and use the base classes ServerProcessBase.poll() as it will call SimulatorPopen.poll() via self._proc. > Tools/Scripts/webkitpy/xcode/simulator.py:617 > + def install(self, device, app): Maybe a better name for this method and the parameter app would be install_app and bundle_identifier, respectively. I suggest that we move this function to class Device since it operates on a device. Then we can remove the device parameter from the function prototype. > Tools/Scripts/webkitpy/xcode/simulator.py:622 > + def run(self, device, app_id, args, env=None): Maybe a better name for this method and the parameter app_id would be launch_app and bundle_identifier, respectively. I suggest that we move this function to class Device since it operates on a device. Then we can remove the device parameter from the function prototype. > Tools/Scripts/webkitpy/xcode/simulator.py:635 > + def kill(self, device, app_id): Maybe a better name for this function and the parameter app_id would be terminate_app/kill_app and bundle_identifier, respectively. I suggest that we move this function to class Device since it operates on a device. Then we can remove the device parameter from the function prototype. Comment on attachment 298485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298485&action=review >> Tools/Scripts/webkitpy/port/driver.py:625 >> environment['DEVELOPER_DIR'] = self._port.developer_dir > > This class is not very meaningful anymore. As far as I can tell it only differs from the behavior of its base class in that it sets the environment variable DEVELOPER_DIR to the path to the Xcode developer directory and constructs an invalid command line as a means to pass the UDID of the device to use to the ServerProcess object. I do not believe the former is necessary now that we are moving to use simctl to interact with the Simulator instead of the CoreSimulator APIs and the latter is an abuse of the purpose of the Driver.cmd_line() method to return a valid command line to invoke so as to pass additional data to an instantiated ServerProcess. One way to avoid such an abuse is to modify the ServerProcess constructor to take a worker number (is there a better way?). I agree. I'm not sure what the best way to remove it is, though. I think for the time being, I'd like to keep this class around. I will have a patch later which removes it, I think given the scope of this patch, this change should wait. Created attachment 298790 [details]
Patch
Attachment 298790 [details] did not pass style-queue:
ERROR: Tools/Scripts/webkitpy/port/simulator_process.py:122: [SimulatorProcess._kill] Undefined variable 'elf' [pylint/E0602] [5]
ERROR: Tools/Scripts/webkitpy/port/simulator_process.py:111: [SimulatorProcess._start] Access to member '_stdin' before its definition line 113 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:92: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:93: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:94: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:95: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:96: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:97: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:98: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:99: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:100: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
Total errors found: 11 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 298791 [details]
Patch
Attachment 298791 [details] did not pass style-queue:
ERROR: Tools/Scripts/webkitpy/port/simulator_process.py:111: [SimulatorProcess._start] Access to member '_stdin' before its definition line 113 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:91: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:92: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:93: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:94: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:95: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:96: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:97: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:98: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:99: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
ERROR: Tools/Scripts/webkitpy/port/server_process_base.py:100: [ServerProcessBase._reset] Access to member '_proc' before its definition line 102 [pylint/E0203] [5]
Total errors found: 11 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 298792 [details]
Patch
Created attachment 298794 [details]
Removes layout test relay
Comment on attachment 298792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298792&action=review > Tools/Scripts/webkitpy/port/simulator_process.py:35 > +class fifoProcess(object): Can we come up with a better name? Maybe SimulatorProcessPopen? We should nest this class into SimulatorProcess. > Tools/Scripts/webkitpy/port/simulator_process.py:37 > + def __init__(self, stdin_handle, stdout_handle, stderr_handle, pid): These should take the Python File objects instead of file descriptors. > Tools/Scripts/webkitpy/port/simulator_process.py:71 > + self._bundleID = self._port.app_identifier_from_bundle(self._cmd[1]) This variable name does not conform to PEP-8 > Tools/Scripts/webkitpy/port/simulator_process.py:73 > + self._device.install_app(self._cmd[1]) We should assert that this does not return None. > Tools/Scripts/webkitpy/port/simulator_process.py:88 > + def __unlink_check_exists(self, path): > + try: > + os.unlink(path) > + except OSError as e: > + if os.path.exists(path): > + raise OSError("Could not unlink '" + path + "', the target path is a file not a pipe.") I do not see the value in the try/except and there is a time of check to time of use bug. I think we should inline the os.unlink() call into the caller and let it throw an exception if it does. > Tools/Scripts/webkitpy/port/simulator_process.py:98 > + def _start(self): > + os.mkfifo(self._in_path, 0600) I would have expected we do the same check for non-None self._proc and call _reset() here as in the base class. > Tools/Scripts/webkitpy/port/simulator_process.py:106 > + stdout = os.open(self._out_path, os.O_RDONLY | os.O_NONBLOCK) > + stderr = os.open(self._error_path, os.O_RDONLY | os.O_NONBLOCK) I suggest that we use the higher-level open() call to open these and then have a convenience functions, say set_non_blocking(), that take a Python File object and makes non-blocking. > Tools/Scripts/webkitpy/port/simulator_process.py:112 > + except OSError as e: > + self._crashed = True > + if stdout: > + os.close(stdout) > + if stderr: > + os.close(stderr) This is just to defend against the FIFO being deleted between the time the app launches and now, right? > Tools/Scripts/webkitpy/xcode/simulator.py:267 > + if self._host.executive.run_command(['xcrun', 'simctl', 'install', self.udid, app_path], return_exit_code=True) > 0: > + return False > + return True I would write this as: return !self._host.executive.run_command(['xcrun', 'simctl', 'install', self.udid, app_path], return_exit_code=True) > Tools/Scripts/webkitpy/xcode/simulator.py:270 > + transferedEnvironment = {} Maybe a better name for this would be environment_to_use? At least make this name conform to PEP-8 naming conventions. > Tools/Scripts/webkitpy/xcode/simulator.py:273 > + if not "SIMCTL_CHILD_" in value: Take value := "X_SIMCTL_CHILD_Y" then we will not prepend SIMCTL_CHILD_ to value. > Tools/Scripts/webkitpy/xcode/simulator.py:276 > + str = self._host.executive.run_command(['xcrun', 'simctl', 'launch', self.udid, bundle_identifier] + args, env=transferedEnvironment) Can we come up with a better name for this variable? Maybe output? > Tools/Scripts/webkitpy/xcode/simulator.py:280 > + if not bundle_identifier in str: > + return None > + str = str.rstrip('\n').split()[1] > + return int(str) This seems brittle. I suggest that we match against a regular expression. > Tools/Scripts/webkitpy/xcode/simulator.py:285 > + if self._host.executive.run_command(['xcrun', 'simctl', 'terminate', self.udid, bundle_identifier], return_exit_code=True) > 0: > + return False > + return True I would write this as: return !self._host.executive.run_command(['xcrun', 'simctl', 'terminate', self.udid, bundle_identifier], return_exit_code=True) (In reply to comment #23) > Comment on attachment 298485 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298485&action=review > > >> Tools/Scripts/webkitpy/port/driver.py:625 > >> environment['DEVELOPER_DIR'] = self._port.developer_dir > > > > This class is not very meaningful anymore. As far as I can tell it only differs from the behavior of its base class in that it sets the environment variable DEVELOPER_DIR to the path to the Xcode developer directory and constructs an invalid command line as a means to pass the UDID of the device to use to the ServerProcess object. I do not believe the former is necessary now that we are moving to use simctl to interact with the Simulator instead of the CoreSimulator APIs and the latter is an abuse of the purpose of the Driver.cmd_line() method to return a valid command line to invoke so as to pass additional data to an instantiated ServerProcess. One way to avoid such an abuse is to modify the ServerProcess constructor to take a worker number (is there a better way?). > > I agree. I'm not sure what the best way to remove it is, though. What about the way I describe above to pass the worker number when instantiating the ServerProcess via _test_runner_process_constructor()? > I think for the time being, I'd like to keep this class around. I will have a patch > later which removes it, I think given the scope of this patch, this change > should wait. I would prefer to remove this class now as it only exists to pass the UDID of the device associated with the worker number and it seems straightforward to pass the worker number using the above idea (though it may seem a bit ugly). Comment on attachment 298792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298792&action=review > Tools/Scripts/webkitpy/port/darwin.py:178 > + def app_identifier_from_bundle(self, app): I suggest that we rename the second argument from app to bundle so as to be consistent with the name of the function. > Tools/Scripts/webkitpy/port/darwin.py:179 > + plist_path = app + '/Info.plist' We should be using the FileSystem object (self._filesystem) to do this path join instead of doing it ourselves. > Tools/Scripts/webkitpy/port/darwin.py:181 > + plist_path = plist_path = app + '/Contents/Info.plist' Ditto. > Tools/Scripts/webkitpy/port/server_process.py:-45 > - import fcntl ServerProcess._start() is making use of this module. How is this code working without importing it? I take it we are importing this module through some other import. We may not need to make this change if we keep the original ServerProcess class and do not create a ServerProcessBase class. See my remark below for more details. > Tools/Scripts/webkitpy/port/server_process.py:45 > > -class ServerProcess(object): > - """This class provides a wrapper around a subprocess that > - implements a simple request/response usage model. The primary benefit > - is that reading responses takes a deadline, so that we don't ever block > - indefinitely. The class also handles transparently restarting processes > - as necessary to keep issuing commands.""" > +class ServerProcess(ServerProcessBase): When I originally suggested to you to extract the common functionality of this class and SimulatorProcess into a shared base class, ServerProcessBase, I envisioned that ServerProcessBase would be platform-independent code and the derived classes would implement the platform-specific code. The proposed change seems to just move code from ServerProcess to ServerProcessBase such that both ServerProcess and ServerProcessBase contain platform-specific code. I do not see the value in such a refactoring. And it may be sufficient for our purposes and time-efficient to keep the original ServerProcess class (with its combined POSIX and Windows implementations) and have class SimulatorProcess extend ServerProcess. > Tools/Scripts/webkitpy/port/simulator_process.py:54 > + try: > + os.kill(self.pid, 0) > + except OSError, err: > + if err.errno == errno.ESRCH: > + return 1 > + elif err.errno == errno.EPERM: > + return None > + else: > + print -1 > + else: > + return None This implementation not conform to the description of Popen.poll(). In particular, poll() must "Set and return returncode attribute." [1]. That is, we need to define an instance variable called returncode and this method needs to set returncode to the appropriate value and then return returncode. [1] <https://docs.python.org/2/library/subprocess.html#subprocess.Popen.poll> > Tools/Scripts/webkitpy/port/simulator_process.py:56 > + # Per subprocess's documentation, this mimicks it's behavior I do not feel that this comment adds value. Please remove it. > Tools/Scripts/webkitpy/port/simulator_process.py:60 > + def wait(self): > + while not self.poll(): > + time.sleep(.01) > + return self.poll() This implementation does not conform to the description of Popen.wait(). In particular, Popen.wait() must "Set and return returncode attribute." [2]. Much of this behavior will fallout naturally once we fix Popen.poll(). As an optimization we should return immediately if we have a non-None returncode attribute. [2] <https://docs.python.org/2/library/subprocess.html#subprocess.Popen.wait> Created attachment 299251 [details]
Patch
Comment on attachment 299251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299251&action=review > Tools/Scripts/webkitpy/port/simulator_process.py:60 > + try: > + os.kill(self.pid, 0) > + except OSError, err: > + if err.errno == errno.ESRCH: > + return 1 > + elif err.errno == errno.EPERM: > + return None > + else: > + return -1 > + else: > + return None Python's queue isn't the right choice for this for a few reasons. First, it requires setup and teardown, the os.kill approach does not. Second, while the kevent API can accept a pid, Python's kevent wrapper seems to only expect a file handle and will crash if it cannot access the provided argument as a file handle. > Tools/Scripts/webkitpy/port/simulator_process.py:114 > + self._proc.stdout = os.fdopen(os.open(self._out_path, os.O_RDONLY | os.O_NONBLOCK), "rb") > + self._proc.stderr = os.fdopen(os.open(self._error_path, os.O_RDONLY | os.O_NONBLOCK), "rb") If the opening of the file is not non-blocking, then this code will hang. stdout and stderr must open before the app starts, otherwise the app won't be able to attach to the files. (In reply to comment #34) > Comment on attachment 299251 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299251&action=review > > > Tools/Scripts/webkitpy/port/simulator_process.py:60 > > + try: > > + os.kill(self.pid, 0) > > + except OSError, err: > > + if err.errno == errno.ESRCH: > > + return 1 > > + elif err.errno == errno.EPERM: > > + return None > > + else: > > + return -1 > > + else: > > + return None > > Python's queue isn't the right choice for this for a few reasons. First, it > requires setup and teardown, the os.kill approach does not. OK. > Second, while the kevent API can accept a pid, Python's kevent wrapper seems to only > expect a file handle and will crash if it cannot access the provided > argument as a file handle. > This explanation is either incorrect or it implies there are bugs in Python's implementation. Regardless, sending the null signal to determine whether a process is running is sufficient. > > Tools/Scripts/webkitpy/port/simulator_process.py:114 > > + self._proc.stdout = os.fdopen(os.open(self._out_path, os.O_RDONLY | os.O_NONBLOCK), "rb") > > + self._proc.stderr = os.fdopen(os.open(self._error_path, os.O_RDONLY | os.O_NONBLOCK), "rb") > > If the opening of the file is not non-blocking, then this code will hang. > stdout and stderr must open before the app starts, otherwise the app won't > be able to attach to the files. You're right! We cannot use the built-in open(). Comment on attachment 299251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299251&action=review > Tools/Scripts/webkitpy/port/driver.py:371 > - self._server_process = self._port._server_process_constructor(self._port, self._server_name, self.cmd_line(pixel_tests, per_test_args), environment) > + self._server_process = self._port._test_runner_process_constructor(self._port, self._server_name, self.cmd_line(pixel_tests, per_test_args), environment, driver=self) How did you come to the decision to pass the Driver object to the ServerProcess as opposed to passing just the the worker number as I mentioned in comment #22? I mean, we are only passing the Driver object to retrieve the worker number at this time and I am unclear if we should expose the entire interface of Driver to ServerProcess. > Tools/Scripts/webkitpy/port/server_process.py:106 > + def _set_file_nonblocking(self, file): This should be a static method as it does not make use of any instance variables. You can make this a static method be removing the first argument (self) and annotating the method with the function decorator @staticmethod. See <https://docs.python.org/2/library/functions.html#staticmethod> for more details. > Tools/Scripts/webkitpy/port/server_process.py:154 > + self._proc.stdin.flush() Can you elaborate on this change? > Tools/Scripts/webkitpy/port/simulator_process.py:28 > +import select > +import subprocess We are not making use of functionality from these modules. > Tools/Scripts/webkitpy/port/simulator_process.py:31 > +from select import kqueue, kevent We are not making use of kqueue or event in this file. >>> Tools/Scripts/webkitpy/port/simulator_process.py:60 >>> + def poll(self): >>> + try: >>> + os.kill(self.pid, 0) >>> + except OSError, err: >>> + if err.errno == errno.ESRCH: >>> + return 1 >>> + elif err.errno == errno.EPERM: >>> + return None >>> + else: >>> + return -1 >>> + else: >>> + return None >> >> Python's queue isn't the right choice for this for a few reasons. First, it requires setup and teardown, the os.kill approach does not. Second, while the kevent API can accept a pid, Python's kevent wrapper seems to only expect a file handle and will crash if it cannot access the provided argument as a file handle. > > OK. The implementation of this function does not conform to the description of Popen.poll(). Please read my remark in comment #32 for more details. > Tools/Scripts/webkitpy/port/simulator_process.py:71 > + def wait(self, timeout=None): > + ret = self.poll() > + if timeout: > + deadline = time.time() + timeout > + while not ret: > + time.sleep(.01) > + ret = self.poll() > + if deadline and deadline < time.time(): > + raise IOError("Timed out waiting for " + str(self.pid) + " to crash") > + return ret Did you have a chance to read my remark in comment 32? When I wrote the remark in comment 32 the implementation of this function (in attachment 298792 [details]) did not conform to the description of Popen.wait() with respect to setting and returning the instance variable returncode. This iteration of the patch gets us farther away from conformance with the introduction of an optional parameter timeout. Moreover, we do not make use of this optional timeout argument in this patch. > Tools/Scripts/webkitpy/port/simulator_process.py:95 > + def __unlink_check_exists(self, path): > + try: > + os.unlink(path) > + except OSError as e: > + if os.path.exists(path): > + raise OSError("Could not unlink '" + path + "', the target path is a file not a fifo.") You did not address my remark about this function in comment #30. Please address it. If it turns out that it is meaningful to have raise a more human-readable exception for the case where the FIFO exists (why?) then is it possible to determine this case by examining the OSError we caught as opposed to calling os.path.exists()? > Tools/Scripts/webkitpy/port/simulator_process.py:126 > + deadline = time.time() + 3 > + while not self._proc.stdin and deadline > time.time(): > + try: > + self._proc.stdin = open(self._in_path, "w") > + except OSError as e: > + pass > + if not self._proc.stdin: > + self._crashed = True Does this work? I mean, I would have inferred from your remark in comment 34 that using the built-in open() would block when opening a named pipe for writing; => we would hang regardless of the deadline. If open() blocks then we may be able to take inspiration from <https://docs.python.org/2/library/signal.html#example> and use alarm() to interrupt the the blocking open() call. Alternatively we could switch back to using a non-blocking open() and this polling solution (we're not polling for a long time :/) > Tools/Scripts/webkitpy/xcode/simulator.py:268 > + transfered_environment = {} As I asked in comment #30, can we come up with a better name for this variable? One suggestion I gave was the name "environment_to_use". If you have your heart set on the name transferred_environment then please fix the misspelling of "transfered" in its name. > Tools/Scripts/webkitpy/xcode/simulator.py:270 > + for value in env: I would write this as: for value in (env or {}): Then we can remove the "if env" conditional above and remove one level of indentation. Alternatively we could define the default value of the env argument to be {} to have the same effect though this could lead to a bug if this function were ever changed to modify env directly. What is the preferred Python way to deal with a function that copies from an optionally specified dictionary? > Tools/Scripts/webkitpy/xcode/simulator.py:280 > + if not output.startswith(bundle_identifier + ": "): > + return None > + output = output.rstrip('\n').split(' ')[1] > + return int(output) This still seems brittle. Maybe we should even assert that we get a match instead of returning None to catch a change to the output of the "simctl launch" command? Can you please elaborate on your reason not to use a regular expression to do this parsing as I suggested in comment #30? Comment on attachment 299251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299251&action=review >> Tools/Scripts/webkitpy/port/driver.py:371 >> + self._server_process = self._port._test_runner_process_constructor(self._port, self._server_name, self.cmd_line(pixel_tests, per_test_args), environment, driver=self) > > How did you come to the decision to pass the Driver object to the ServerProcess as opposed to passing just the the worker number as I mentioned in comment #22? I mean, we are only passing the Driver object to retrieve the worker number at this time and I am unclear if we should expose the entire interface of Driver to ServerProcess. Originally, I had done this because the driver has more information which I though would be of use. Looking closer at the driver, though, I think that just passing the worker number will suffice. >> Tools/Scripts/webkitpy/port/server_process.py:154 >> + self._proc.stdin.flush() > > Can you elaborate on this change? The ServerProcess does not know the settings of stdin and how it is buffering. Previously, stdin was flushing on a newline. SimulatorProcess does not open stdin such that it flushes on a newline. Rather than mandating that every stdin flush on a newline (which is basically the intended behavior) I decided to have server_process force the flush, because this will work more generally. In future expansions of ServerProcess, this approach will be much less error-prone, since the buffering settings of stdin will not matter. >>>> Tools/Scripts/webkitpy/port/simulator_process.py:60 >>>> + return None >>> >>> Python's queue isn't the right choice for this for a few reasons. First, it requires setup and teardown, the os.kill approach does not. Second, while the kevent API can accept a pid, Python's kevent wrapper seems to only expect a file handle and will crash if it cannot access the provided argument as a file handle. >> >> OK. > > The implementation of this function does not conform to the description of Popen.poll(). Please read my remark in comment #32 for more details. I will add the public return code and set it here. I hadn't realized that returncode was actually a publicly available variable owned by subprocess. >> Tools/Scripts/webkitpy/port/simulator_process.py:71 >> + return ret > > Did you have a chance to read my remark in comment 32? When I wrote the remark in comment 32 the implementation of this function (in attachment 298792 [details]) did not conform to the description of Popen.wait() with respect to setting and returning the instance variable returncode. This iteration of the patch gets us farther away from conformance with the introduction of an optional parameter timeout. Moreover, we do not make use of this optional timeout argument in this patch. The timeout argument is documented in subprocess.wait() (https://docs.python.org/3/library/subprocess.html), which was why I added it. See poll for discussion on the return code. >> Tools/Scripts/webkitpy/port/simulator_process.py:95 >> + raise OSError("Could not unlink '" + path + "', the target path is a file not a fifo.") > > You did not address my remark about this function in comment #30. Please address it. If it turns out that it is meaningful to have raise a more human-readable exception for the case where the FIFO exists (why?) then is it possible to determine this case by examining the OSError we caught as opposed to calling os.path.exists()? Sorry, forgot to address this concern. I did attempt to change this. The problem is, a fifo will not show up with an os.exists(path). So the following code: if os.path.exists(path): os.unlink(path) will not actually unlink the fifo. However, just a pure os.unlink will fail if there is not a fifo at the specified path. If there isn't a fifo at the specified path, their is either nothing (which is what we want) or there is another file at this location, which would be an unrecoverable error. I will look into examining OSError, I'm not sure if it's worth it, though. Any other type of error would cause os.mkfifo to fail (and actually, another-file-at-this-location would cause os.mkfifo to fail as well). If anything, we should just ignore a failure to unlink and assume os.mkfifo will catch any problems. > Tools/Scripts/webkitpy/port/simulator_process.py:116 > + self._pid = self._device.launch_app(self._bundle_id, self._cmd[1:], env=self._env) Should assert that this is not None >> Tools/Scripts/webkitpy/port/simulator_process.py:126 >> + self._crashed = True > > Does this work? I mean, I would have inferred from your remark in comment 34 that using the built-in open() would block when opening a named pipe for writing; => we would hang regardless of the deadline. If open() blocks then we may be able to take inspiration from <https://docs.python.org/2/library/signal.html#example> and use alarm() to interrupt the the blocking open() call. Alternatively we could switch back to using a non-blocking open() and this polling solution (we're not polling for a long time :/) It does work. It also shouldn't be polling for very long. In short, what is going on here is that a fifo can't be opened for writing until someone else has opened it for reading. This is why stdout and stderr need to be opened before the application launches. I didn't think about the fact that the built-in open would block when opening, though. I'm also not totally sure it does block (since in debugging, I had print statements inside the except at one point, and was seeing output) but I will quickly test this. If it does block, then I think the alarm approach is much more straight forward. >> Tools/Scripts/webkitpy/xcode/simulator.py:270 >> + for value in env: > > I would write this as: > > for value in (env or {}): > > Then we can remove the "if env" conditional above and remove one level of indentation. Alternatively we could define the default value of the env argument to be {} to have the same effect though this could lead to a bug if this function were ever changed to modify env directly. What is the preferred Python way to deal with a function that copies from an optionally specified dictionary? I think your for loop suggestion is the best here. If this function stood alone, I would say that defining the default value was the better approach. Unfortunately, everywhere else in our code we pass 'None' for an empty environment, instead of an empty dictionary, so it seem like that is the method we should continue with >> Tools/Scripts/webkitpy/xcode/simulator.py:280 >> + return int(output) > > This still seems brittle. Maybe we should even assert that we get a match instead of returning None to catch a change to the output of the "simctl launch" command? Can you please elaborate on your reason not to use a regular expression to do this parsing as I suggested in comment #30? High level point: I don't think we want to crash if we don't match this output. The problem with crashing if the output isn't matched is that yes, this could be because the output has changed, but, more likely, this is because the launch failed. It should be up to those using this function to assert that the pid they are receiving is not empty (as should be done in SimulatorProcess, I have made a note) With that in mind, this solution does have a bug, since int(output) will throw an error if output isn't actually an int. To your question as to why I used Python's string parsing instead of regular expressions: essentially, I think regular expressions are over-kill here. ' ' and '\n' are not valid characters in a bundle identifier, and the output should be "<bundle id>: <pid>\n", this seems like the kind of limited situation where Python's string parsing functions are ideally suited (also, just looking through Simulator.py, it seems we tend to prefer these kinds of simple Python expressions to regular expressions when given a choice). If this output is changed in the future, SimulatorProcess will begin triggering it's assert, and the error should be pretty obvious. Created attachment 299284 [details]
Patch
Comment on attachment 299284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299284&action=review > Tools/Scripts/webkitpy/port/simulator_process.py:96 > + # Unlinks are needed on reset in the event that the Python code unexpectedly > + # fails between reset and kill. This can be caused by a sigkill or a crash. > + # This ensures that os.mkfifo will not be obstructed by previous fifos. > + # Other files will still cause os.mkfifo to fail. > + try: > + os.unlink(self._in_path) > + except: > + pass > + try: > + os.unlink(self._out_path) > + except: > + pass > + try: > + os.unlink(self._error_path) > + except: > + pass The most notable example of this is launch_app failing. It is fairly straight-forward for a simulator to be deliberately shutdown or crash between when the app is installed and (during __init__) and when it is launched, especially since _start is called any time a DumpRenderTree/WebKitTestRunner crashes and must be restarted. > Tools/Scripts/webkitpy/port/simulator_process.py:111 > + assert self._pid subprocess.Popen will throw an exception if the pipe cannot be created. Similarly, we crash here if no pid is returned from launch_app. > Tools/Scripts/webkitpy/port/simulator_process.py:119 > + self._proc.stdin = open(self._in_path, "w", 0) subprocess.Popen uses no buffer by default. We can set a buffer (the reader reads line by line), this is mimicking server_process's behavior. Comment on attachment 299284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299284&action=review > Tools/Scripts/webkitpy/port/simulator_process.py:82 > + # fails between reset and kill. This can be caused by a sigkill or a crash. This should read: "fails between _start and kill" Comment on attachment 299284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299284&action=review > Tools/Scripts/webkitpy/port/server_process.py:108 > + fl = fcntl.fcntl(file.fileno(), fcntl.F_GETFL) I know that you moved this code. I do not see the need to abbreviate flags as "fl". We should take this opportunity to rename this local variable. > Tools/Scripts/webkitpy/port/simulator_process.py:1 > +# Copyright (C) 2016 Apple Inc. All rights reserved. Nit: 2017 > Tools/Scripts/webkitpy/port/simulator_process.py:45 > + if not self.pid: > + self.returncode = -1 I suggest that we require that SimulatorProcessPopen be instantiated with a pid, stdin, stdout, and stderr handles instead of handling the case of an improperly initialized SimulatorProcessPopen object. Then we can remove this check. > Tools/Scripts/webkitpy/port/simulator_process.py:63 > + from webkitpy.xcode.simulator import Simulator # Avoid circular import Are we still importing here to avoid a circular import? > Tools/Scripts/webkitpy/port/simulator_process.py:74 > + file_location = '/tmp/' + env['IPC_IDENTIFIER'] > + self._in_path = file_location + '_IN' > + self._out_path = file_location + '_OUT' > + self._error_path = file_location + '_ERROR' We should add a comment here to explain that the location of the FIFOs must match the location of the FIFOs in WebKitTestRunner/DumpRenderTree. > Tools/Scripts/webkitpy/xcode/simulator.py:270 > + if not value.startswith("SIMCTL_CHILD_"): " => ' Is it intentional that we do not copy environment variables that are prefixed with SIMCTL_CHILD_? > Tools/Scripts/webkitpy/xcode/simulator.py:272 > + environment_to_use[value] = env[value] Is this intentional? > Tools/Scripts/webkitpy/xcode/simulator.py:275 > + def launch_handler(err): > + raise RuntimeError('Failed to launch app') How did you come to the decision to use a custom error handler that raises a hardcoded exception as opposed to using the default error handler (executive.default_error_handler)? Notice that when error_handler is not specified for executive.run_comment() and return_exit_code := False (default value) then the default error handler, executive.default_error_handler, is used. Comment on attachment 299284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299284&action=review >> Tools/Scripts/webkitpy/port/simulator_process.py:63 >> + from webkitpy.xcode.simulator import Simulator # Avoid circular import > > Are we still importing here to avoid a circular import? No longer needed. Moved this to the top of the file. >> Tools/Scripts/webkitpy/xcode/simulator.py:270 >> + if not value.startswith("SIMCTL_CHILD_"): > > " => ' > > Is it intentional that we do not copy environment variables that are prefixed with SIMCTL_CHILD_? Half-intended. Variables which have SIMCTL_CHILD_ should not have SIMCTL_CHILD_ places in front of them, but they should still be copied. >> Tools/Scripts/webkitpy/xcode/simulator.py:272 >> + environment_to_use[value] = env[value] > > Is this intentional? Partially. 'environment_to_use[value] = env[value]' should not be inside the if statement. Created attachment 299554 [details]
Patch
Comment on attachment 299554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299554&action=review > Tools/Scripts/webkitpy/port/server_process.py:109 > + flag = fcntl.fcntl(file.fileno(), fcntl.F_GETFL) flags? > Tools/Scripts/webkitpy/port/simulator_process.py:66 > + assert self._device.install_app(cmd[0]) Notice that assertions can be disabled at compile time (run Python with option -O). If assertion are disabled then we will never attempt to install the app in the Simulator. On another note <https://wiki.python.org/moin/UsingAssertionsEffectively> suggests that we raise an exception for this failure case, saying "assertions should *not* be used to test for failure cases that can occur because of ... operating system/environment failures." > Tools/Scripts/webkitpy/port/simulator_process.py:82 > + # fails between _start and kill. This can be caused by a sigkill or a crash. Nit: _start => _start() Nit: kill => kill() (These are functions) Nit: sigkill => SIGKILL > Tools/Scripts/webkitpy/port/simulator_process.py:83 > + # This ensures that os.mkfifo will not be obstructed by previous fifos. Nit: os.mkfifo => os.mkfifo() (This is a function) > Tools/Scripts/webkitpy/port/simulator_process.py:84 > + # Other files will still cause os.mkfifo to fail. Ditto. > Tools/Scripts/webkitpy/port/simulator_process.py:107 > + stdout = os.fdopen(os.open(self._out_path, os.O_RDONLY | os.O_NONBLOCK), "rb") > + stderr = os.fdopen(os.open(self._error_path, os.O_RDONLY | os.O_NONBLOCK), "rb") " => ' > Tools/Scripts/webkitpy/port/simulator_process.py:110 > + assert self._pid The wiki page <https://wiki.python.org/moin/UsingAssertionsEffectively> suggest that it is more appropriate to raise an exception for this case. > Tools/Scripts/webkitpy/port/simulator_process.py:113 > + raise IOError("Couldn't open stdin") I suggest that we emphasize in the error description text that we timed out waiting for DumpRenderTree/WebKitTestRunner to open self._in_path. Maybe something like: 'Timed out waiting for process to open {}'.format(self._in_path)? We should assert that signum == signal.SIGALRM. Nit: " => ' > Tools/Scripts/webkitpy/port/simulator_process.py:117 > + stdin = open(self._in_path, "w", 0) " => ' I suggest we add an inline comment to explain that the magic number 0 means we are opening the file with buffering mode unbuffered. > Tools/Scripts/webkitpy/port/simulator_process.py:120 > + signal.alarm(0) I suggest that we add an inline comment "# Cancel alarm" to explain that the magic number 0 is used to cancel an already scheduled alarm. > Tools/Scripts/webkitpy/port/simulator_process.py:124 > + self._proc = SimulatorProcess.SimulatorProcessPopen(self._pid, stdin, stdout, stderr) > + if not stdin: > + self._crashed = True Suppose that WebKitTestRunner/DumpRenderTree to do open the named pipe we are using for stdin. How do we detect this case? What code is responsible for terminating the simulator process? Comment on attachment 299554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299554&action=review > Tools/Scripts/webkitpy/xcode/simulator.py:171 > + def __init__(self, name, udid, available, runtime, host=None): Can we make host non-optional? Also please update the docstring below. > Tools/Scripts/webkitpy/xcode/simulator.py:284 > + assert output.startswith(bundle_identifier + ": ") Assertions can be disabled. Moreover, from reading <https://wiki.python.org/moin/UsingAssertionsEffectively> it it preferred that we raise an exception for such a failure. Comment on attachment 299554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299554&action=review > Tools/Scripts/webkitpy/xcode/simulator.py:281 > + try: > + output = self._host.executive.run_command( > + ['xcrun', 'simctl', 'launch', self.udid, bundle_identifier] + args, > + env=environment_to_use, > + ) > + except ScriptError: > + return None We should let the exception bubble instead of catching it. If we fail to launch the app this is an operating system/environment failure. Created attachment 299619 [details]
Patch
(In reply to comment #46) > Comment on attachment 299554 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299554&action=review > > > Tools/Scripts/webkitpy/xcode/simulator.py:281 > > + try: > > + output = self._host.executive.run_command( > > + ['xcrun', 'simctl', 'launch', self.udid, bundle_identifier] + args, > > + env=environment_to_use, > > + ) > > + except ScriptError: > > + return None > > We should let the exception bubble instead of catching it. If we fail to > launch the app this is an operating system/environment failure. We had discussed this earlier and agreed to throw an exception in the case where parsing of simctl's output fails, but allow the caller to manage the error in the event that an app failed to launch for some other reason. Created attachment 299846 [details]
Patch
Comment on attachment 299846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299846&action=review > Tools/Scripts/webkitpy/port/darwin.py:181 > + plist_path = self._filesystem.join(app_bundle, 'Contents/Info.plist') We should split the second argument into two arguments and let FileSystem.join() concatenate the path with the OS-specific directory delimiter. > Tools/Scripts/webkitpy/port/simulator_process.py:103 > + os.mkfifo(self._in_path, 0600) Cosmetic: I suggest putting an empty line above this line to separate it from the reset logic and to group the mkfifo() calls. > Tools/Scripts/webkitpy/port/simulator_process.py:116 > + raise IOError("Timed out waiting for process to open {}".format(self._in_path)) Python never dies with this exception as we catch it and silently discard it. What is the point of having a nice description? > Tools/Scripts/webkitpy/port/simulator_process.py:126 > + self._proc = SimulatorProcess.SimulatorProcessPopen(self._pid, stdin, stdout, stderr) > + if not stdin: We should differentiate between a crash of the simulator process vs the process not opening stdin vs process did not open stdin before our 3 second timeout. > Tools/Scripts/webkitpy/port/simulator_process.py:127 > + self.kill() # Will set self._proc to none Do we really need to execute everything that self.kill() does? Can we just call _kill() and then self._reset()? > Tools/Scripts/webkitpy/xcode/simulator.py:171 > + def __init__(self, name, udid, available, runtime, host=None): As I asked before (comment #45), can we make this non-optional? > Tools/Scripts/webkitpy/xcode/simulator.py:180 > :param runtime: The iOS Simulator runtime that hosts this device > :type runtime: Runtime As I said in comment #45, please update this docstring with a description of the host argument. > Tools/Scripts/webkitpy/xcode/simulator.py:273 > + environment_to_use[value] = env[value] I suggest that we do not keep non-SIMCTL_CHILD_-prefixed environment variable to make this function easy to use correctly and hard to use incorrectly. > Tools/Scripts/webkitpy/xcode/simulator.py:281 > + try: > + output = self._host.executive.run_command( > + ['xcrun', 'simctl', 'launch', self.udid, bundle_identifier] + args, > + env=environment_to_use, > + ) > + except ScriptError: > + return None I suggest we let the exception bubble so that we do not obscure the reason run_command() failed. > Tools/Scripts/webkitpy/xcode/simulator.py:285 > + raise RuntimeError("simctl launch output did not match the expected output, although the command succeeded.") I suggest that we include |output| in the exception description to help with debugging/diagnosing a failure. Created attachment 299874 [details]
Patch
Comment on attachment 299874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299874&action=review > Tools/ChangeLog:3 > + Removing LayoutTestRelay Can we come up with a more descriptive title? Even if we decide to remove LayoutTestRelay in this patch or as a second patch associated with the same bug we should convey that this part (the one represented by this patch) is teaching Python how to interact with simctl. > Tools/ChangeLog:7 > + Please explain the purpose of this change. > Tools/Makefile:-12 > endif > - ifeq (,$(DO_NOT_BUILD_LAYOUT_TEST_RELAY)) > - MODULES += LayoutTestRelay > - endif This change should be in the patch that removes LayoutTestRelay. This patch is focused on teaching Python how to use simctl. > Tools/Scripts/build-webkit:-206 > - > - if (willUseIOSSimulatorSDK()) { > - (system("perl", "Tools/Scripts/build-layouttestrelay", argumentsForConfiguration()) == 0) or die; > - } Ditto. > Tools/Scripts/webkitpy/port/simulator_process.py:35 > + class SimulatorProcessPopen(object): Minor: I do not see the need to prefix this class with the name of its parent. The class hierarchy will make the relationship clear. I suggest that we name this class Popen. > Tools/Scripts/webkitpy/port/simulator_process.py:53 > + if err.errno == errno.ESRCH: > + self.returncode = 1 > + elif err.errno != errno.EPERM: > + self.returncode = -1 Would it be too draconian to assert err.errno == errno.ESRCH and then unconditional set self.returncode? assert err.errno == errno.ESRCH self.returncode = 1 return self.returncode > Tools/Scripts/webkitpy/port/simulator_process.py:57 > + while not self.poll(): Very minor: As a slight optimization we could return early, and avoid a function call, if self.returncode is non None. Not necessary to do this. > Tools/Scripts/webkitpy/port/simulator_process.py:58 > + time.sleep(.01) Consider adding an inline comment "# seconds" to explain the meaning of 0.01. > Tools/Scripts/webkitpy/port/simulator_process.py:62 > + self._simulator_manager = Simulator(port_obj.host) I would call this self._simulator. > Tools/Scripts/webkitpy/port/simulator_process.py:67 > + raise RuntimeError("Failed to install app {} on device {}".format(cmd[0], self._device.udid)) Nit: " => ' Maybe it would be clearer to write "simulator device" instead of "device". > Tools/Scripts/webkitpy/port/simulator_process.py:68 > + env['IPC_IDENTIFIER'] = self._bundle_id + "-" + self._device.udid Ditto. > Tools/Scripts/webkitpy/port/simulator_process.py:101 > + raise ValueError("%s already running" % self._name) Nit: " => ' I know you copied this code from server_process.py. This patch alternatives between using String.format() and the %-operator. We should pick one style and stick with it. > Tools/Scripts/webkitpy/port/simulator_process.py:106 > + os.mkfifo(self._in_path, 0600) > + os.mkfifo(self._out_path, 0600) > + os.mkfifo(self._error_path, 0600) Are there named constants that we could use to build up a bitmask that is equal to 0600? This would make 0600 less magical. Would it make sense to define a constant local variable and reference it instead of hardcoding the bitmask? If there are no named constants then we should add an comment above this code to explain what 0600 means. > Tools/Scripts/webkitpy/port/simulator_process.py:115 > + raise Exception("Timed out waiting for process to open {}".format(self._in_path)) Ditto. > Tools/Scripts/webkitpy/port/simulator_process.py:117 > + signal.alarm(3) We should add an inline comment "# seconds" to explain that meaning of 3. > Tools/Scripts/webkitpy/port/simulator_process.py:125 > + e = Exception("Process {} crashed before stdin could be attached".format(self._pid)) I suggest we also include the name of the app (I think this is basename(cmd[0])). > Tools/Scripts/webkitpy/port/simulator_process.py:129 > + self._proc = SimulatorProcess.SimulatorProcessPopen(self._pid, stdin, stdout, stderr) > + if self._proc.poll(): > + e = Exception("Process {} crashed before stdin could be attached".format(self._pid)) > + else: > + self._kill() > + self._reset() > + raise e Jonathan pointed out that "raise e" would change the stack trace of the original exception to be the stack trace from the "raise e" line. We can avoid this by using the directive raise without an argument. I suggest we write this code as: # We set self._proc as _reset() and _kill() depend on it. self._proc = SimulatorProcess.SimulatorProcessPopen(self._pid, stdin, stdout, stderr) if self._proc.poll() is not None: self._reset() raise Exception(...) self._kill() self._reset() raise > Tools/Scripts/webkitpy/xcode/simulator.py:274 > + if not value.startswith('SIMCTL_CHILD_'): > + environment_to_use['SIMCTL_CHILD_' + value] = env[value] Minor: I suggest that we define a local constant for "SIMCTL_CHILD_" instead of duplicating this string twice. > Tools/Scripts/webkitpy/xcode/simulator.py:285 > + raise RuntimeError("simctl launch output did not match the expected output: '<bundle id>: <pid>'. Received: '{}' instead".format(output)) Other than to save a some time to perform code archaeology, it is not meaningful to debugging to emit the format "<bundle id>: <pid>" that we expected in the exception. We should emit the app name in the exception message. I also find it unnecessary and or little value to expose the implementation details that we used simctl. I would use a message of the form: "Failed to find process id for {app name}: {output}". Comment on attachment 299874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299874&action=review >> Tools/Scripts/webkitpy/port/simulator_process.py:57 >> + while not self.poll(): > > Very minor: As a slight optimization we could return early, and avoid a function call, if self.returncode is non None. Not necessary to do this. I believe this optimization would end up costing time, actually. Keep in mind that it is more likely that the process will be running when this function is called, or at the very least, this object will not yet have recognized that the process it is observing is no longer running. >> Tools/Scripts/webkitpy/port/simulator_process.py:62 >> + self._simulator_manager = Simulator(port_obj.host) > > I would call this self._simulator. This is an unneeded declaration. There is no reason to keep track of the simulator, we only use it to access the current device. >> Tools/Scripts/webkitpy/xcode/simulator.py:285 >> + raise RuntimeError("simctl launch output did not match the expected output: '<bundle id>: <pid>'. Received: '{}' instead".format(output)) > > Other than to save a some time to perform code archaeology, it is not meaningful to debugging to emit the format "<bundle id>: <pid>" that we expected in the exception. We should emit the app name in the exception message. I also find it unnecessary and or little value to expose the implementation details that we used simctl. I would use a message of the form: "Failed to find process id for {app name}: {output}". Minor note to the changes made here. launch_app() is not actually aware of the app name, only it's bundle identifier. Created attachment 299965 [details]
Python replacement of LayoutTestRelay
Created attachment 299967 [details]
Removal of LayoutTestRelay
Comment on attachment 299967 [details] Removal of LayoutTestRelay Note that without https://bug-165927-attachments.webkit.org/attachment.cgi?id=299965, this patch will fail EWS. Created attachment 299984 [details]
Python replacement of LayoutTestRelay
A few note on simctl and old machines: simctl was introduced in Xcode 6 (So iOS 8). It doesn't seem that simctl's mechanism for starting and stopping the simulators themselves was ever separated from it's ability to start and stop the simulators. Since we already use simctl in the current scripts, all our bots will be able to run this. (In reply to comment #58) > A few note on simctl and old machines: > simctl was introduced in Xcode 6 (So iOS 8). It doesn't seem that simctl's > mechanism for starting and stopping the simulators themselves was ever > separated from it's ability to start and stop the simulators. Huh? > Since we already use simctl in the current scripts, all our bots will be able to run > this. As I mentioned to you in-person today (01/27), the proposed patch makes use of the simctl subcommands "install", "terminate" and "launch" and I am unclear what was the first version of the public iOS SDK that supported these subcommands. Some or all of these subcommands were not available in earlier versions of the iOS SDK and hence LayoutTestRelay was born. These subcommands are available in CoreSimulator-303.7, included in the public iOS 10 SDK (14A5339a). So long as the OpenSource buildbots and EWS machines have this version of the iOS SDK or newer then we can safely land the proposed patch. Otherwise we need to update Xcode/iOS SDK on the bots before landing the patch. I am also assuming that we do not need to support building WebKit for iOS Simulator using the iOS 9 SDK (can you even build it?). I do not believe that all of the aforementioned simctl subcommands are available in the iOS 9 SDK.. Comment on attachment 299984 [details] Python replacement of LayoutTestRelay View in context: https://bugs.webkit.org/attachment.cgi?id=299984&action=review Thank you Jonathan for iterating on this patch. I noticed some very minor stylistics nits. r=me > Tools/ChangeLog:11 > + LayoutTestRelay uses SPI, since recent versions of the iOS SDK allow for installing apps on # Note iOS version > + simulators through simctl, use this functionality instead. You left in "# Note iOS version". > Tools/ChangeLog:29 > + (IOSSimulatorPort._check_relay): Use base class. You seem to alternate between using "Deleted" and "Use base class" to describe the derived class functions that were removed. I prefer that we use "Deleted." or "Deleted. Use base class implementation". Regardless, we should pick one style and stick with it for describing derived class functions we were able to remove. > Tools/ChangeLog:33 > + (IOSSimulatorPort._driver_class): Use base class. Ditto. > Tools/ChangeLog:38 > + (ServerProcess.__init__): Accept worker_number, to match SimulatorProcesssâs constructor. I would not describer this change as adding the argument worker_number to the ServerProcess constructor because the derived class SimulatorProcess constructor takes this argument. The relationship is the opposite. That is, the base class constructor prototype forces the arguments that must be accepted by the constructor of the derived class. So, I would describe this change simply as: "Added argument worker_number. This class does not make use of it. We will make use of this argument in SimulatorProcess to lookup the associated simulator device." > Tools/ChangeLog:41 > + (MockServerProcess.__init__): Accept worker_number, to match SimulatorProcesssâs constructor. By a similar argument, I would describer this change as "Added argument worker_number." > Tools/ChangeLog:44 > + (SimulatorProcess): > + (SimulatorProcess.Popen): Please add the remark "Added." to these entries so that we can grep ChangeLog/search trac.webkit.org for when these functions were added. > Tools/ChangeLog:45 > + (SimulatorProcess.Popen.__init__): Set-up Popen structure with stdin, stdout, stderr and pid. Set-up => Setup Maybe a better word would be "initialize". > Tools/ChangeLog:47 > + (SimulatorProcess.Popen.wait): Wait for target process to close. I would omit the word "target" here so that the language is consistent with the language used for poll() above. > Tools/ChangeLog:57 > + (Simulator._parse_devices): Pass host to device. device => Device (we are talking about the class) > Tools/Scripts/webkitpy/xcode/simulator.py:283 > + match = re.match(r'(?P<bundle>[^:]+): (?P<pid>\d+)\n', output) We could use String.rstrip() as you did app_identifier_from_bundle() to remove the trailing newline from output and then we do not need to match the \n in the regular expression. If you prefer to have the regular expression match end of line then would it make sense to substitute the regular expression metacharacter $ for \n? Comment on attachment 299967 [details]
Removal of LayoutTestRelay
We need to make an Apple Internal change before we can land this.
Created attachment 300076 [details]
Python replacement of LayoutTestRelay
> ...
Here is what I have been able to find.
If any of these commands weren't supported in earlier versions of the SDK, it would be the install command, as that is the one I can't find references to in documentation of the earlier SDKs. Although, I can't seem to find an explicit 'this command was added in version x.x.x.' What I was able to find is that in early versions of Xcode 7, 'simctl install' would hang, which is perhaps the reason for not using simctl install in the recent past. In the change log, I noted iOS 10 as the point where we know this works, since iOS 9 with some version of Xcode 7 would hang.
In any case, all of our bots use iOS 10, so none of this should be an issue.
Created attachment 300113 [details]
Replace LayoutTestRelay
Although the patch is fine to land as-is and is mostly port-specific code, we may want to consider adding unit tests for this code to prevent regressions. This can be done in a follow up bug/patch. Comment on attachment 300113 [details] Replace LayoutTestRelay Clearing flags on attachment: 300113 Committed r211370: <http://trac.webkit.org/changeset/211370> All reviewed patches have been landed. Closing bug. Reopening, as LayoutTestRelay has been replaced but not removed. Created attachment 300129 [details]
Patch
Comment on attachment 300129 [details]
Patch
Remove LayoutTestRelay
Created attachment 301048 [details]
Remove LayoutTestRelay
Comment on attachment 301048 [details] Remove LayoutTestRelay View in context: https://bugs.webkit.org/attachment.cgi?id=301048&action=review Obviously you will need to rebase this patch as it does not apply to trunk according to EWS. Please update or remove the comment(s) in IOSApplication::isDumpRenderTree() (defined in Source/WebCore/platform/RuntimeApplicationChecks.mm [1]) that reference LayoutTestRelay and DumpRenderTree0. Please coordinate landing this change with the corresponding change to the Apple Internal code to avoid breaking the Apple Internal build. [1] <http://trac.webkit.org/browser/trunk/Source/WebCore/platform/RuntimeApplicationChecks.mm?rev=212000#L179> > Tools/ChangeLog:9 > + LayoutTestRelay uses SPI, since recent versions of the iOS SDK allow for installing apps on simulators through simctl, use this functionality instead. It is unwritten convention that we wrap ChangeLog lines to approximately 100 characters. Please break this line across two or more lines. > Tools/ChangeLog:12 > + (archiveBuiltProduct): No more special case for iOS Simulator products. I would describe this change as: Remove archiving of LayoutTestRelay as it is no longer being built. Added a FIXME that we will need to implement similar archiving machinery once we build ImageDiff for Mac when building WebKit for iOS. Currently ImageDiff is built with the iOS SDK. > Tools/BuildSlaveSupport/built-product-archive:168 > + # FIXME: ImageDiff should be built with the host SDK <rdar://problem/30266038> Missing a period at the end of this sentence. I suggest adding a comma after "SDK" to separate the first clause from the the Radar URL. I also suggest that we add a remark to explain that we should take a similar approach for archiving ImageDiff as was done for LayoutTestRelay in <https://trac.webkit.org/changeset/190515>. Once we fix <rdar://problem/30266038> we likely can do a partial revert of this patch (i.e. add the code from <https://trac.webkit.org/changeset/190515> again) and alter it archive ImageDiff instead of LayoutTestRelay and LayoutTestRelay.dSYM. > Tools/BuildSlaveSupport/kill-old-processes:92 > + "Simulator" Please add a comma (,) to the end of this line. The benefit of this approach is that when subsequently appending to this list we will not need to add a comma to the end of this line. (In reply to comment #72) Just a few notes on Dan's comments: > Obviously you will need to rebase this patch as it does not apply to trunk > according to EWS. This patch does apply to trunk, this is another example of https://bugs.webkit.org/show_bug.cgi?id=167906. I will modify the patch so that it can be accepted through commit queue. > Please coordinate landing this change with the corresponding change to the > Apple Internal code to avoid breaking the Apple Internal build. Changes were committed to the Apple Internal code yesterday, February 9th. Committed r212148: <http://trac.webkit.org/changeset/212148> |