Summary: | investigate use of 'mac' and 'win' as fully-specified port names for the apple ports | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||
Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, ap, aroben, eric, ojan, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 75037 | ||||||
Attachments: |
|
Description
Dirk Pranke
2012-01-17 12:39:46 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? (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. I don't think anyone ever uses --platform=mac or --platform=win, since those are the default behavior on those operating systems anyway. (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.) (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. Created attachment 123689 [details]
Patch
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 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.
Committed r105951: <http://trac.webkit.org/changeset/105951> |