RESOLVED FIXED 140585
iOS EWS queue name should be consistent
https://bugs.webkit.org/show_bug.cgi?id=140585
Summary iOS EWS queue name should be consistent
David Kilzer (:ddkilzer)
Reported 2015-01-17 16:43:00 PST
Bug 140476 adds support to create an iOS EWS queue, but it wasn't consistent about using a name of 'ios-device-ews'. This patch corrects that.
Attachments
Patch v1 (5.15 KB, patch)
2015-01-17 16:52 PST, David Kilzer (:ddkilzer)
no flags
Patch v2 (2.81 KB, patch)
2015-01-17 18:53 PST, David Kilzer (:ddkilzer)
no flags
Patch v3 (4.33 KB, patch)
2015-01-18 21:06 PST, David Kilzer (:ddkilzer)
no flags
David Kilzer (:ddkilzer)
Comment 1 2015-01-17 16:52:18 PST
Created attachment 244845 [details] Patch v1
Alexey Proskuryakov
Comment 2 2015-01-17 16:54:22 PST
Comment on attachment 244845 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=244845&action=review > Tools/ChangeLog:8 > + * EWSTools/start-queue-mac.sh: Update for rename of 'ios-ews' to Ugh, this will make the bubble a lot wider. Not a fan.
David Kilzer (:ddkilzer)
Comment 3 2015-01-17 18:38:08 PST
Alexey expressed concern about making the queue name "ios-device-ews", so changing the patch to be consistent the other way so the queue name is "ios-ews".
David Kilzer (:ddkilzer)
Comment 4 2015-01-17 18:53:05 PST
Created attachment 244848 [details] Patch v2
Alexey Proskuryakov
Comment 5 2015-01-17 19:00:12 PST
Comment on attachment 244848 [details] Patch v2 Looks worth trying, hopefully this won't break ios-simulator.
David Kilzer (:ddkilzer)
Comment 6 2015-01-18 11:11:46 PST
(In reply to comment #5) > Comment on attachment 244848 [details] > Patch v2 > > Looks worth trying, hopefully this won't break ios-simulator. Did a previous version of the patch for Bug 140476 break iso-simulator?
Alexey Proskuryakov
Comment 7 2015-01-18 11:54:11 PST
Not sure how (or what) to check. Dan Bates and Simon Fraser would probably be the ones to advise.
David Kilzer (:ddkilzer)
Comment 8 2015-01-18 16:17:35 PST
(In reply to comment #7) > Not sure how (or what) to check. Dan Bates and Simon Fraser would probably > be the ones to advise. I'm pretty sure it's just `run-webkit-tests --ios-simulator` that needs to be tested with this change.
David Kilzer (:ddkilzer)
Comment 9 2015-01-18 18:52:28 PST
(In reply to comment #8) > (In reply to comment #7) > > Not sure how (or what) to check. Dan Bates and Simon Fraser would probably > > be the ones to advise. > > I'm pretty sure it's just `run-webkit-tests --ios-simulator` that needs to > be tested with this change. Well, after applying the patch, this happens: $ ./Tools/Scripts/run-webkit-tests --ios-simulator --debug LayoutTests/fast/url Traceback (most recent call last): File "Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 425, in <module> sys.exit(main(sys.argv[1:], sys.stdout, sys.stderr)) File "Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 73, in main port = host.port_factory.get(options.platform, options) File "Tools/Scripts/webkitpy/port/factory.py", line 119, in get return cls(self._host, port_name, options=options, **kwargs) File "Tools/Scripts/webkitpy/port/ios.py", line 66, in __init__ super(IOSPort, self).__init__(*args, **kwargs) File "Tools/Scripts/webkitpy/port/apple.py", line 88, in __init__ assert port_name in allowed_port_names, "%s is not in %s" % (port_name, allowed_port_names) AssertionError: ios-simulator is not in ['ios-7', 'ios-8', 'ios-future'] (Without the patch applied, everything works as expected.) Need to fix that issue first.
David Kilzer (:ddkilzer)
Comment 10 2015-01-18 20:53:37 PST
(In reply to comment #9) > Well, after applying the patch, this happens: > > $ ./Tools/Scripts/run-webkit-tests --ios-simulator --debug > LayoutTests/fast/url > Traceback (most recent call last): > File "Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 425, > in <module> > sys.exit(main(sys.argv[1:], sys.stdout, sys.stderr)) > File "Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 73, > in main > port = host.port_factory.get(options.platform, options) > File "Tools/Scripts/webkitpy/port/factory.py", line 119, in get > return cls(self._host, port_name, options=options, **kwargs) > File "Tools/Scripts/webkitpy/port/ios.py", line 66, in __init__ > super(IOSPort, self).__init__(*args, **kwargs) > File "Tools/Scripts/webkitpy/port/apple.py", line 88, in __init__ > assert port_name in allowed_port_names, "%s is not in %s" % (port_name, > allowed_port_names) > AssertionError: ios-simulator is not in ['ios-7', 'ios-8', 'ios-future'] > > (Without the patch applied, everything works as expected.) > > Need to fix that issue first. Sigh. In webkitpy/port/factory.py, PortFactory.get() does prefix matching on the PORT_CLASSES list. Because "IOSPort" appears in the list before "IOSSimulatorPort", the IOSPort.port_name ("ios") is a prefix of "ios-simulator" (the port_name derived from the --ios-simulator command line switch), so IOSPort is chosen instead of IOSSimulatorPort. The reason using "ios-device" for the port name worked before is that "ios-device" is not a prefix of "ios-simulator", so IOSPort was not used (and IOSSimulatorPort was chosen instead). The easiest solution is to make sure PortFactory.PORT_CLASSES always lists "more specific" ports first when port_name values have a common prefix.
David Kilzer (:ddkilzer)
Comment 11 2015-01-18 21:06:22 PST
Created attachment 244876 [details] Patch v3
Daniel Bates
Comment 12 2015-01-18 21:28:05 PST
Comment on attachment 244876 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=244876&action=review > Tools/ChangeLog:22 > + (PortFactory): Re-order PORT_CLASSES so ios.IOSSimulatorPort > + appears before ios.IOSPort. If this is not done, > + `run-webkit-tests --ios-simulator` will get the wrong Port class > + (IOSPort instead of IOSSimulatorPort) due to the way that > + PortFactory.get() uses prefix matching of <Port>.port_name to > + find the correct class to use. The approach taken in this patch to re-order the array values in PortFactory.PORT_CLASSES is OK. We may want to investigate using strict equality instead of prefix matching in PortFactory.get(). > Tools/Scripts/webkitpy/port/ios.py:49 > + VERSION_FALLBACK_ORDER = ['ios-7', 'ios-8'] I take it we plan to build for iOS 7?
David Kilzer (:ddkilzer)
Comment 13 2015-01-18 22:31:46 PST
(In reply to comment #12) > Comment on attachment 244876 [details] > Patch v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244876&action=review > > > Tools/ChangeLog:22 > > + (PortFactory): Re-order PORT_CLASSES so ios.IOSSimulatorPort > > + appears before ios.IOSPort. If this is not done, > > + `run-webkit-tests --ios-simulator` will get the wrong Port class > > + (IOSPort instead of IOSSimulatorPort) due to the way that > > + PortFactory.get() uses prefix matching of <Port>.port_name to > > + find the correct class to use. > > The approach taken in this patch to re-order the array values in > PortFactory.PORT_CLASSES is OK. We may want to investigate using strict > equality instead of prefix matching in PortFactory.get(). Yes, that would be the preferred, long-term fix. > > Tools/Scripts/webkitpy/port/ios.py:49 > > + VERSION_FALLBACK_ORDER = ['ios-7', 'ios-8'] > > I take it we plan to build for iOS 7? No, adding 'ios-7' was a workaround to make test-webkitpy pass on systems with 10.8 and 10.9 SDKs installed. See comments in Bug 140476 Comment #16 for more details.
WebKit Commit Bot
Comment 14 2015-01-18 23:15:36 PST
Comment on attachment 244876 [details] Patch v3 Clearing flags on attachment: 244876 Committed r178641: <http://trac.webkit.org/changeset/178641>
WebKit Commit Bot
Comment 15 2015-01-18 23:15:42 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.