Bug 76475 - investigate use of 'mac' and 'win' as fully-specified port names for the apple ports
Summary: investigate use of 'mac' and 'win' as fully-specified port names for the appl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 75037
  Show dependency treegraph
 
Reported: 2012-01-17 12:39 PST by Dirk Pranke
Modified: 2012-01-25 17:29 PST (History)
6 users (show)

See Also:


Attachments
Patch (11.67 KB, patch)
2012-01-23 19:15 PST, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-01-17 12:39:46 PST
With all the cleanup of creating Port objects in webkitpy, I have tried to distinguish between 'port name as short hand for the version of this implementation appropriate for the platform I'm running on' and 'a fully-specified port name', for example, 'chromium-mac' vs. 'chromium-mac-leopard'. Most places in the code outside of NRWT should only every use the latter kind of port names.

However, it looks like there are places in the code that rely on being able to say 'mac' and 'win', and it is now ambiguous what they is supposed to refer to. I am filing this bug to track the tests that fail and ensure that 'mac' and 'win' are not considered fully specified names anywhere any longer.
Comment 1 Eric Seidel (no email) 2012-01-17 12:48:01 PST
Where would these aliases be needed?  Can we just use the Port objects directly?

Port objects seem to be moving away from having any real logic in them and just being (relatively lightweight) configuration objects, right?
Comment 2 Dirk Pranke 2012-01-17 12:52:49 PST
(In reply to comment #1)
> Where would these aliases be needed?  Can we just use the Port objects directly?
> 

On the command line of NRWT, so, no :) (--platform chromium is functionally equivalent to --chromium, and in fact the latter is implemented in terms of the former).

> Port objects seem to be moving away from having any real logic in them and just being (relatively lightweight) configuration objects, right?

Sort of? There's a fair amount of heavy lifting in the base Port class, and some of those things do get overridden (e.g., diff_image in chromium.py to deal with the fact that Chromium's ImageDiff is quite different from the one the other ports use). 

My hope is to eventually remove most of the chromium'isms, though.
Comment 3 Eric Seidel (no email) 2012-01-17 12:54:49 PST
I don't think anyone ever uses --platform=mac or --platform=win, since those are the default behavior on those operating systems anyway.
Comment 4 Eric Seidel (no email) 2012-01-17 12:58:19 PST
(Btw, I'm not arguing against any proposed change, just letting you know that as I understand this bug, it's likely no change is necessary.  I'm happy to drop support for the command line flags --platform=mac/--platform=win entirely, since I doubt they're used at all.)
Comment 5 Dirk Pranke 2012-01-17 13:01:20 PST
(In reply to comment #4)
> (Btw, I'm not arguing against any proposed change, just letting you know that as I understand this bug, it's likely no change is necessary.  I'm happy to drop support for the command line flags --platform=mac/--platform=win entirely, since I doubt they're used at all.)

Oh, I agree, in the mac/win case. But there are tests that fail if I make 'mac' and 'win' illegal, so those tests need to be fixed: that's what this bug is for. 

That, and I need to confirm w/ aroben or somebody that 'mac' and 'win' are not considered legal fully-specified port names, period.
Comment 6 Dirk Pranke 2012-01-23 19:15:11 PST
Created attachment 123689 [details]
Patch
Comment 7 Dirk Pranke 2012-01-23 19:20:59 PST
Comment on attachment 123689 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/apple.py:55
> +            assert port_name == host.platform.os_name

Note the new assertions here and on line 58.

> Tools/Scripts/webkitpy/layout_tests/port/apple.py:73
> +        port_name = port_name.replace('-wk2', '')

This logic is a bit goofy and the assertion error on line 81 is actually wrong (since it doesn't list the -wk2 variants. At some point it might just be easier and clearer to keep static lists of all of the port names and just add to them when we add new variants.

> Tools/Scripts/webkitpy/layout_tests/port/apple.py:77
> +    # FIXME: A more sophisticated version of this function should move to WebKitPort and replace all calls to name().

I'm not actually sure how such a thing would work, or if it would be worth it.

> Tools/Scripts/webkitpy/layout_tests/port/builders.py:69
> +    "Windows 7 Release (WebKit2 Tests)": {"port_name": "win-future-wk2", "specifiers": set(["wk2"])},

These lines had to change since 'mac-wk2' and 'win-wk2' don't actually tell you which version things are (they're not fully-specified).

> Tools/Scripts/webkitpy/layout_tests/port/factory_unittest.py:70
> +        self.assert_port(port_name='win', os_name='win', os_version='xp', cls=win.WinPort)

These lines had to change since it's no longer legal to say 'mac' on a non-mac host.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:55
> +        fallback_names = list(self.VERSION_FALLBACK_ORDER[fallback_index:])

Here and below, note that old versions will no longer be siliently allowed (we now also have tests for this).
Comment 8 Eric Seidel (no email) 2012-01-25 15:30:36 PST
Comment on attachment 123689 [details]
Patch

This feels a little hacky, but I think it's moving us in the right direction.  Thank you.  And thnak you again for bringing me up to speed on IRC.
Comment 9 Dirk Pranke 2012-01-25 17:29:26 PST
Committed r105951: <http://trac.webkit.org/changeset/105951>