Bug 169083 - Standardize device/simulator naming conventions
Summary: Standardize device/simulator naming conventions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks: 169302
  Show dependency treegraph
 
Reported: 2017-03-02 10:06 PST by Jonathan Bedard
Modified: 2017-03-09 11:46 PST (History)
7 users (show)

See Also:


Attachments
Patch (8.69 KB, patch)
2017-03-02 10:07 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.17 KB, patch)
2017-03-02 10:50 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.46 KB, patch)
2017-03-02 10:56 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.21 KB, patch)
2017-03-07 11:08 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.25 KB, patch)
2017-03-07 11:13 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2017-03-07 13:35 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.15 KB, patch)
2017-03-07 13:57 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.17 KB, patch)
2017-03-08 15:59 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-03-02 10:06:13 PST
Currently, we use a different naming convention for device and simulator when command line flags are passed.  These ports should be using the same naming convention.  For example, if --ios-simulator is a valid command line flag, so should --ios-device.  If --device is a valid command line flag, so should --simulator.  These command line flags and port names should follow a more predictable pattern.
Comment 1 Jonathan Bedard 2017-03-02 10:07:47 PST
Created attachment 303215 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2017-03-02 10:15:18 PST
<rdar://problem/30810466>
Comment 3 Srinivasan Vijayaraghavan 2017-03-02 10:44:21 PST
Looks good to me.
Comment 4 Srinivasan Vijayaraghavan 2017-03-02 10:47:39 PST
You'll also need to make changes to the QueueStatusServer since the name of the ews queue will now be ios-device-ews.

Also, it would be great more people on the operations team had reviewer privileges.
Comment 5 Jonathan Bedard 2017-03-02 10:50:43 PST
Created attachment 303218 [details]
Patch
Comment 6 Jonathan Bedard 2017-03-02 10:56:59 PST
Created attachment 303220 [details]
Patch
Comment 7 Dean Johnson 2017-03-02 13:04:42 PST
Comment on attachment 303220 [details]
Patch

LGTM
Comment 8 Alexey Proskuryakov 2017-03-07 09:12:57 PST
Comment on attachment 303220 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303220&action=review

> Tools/QueueStatusServer/config/queues.py:37
> -    "ios-ews",
> +    "ios-device-ews",

Not sure if we need this change. The queue name doesn't document what the queue does very well anyway (it's iOS Device armv7s build only), and the shorter it is, the less space it takes in Bugzilla UI.

> Tools/Scripts/build-webkit:96
> +  --ios-device  or --device         Use "iphoneos.internal" SDK if installed, else "iphoneos" SDK (iOS only)
> +  --ios-simulator or --simulator    Use the current iphonesimulator SDK (iOS only)

I think that it may be slightly nicer to put each option on its own line, so that it's easier to point out which ones are deprecated.
Comment 9 Jonathan Bedard 2017-03-07 11:08:43 PST
Created attachment 303681 [details]
Patch
Comment 10 Jonathan Bedard 2017-03-07 11:13:10 PST
Created attachment 303682 [details]
Patch
Comment 11 Jonathan Bedard 2017-03-07 13:35:18 PST
Created attachment 303715 [details]
Patch
Comment 12 Jonathan Bedard 2017-03-07 13:57:52 PST
Created attachment 303721 [details]
Patch
Comment 13 WebKit Commit Bot 2017-03-07 15:06:01 PST
Comment on attachment 303721 [details]
Patch

Clearing flags on attachment: 303721

Committed r213545: <http://trac.webkit.org/changeset/213545>
Comment 14 WebKit Commit Bot 2017-03-07 15:06:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Jonathan Bedard 2017-03-08 09:50:39 PST
Reverted r213545 for reason:

iOS EWS broken by this change

Committed r213577: <http://trac.webkit.org/changeset/213577>
Comment 16 Jonathan Bedard 2017-03-08 15:59:04 PST
Created attachment 303857 [details]
Patch
Comment 17 Alexey Proskuryakov 2017-03-09 10:21:34 PST
Comment on attachment 303857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303857&action=review

> Tools/Scripts/webkitpy/common/config/ews.json:16
> +        "name": "ios-ews",

So this is the additional fix. That's surprising to me - I thought that the name was only used for display purposes.

r=me if this fixes the problem, but I'd like to know more about why it does.
Comment 18 Jonathan Bedard 2017-03-09 10:23:51 PST
Comment on attachment 303857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303857&action=review

>> Tools/Scripts/webkitpy/common/config/ews.json:16
>> +        "name": "ios-ews",
> 
> So this is the additional fix. That's surprising to me - I thought that the name was only used for display purposes.
> 
> r=me if this fixes the problem, but I'd like to know more about why it does.

We actually pull the commands to run EWS from this JSON file, as I mention in https://bugs.webkit.org/show_bug.cgi?id=169385.
Comment 19 Alexey Proskuryakov 2017-03-09 11:25:33 PST
> We actually pull the commands to run EWS from this JSON file

This is correct. What surprises me is that we use the name, as most EWSes don't even have in specified in ews.json.
Comment 20 WebKit Commit Bot 2017-03-09 11:46:23 PST
Comment on attachment 303857 [details]
Patch

Clearing flags on attachment: 303857

Committed r213654: <http://trac.webkit.org/changeset/213654>
Comment 21 WebKit Commit Bot 2017-03-09 11:46:29 PST
All reviewed patches have been landed.  Closing bug.