Create iOS-EWS client queue.
Created attachment 244658 [details] Potential patch (needs testing!)
It's aliiiive! https://webkit-queues.appspot.com/queue-status/ios-ews
Comment on attachment 244658 [details] Potential patch (needs testing!) View in context: https://bugs.webkit.org/attachment.cgi?id=244658&action=review > Tools/EWSTools/start-queue-mac.sh:47 > + (cd $WEBKIT_HOME; sudo ./Tools/Scripts/configure-xcode-for-ios-development) Does this need to be done on every start, or is once per machine enough? Sudoing a script from a script makes me nervous for some reason. > Tools/Scripts/webkitpy/common/config/ports.py:147 > + def run_javascriptcore_tests_command(self): > + return None > + > + def run_webkit_unit_tests_command(self): > + return None > + > + def run_webkit_tests_command(self): > + return None > + > + def run_python_unittests_command(self): > + return None > + > + def run_perl_unittests_command(self): > + return None > + > + def run_bindings_tests_command(self): > + return None I don't know if we need any of this. Shouldn't we just configure EWS in such a way that it doesn't attempt to run tests? I don't know how it's done, but Windows, Gtk and Efl EWS bots all avoid running tests. > Tools/Scripts/webkitpy/port/apple.py:59 > + if port_name == 'ios': > + return port_name This explicitly prevents ios-wk2 from being produced, why? Also, iOS full port name will not include a version, making integration with TestExpectations harder. > Tools/Scripts/webkitpy/port/apple.py:87 > - allowed_port_names = self.VERSION_FALLBACK_ORDER + [self.operating_system() + "-future"] > + allowed_port_names = self.VERSION_FALLBACK_ORDER + [self.operating_system() + "-future"] + ['ios'] Hmm, this is overridden in mac.py, should ios.py just do the same as mac.py? It doesn't seem right to make apple.py know about iOS, but not about any OS X versions. > Tools/Scripts/webkitpy/port/ios.py:58 > + self._leak_detector = LeakDetector(self) This will be dead code, probably best to not land it. > Tools/Scripts/webkitpy/port/ios.py:64 > + mac_config = port_config.Config(self._executive, self._filesystem, 'mac') > + self._mac_build_directory = mac_config.build_directory(self.get_option('configuration')) Hmm. > Tools/Scripts/webkitpy/port/ios.py:73 > + if self.get_option('driver_name'): > + return self.get_option('driver_name') > + if self.get_option('webkit_test_runner'): > + return 'WebKitTestRunnerApp.app' > + return 'DumpRenderTree.app' More dead code. Almost everything down below is about running tests.
> Shouldn't we just configure EWS in such a way that it doesn't attempt to run tests? I think that you already have that taken care of, through ews.json.
(In reply to comment #3) > Comment on attachment 244658 [details] > Potential patch (needs testing!) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244658&action=review > > > Tools/EWSTools/start-queue-mac.sh:47 > > + (cd $WEBKIT_HOME; sudo ./Tools/Scripts/configure-xcode-for-ios-development) > > Does this need to be done on every start, or is once per machine enough? > Sudoing a script from a script makes me nervous for some reason. It should only need to be run once, but running it multiple times shouldn't be a problem. I had a discussion with Lucas regarding whether this was better to just run once on the machine, and then clone the machine's drive with that already taken care of. His stance was that we should just run it every time because it's one less configuration piece that needs to have occurred in order for the script to function. Personally, I'm not strongly opinionated in either direction. > > > Tools/Scripts/webkitpy/common/config/ports.py:147 > > + def run_javascriptcore_tests_command(self): > > + return None > > + > > + def run_webkit_unit_tests_command(self): > > + return None > > + > > + def run_webkit_tests_command(self): > > + return None > > + > > + def run_python_unittests_command(self): > > + return None > > + > > + def run_perl_unittests_command(self): > > + return None > > + > > + def run_bindings_tests_command(self): > > + return None > > I don't know if we need any of this. Shouldn't we just configure EWS in such > a way that it doesn't attempt to run tests? I don't know how it's done, but > Windows, Gtk and Efl EWS bots all avoid running tests. I'll start pruning things until something breaks to boil down this patch. > > > Tools/Scripts/webkitpy/port/apple.py:59 > > + if port_name == 'ios': > > + return port_name > > This explicitly prevents ios-wk2 from being produced, why? Also, iOS full > port name will not include a version, making integration with > TestExpectations harder. I was just trying to circumvent this assert (which will fail, because "mac" is not in "ios" ) assert host.platform.os_name in port_name, "%s is not in %s!" % (host.platform.os_name, port_name) I'll think harder about what the correct thing to do is and post another patch. > > > Tools/Scripts/webkitpy/port/apple.py:87 > > - allowed_port_names = self.VERSION_FALLBACK_ORDER + [self.operating_system() + "-future"] > > + allowed_port_names = self.VERSION_FALLBACK_ORDER + [self.operating_system() + "-future"] + ['ios'] > > Hmm, this is overridden in mac.py, should ios.py just do the same as mac.py? > It doesn't seem right to make apple.py know about iOS, but not about any OS > X versions. Good point! > > > Tools/Scripts/webkitpy/port/ios.py:58 > > + self._leak_detector = LeakDetector(self) > > This will be dead code, probably best to not land it. > > > Tools/Scripts/webkitpy/port/ios.py:64 > > + mac_config = port_config.Config(self._executive, self._filesystem, 'mac') > > + self._mac_build_directory = mac_config.build_directory(self.get_option('configuration')) > > Hmm. > > > Tools/Scripts/webkitpy/port/ios.py:73 > > + if self.get_option('driver_name'): > > + return self.get_option('driver_name') > > + if self.get_option('webkit_test_runner'): > > + return 'WebKitTestRunnerApp.app' > > + return 'DumpRenderTree.app' > > More dead code. Almost everything down below is about running tests. Yeah, I'll prune stuff and get back to you.
Comment on attachment 244658 [details] Potential patch (needs testing!) View in context: https://bugs.webkit.org/attachment.cgi?id=244658&action=review > Tools/Scripts/webkitpy/port/ios.py:310 > port_name = "ios-simulator" This is not part of this patch, but I just wanted to mention that this may be confusing a lot of webkitpy code. It probably parses "simulator" as an OS version.
Created attachment 244803 [details] Patch
Comment on attachment 244803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244803&action=review > Tools/EWSTools/start-queue-mac.sh:3 > +# Copyright (c) 2014-2015 Apple Inc. All rights reserved. I think that "2014, 2015" is right when there is nothing in between. > Tools/Scripts/webkitpy/common/config/ports.py:67 > + "ios": IOSPort, I'm worried that ios-simulator will now drop here (there are probably code paths that strip the version before constructing a DeprecatedPort). We used to get MacPort for the simulator, and now we get IOSPort. > Tools/Scripts/webkitpy/common/config/ports.py:127 > + command = super(IOSPort, self).build_webkit_command(build_style=build_style) build-webkit --help says that --device uses current iphoneos.internal SDK. Is that a lie? Or is this dead code? > Tools/Scripts/webkitpy/port/factory.py:87 > 'ios.IOSSimulatorPort', > + 'ios.IOSPort', Alphabetical! > Tools/Scripts/webkitpy/port/ios.py:1 > +# Copyright (C) 2014-2015 Apple Inc. All rights reserved. 2014, 2015 > Tools/Scripts/webkitpy/port/ios.py:55 > + sdk_command_stdout = sdk_command_process.communicate()[0].strip() It would probably be a good idea to plan for iOS 10. > Tools/Scripts/webkitpy/port/ios.py:57 > + assert sdk_command_stdout, "xcode is not installed, and hence we cannot construct an iOS port object!" Xcode (upper case) > Tools/Scripts/webkitpy/port/ios.py:81 > + def _path_to_webcore_library(self): > + return self._build_path('WebCore.framework/WebCore') Why is this needed? > Tools/Scripts/webkitpy/port/ios.py:87 > + def make_command(self): > + return self.xcrun_find('make', '/usr/bin/make') > + > + def nm_command(self): > + return self.xcrun_find('nm') Why are these needed? > Tools/Scripts/webkitpy/port/ios.py:99 > + def developer_dir(self): > + return self._executive.run_command(['xcode-select', '--print-path']).rstrip() It sounds like these things should be inherited from ApplePort, there is nothing iOS specific here.
Created attachment 244828 [details] Patch
Comment on attachment 244828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244828&action=review > Tools/Scripts/webkitpy/port/ios.py:73 > + #Despite its name, these flags do not actually get passed all the way down to webkit-build. Travelers beware. Some grammar here (its/these). Please add a space in the beginning, and remove useless "travelers beware". > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:111 > + self.run_webkit_patch(command + [self._deprecated_port.flag()] + ['--architecture=%s' % self._port.architecture()] if self._port.architecture() else []) Is precedence correct here?
<rdar://problem/19462823>
Comment on attachment 244828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244828&action=review > Tools/Scripts/webkitpy/port/ios.py:54 > + sdk_command_process = subprocess.Popen('xcrun --sdk iphoneos --show-sdk-version', stdout=subprocess.PIPE, shell=True) Added "stderr=None" argument when landing to quiet some console spew running test-webkitpy. >> Tools/Scripts/webkitpy/port/ios.py:73 >> + #Despite its name, these flags do not actually get passed all the way down to webkit-build. Travelers beware. > > Some grammar here (its/these). Please add a space in the beginning, and remove useless "travelers beware". Fixed when landed. > Tools/Scripts/webkitpy/port/ios.py:75 > + return ['--sdk', 'iphoneos', 'ARCHS=%s' % self._architecture] Changed this to check self._architecture when landed: return ['--sdk', 'iphoneos'] + (['ARCHS=%s' % self._architecture] if self._architecture else []) Looking at the overall patch, this probably isn't needed, but won't hurt anything. >> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:111 >> + self.run_webkit_patch(command + [self._deprecated_port.flag()] + ['--architecture=%s' % self._port.architecture()] if self._port.architecture() else []) > > Is precedence correct here? No, it wasn't. Added parenthesis when landed: (['--architecture=%s' % self._port.architecture()] if self._port.architecture() else [])
Committed r178622: <http://trac.webkit.org/changeset/178622>
(In reply to comment #14) > Committed r178622: <http://trac.webkit.org/changeset/178622> Build fix for non-Mac ports (or Mac ports without an iOS SDK installed): Committed r178623: <http://trac.webkit.org/changeset/178623>
(In reply to comment #15) > (In reply to comment #14) > > Committed r178622: <http://trac.webkit.org/changeset/178622> > > Build fix for non-Mac ports (or Mac ports without an iOS SDK installed): > > Committed r178623: <http://trac.webkit.org/changeset/178623> Fix test-webkitpy on Mountain Lion, Mavericks bots: Committed r178624: <http://trac.webkit.org/changeset/178624> Some GTK bots are still failing, but I don't see what the issue is; it's kind of crazy that we try to create Port objects on every single platform, even if they aren't supported: <https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/5459/steps/webkitpy-test/logs/stdio> Traceback (most recent call last): File "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py", line 103, in test_ewses self._test_ews(ews_class()) File "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py", line 96, in _test_ews self.assert_queue_outputs(ews, expected_logs=self._default_expected_logs(ews), options=options) File "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py", line 65, in _default_expected_logs real_port = Host().port_factory.get(real_port_name) File "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/webkitpy/port/factory.py", line 118, in get port_name = cls.determine_full_port_name(self._host, options, port_name) File "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/webkitpy/port/apple.py", line 65, in determine_full_port_name assert host.platform.os_name in port_name, "%s is not in %s!" % (host.platform.os_name, port_name) AssertionError: linux is not in mac-wk2!
(In reply to comment #16) > Some GTK bots are still failing, but I don't see what the issue is; it's > kind of crazy that we try to create Port objects on every single platform, > even if they aren't supported: > > <https://build.webkit.org/builders/GTK%20Linux%2064- > bit%20Release%20%28Tests%29/builds/5459/steps/webkitpy-test/logs/stdio> > > Traceback (most recent call last): > File > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > webkitpy/tool/commands/earlywarningsystem_unittest.py", line 103, in > test_ewses > self._test_ews(ews_class()) > File > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > webkitpy/tool/commands/earlywarningsystem_unittest.py", line 96, in _test_ews > self.assert_queue_outputs(ews, > expected_logs=self._default_expected_logs(ews), options=options) > File > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > webkitpy/tool/commands/earlywarningsystem_unittest.py", line 65, in > _default_expected_logs > real_port = Host().port_factory.get(real_port_name) > File > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > webkitpy/port/factory.py", line 118, in get > port_name = cls.determine_full_port_name(self._host, options, > port_name) > File > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > webkitpy/port/apple.py", line 65, in determine_full_port_name > assert host.platform.os_name in port_name, "%s is not in %s!" % > (host.platform.os_name, port_name) > AssertionError: linux is not in mac-wk2! The code added to earlywarningsystem_unittest.py to find the architecture for the port assumes that it will get the right answer for the system it's running on. That's not true for the port object created for mac-wk2-ews on a GTK Linux box. Not sure how to fix yet. The right thing to do would be to mock the architecture it would seem.
(In reply to comment #17) > (In reply to comment #16) > > Some GTK bots are still failing, but I don't see what the issue is; it's > > kind of crazy that we try to create Port objects on every single platform, > > even if they aren't supported: > > > > <https://build.webkit.org/builders/GTK%20Linux%2064- > > bit%20Release%20%28Tests%29/builds/5459/steps/webkitpy-test/logs/stdio> > > > > Traceback (most recent call last): > > File > > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > > webkitpy/tool/commands/earlywarningsystem_unittest.py", line 103, in > > test_ewses > > self._test_ews(ews_class()) > > File > > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > > webkitpy/tool/commands/earlywarningsystem_unittest.py", line 96, in _test_ews > > self.assert_queue_outputs(ews, > > expected_logs=self._default_expected_logs(ews), options=options) > > File > > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > > webkitpy/tool/commands/earlywarningsystem_unittest.py", line 65, in > > _default_expected_logs > > real_port = Host().port_factory.get(real_port_name) > > File > > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > > webkitpy/port/factory.py", line 118, in get > > port_name = cls.determine_full_port_name(self._host, options, > > port_name) > > File > > "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/ > > webkitpy/port/apple.py", line 65, in determine_full_port_name > > assert host.platform.os_name in port_name, "%s is not in %s!" % > > (host.platform.os_name, port_name) > > AssertionError: linux is not in mac-wk2! > > The code added to earlywarningsystem_unittest.py to find the architecture > for the port assumes that it will get the right answer for the system it's > running on. That's not true for the port object created for mac-wk2-ews on > a GTK Linux box. > > Not sure how to fix yet. The right thing to do would be to mock the > architecture it would seem. Still teasing apart the code to try to figure out when/where the architecture is set when testing EWS classes.
Comment on attachment 244828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244828&action=review I know this path was reviewed and landed. I noticed some issues. > Tools/Scripts/webkitpy/port/ios.py:55 > + sdk_command_process = subprocess.Popen('xcrun --sdk iphoneos --show-sdk-version', stdout=subprocess.PIPE, shell=True) > + sdk_command_stdout = sdk_command_process.communicate()[0].strip() Is it necessary to execute this using the shell (shell=True)? I take it we do to take advantage of the PATH environment variable to find xcun? Instead I suggest we reference the full path to /usr/bin/xcrun and remove the shell=True option. Also, it's sufficient to strip the newline characters are the end of the output (i.e. rstrip) as opposed to using strip to remove whitespace at the start and end of the string since xcrun --sdk iphoneos --show-sdk-version doesn't emit whitespace at the beginning of its output. Moreover, can we simplify this sdk_command_stdout = subprocess.check_output(...).rstrip()? > Tools/Scripts/webkitpy/port/ios.py:59 > + port_name = port_name + '-' + re.match('^([0-9]+).*', sdk_command_stdout).group(1) It seems unnecessary to match zero or more arbitrary characters after the first sequence of ASCII numbers. > Tools/Scripts/webkitpy/tool/commands/queues.py:32 > +import optparse It doesn't seem correct to import optparse to parse command line options at this layer in the software. I would have expected optparse to be used at a higher layer that is closer to the program entry point. > Tools/Scripts/webkitpy/tool/commands/queues.py:282 > + port_options = optparse.Values() > + if self.architecture: > + setattr(port_options, 'architecture', self.architecture) This doesn't seem like the correct way to get at port options and prevents unit testing the architecture option. Aren't the port options passed to PatchProcessingQueue or its base class?
(In reply to comment #19) > Comment on attachment 244828 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244828&action=review > > I know this path was reviewed and landed. I noticed some issues. > > > Tools/Scripts/webkitpy/port/ios.py:55 > > + sdk_command_process = subprocess.Popen('xcrun --sdk iphoneos --show-sdk-version', stdout=subprocess.PIPE, shell=True) > > + sdk_command_stdout = sdk_command_process.communicate()[0].strip() > > Is it necessary to execute this using the shell (shell=True)? I take it we > do to take advantage of the PATH environment variable to find xcun? Instead > I suggest we reference the full path to /usr/bin/xcrun and remove the > shell=True option. Also, it's sufficient to strip the newline characters are > the end of the output (i.e. rstrip) as opposed to using strip to remove > whitespace at the start and end of the string since xcrun --sdk iphoneos > --show-sdk-version doesn't emit whitespace at the beginning of its output. > > Moreover, can we simplify this sdk_command_stdout = > subprocess.check_output(...).rstrip()? > > > Tools/Scripts/webkitpy/port/ios.py:59 > > + port_name = port_name + '-' + re.match('^([0-9]+).*', sdk_command_stdout).group(1) > > It seems unnecessary to match zero or more arbitrary characters after the > first sequence of ASCII numbers. > > > Tools/Scripts/webkitpy/tool/commands/queues.py:32 > > +import optparse > > It doesn't seem correct to import optparse to parse command line options at > this layer in the software. I would have expected optparse to be used at a > higher layer that is closer to the program entry point. > > > Tools/Scripts/webkitpy/tool/commands/queues.py:282 > > + port_options = optparse.Values() > > + if self.architecture: > > + setattr(port_options, 'architecture', self.architecture) > > This doesn't seem like the correct way to get at port options and prevents > unit testing the architecture option. Aren't the port options passed to > PatchProcessingQueue or its base class? I created another bug and patch for these issues, and also addressed the python unit test problems: https://bugs.webkit.org/show_bug.cgi?id=140637
This patch introduced a python test failure on the Windows bots (and perhaps other non-Mac bots): [1231/1425] webkitpy.tool.commands.earlywarningsystem_unittest.EarlyWarningSystemTest.test_ewses /bin/sh: xcrun: command not found [1232/1425] webkitpy.tool.commands.earlywarningsystem_unittest.EarlyWarningSystemTest.test_ewses failed: Traceback (most recent call last): File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py", line 103, in test_ewses self._test_ews(ews_class()) File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py", line 96, in _test_ews self.assert_queue_outputs(ews, expected_logs=self._default_expected_logs(ews), options=options) File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py", line 65, in _default_expected_logs real_port = Host().port_factory.get(real_port_name) File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/port/factory.py", line 118, in get port_name = cls.determine_full_port_name(self._host, options, port_name) File "/home/buildbot/slave/win-release-tests/build/Tools/Scripts/webkitpy/port/ios.py", line 57, in determine_full_port_name assert sdk_command_stdout, "Xcode is not installed, and hence we cannot construct an iOS port object!" AssertionError: Xcode is not installed, and hence we cannot construct an iOS port object!