Bug 140585 – iOS EWS queue name should be consistent

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 244845 [details]
Patch v1

[Alexey Proskuryakov]

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)]

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)]

Created attachment 244848 [details]
Patch v2

[Alexey Proskuryakov]

Comment on attachment 244848 [details]
Patch v2

Looks worth trying, hopefully this won't break ios-simulator.

[David Kilzer (:ddkilzer)]

(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]

Not sure how (or what) to check. Dan Bates and Simon Fraser would probably be the ones to advise.

[David Kilzer (:ddkilzer)]

(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)]

(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)]

(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)]

Created attachment 244876 [details]
Patch v3

[Daniel Bates]

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)]

(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 on attachment 244876 [details]
Patch v3

Clearing flags on attachment: 244876

Committed r178641: <http://trac.webkit.org/changeset/178641>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.