Bug 54248 - new-run-webkit-tests: clean up version handling for the --platform argument
Summary: new-run-webkit-tests: clean up version handling for the --platform argument
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 54127
  Show dependency treegraph
 
Reported: 2011-02-10 14:51 PST by Dirk Pranke
Modified: 2011-02-14 16:03 PST (History)
7 users (show)

See Also:


Attachments
Patch (33.54 KB, patch)
2011-02-10 16:30 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ feedback from aroben, mihaip - remove the leading dash from port.version() (38.28 KB, patch)
2011-02-11 20:04 PST, Dirk Pranke
mihaip: 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 2011-02-10 14:51:21 PST
new-run-webkit-tests: clean up version handling for the --platform argument
Comment 1 Dirk Pranke 2011-02-10 16:30:36 PST
Created attachment 82069 [details]
Patch
Comment 2 Dirk Pranke 2011-02-10 16:41:28 PST
In the course of testing all of the GPU expectation merges last week (and earlier when writing rebaselining unit tests), I realized that the way we're currently running with --platform arguments and checking system versions for baselines is almost completely busted. Namely, '--platform chromium-win' would tell part of the code to run as if there were no version-specific baselines, and the other part of the code *would* react correctly. This caused things to get out of sync, with the result that I don't think any of our version-specific baselines actually would've worked right on the bots.

Fortunately (?) we never look at any versions other than the main ones :(

This patch cleans all of that mess up. It should make more for consistent and predictable behavior. However, It probably merits a good staring at by all the cc'ed parties to make sure it's clear what I'm doing and how things should work going forward.
Comment 3 Adam Roben (:aroben) 2011-02-11 06:45:26 PST
Comment on attachment 82069 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py:52
> +    version_strings = {
> +        (6, 1): '-7',
> +        (6, 0): '-vista',
> +        (5, 1): '-xp',
> +        (5, 0): '-xp',

5.0 is Windows 2000, so "-xp" seems strange. Do you need to include 5.2 (XP64, Server 2003) in here, too?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:42
> +def os_version(os_version_string=None, supported_versions=None):
> +    # FIXME: It's strange that this string is -version, not just version.

Maybe the calling code should be prepending "-"?
Comment 4 Mihai Parparita 2011-02-11 11:08:47 PST
Comment on attachment 82069 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:57
> +            self._version = port_name[port_name.index('-mac') + 4:]

Should this assert that the resulting version is in SUPPORTED_OS_VERSIONS?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:61
> +        fallback_paths = {

Can this be moved to be a class-level attribute like SUPPORTED_OS_VERSIONS?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:-91
> -        # FIXME: It's strange that this string is -version, not just version.

I agree with Adam, if we're cleaning this up anyway, can we switch the convention for version to drop the leading dash?

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py:52
>> +        (5, 0): '-xp',
> 
> 5.0 is Windows 2000, so "-xp" seems strange. Do you need to include 5.2 (XP64, Server 2003) in here, too?

Agreed. We don't have any Win2K bots, so that version should be an error, the same way that Tiger is for chromium-mac.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py:69
> +            self._version = port_name[port_name.index('-win') + 4:]

Ditto about asserting here.
Comment 5 Dirk Pranke 2011-02-11 20:04:04 PST
Created attachment 82227 [details]
update w/ feedback from aroben, mihaip - remove the leading dash from port.version()
Comment 6 Dirk Pranke 2011-02-11 20:09:30 PST
(In reply to comment #3)
> (From update of attachment 82069 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82069&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py:52
> > +    version_strings = {
> > +        (6, 1): '-7',
> > +        (6, 0): '-vista',
> > +        (5, 1): '-xp',
> > +        (5, 0): '-xp',
> 
> 5.0 is Windows 2000, so "-xp" seems strange. Do you need to include 5.2 (XP64, Server 2003) in here, too?
> 

Chromium has never directly supported 5.2, so unless you want to special-case it, no.

> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:42
> > +def os_version(os_version_string=None, supported_versions=None):
> > +    # FIXME: It's strange that this string is -version, not just version.
> 
> Maybe the calling code should be prepending "-"?

Fixed.


(In reply to comment #4)
> (From update of attachment 82069 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82069&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:57
> > +            self._version = port_name[port_name.index('-mac') + 4:]
> 
> Should this assert that the resulting version is in SUPPORTED_OS_VERSIONS?
>

Done.
 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:61
> > +        fallback_paths = {
> 
> Can this be moved to be a class-level attribute like SUPPORTED_OS_VERSIONS?
> 

Done.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py:69
> > +            self._version = port_name[port_name.index('-win') + 4:]
> 
> Ditto about asserting here.

Done.

Also, I should note that there is a somewhat unfortunate side effect of this change that, in order to make things be consistent with port.version() and the version modifiers in the test_expectations file, I've renamed the chromium win 7 port from "chromium-win-7" to "chromium-win-win7". People don't specify the full version by hand that often, so I don't think this'll be a big deal.
Comment 7 Mihai Parparita 2011-02-14 10:54:35 PST
Comment on attachment 82227 [details]
update w/ feedback from aroben, mihaip - remove the leading dash from port.version()

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:495
>                      # TODO(victorw): Remove WIN-VISTA and WIN-7 once we have

Update this comment to reference WIN-WIN7?
Comment 8 Dirk Pranke 2011-02-14 16:03:18 PST
Committed r78522: <http://trac.webkit.org/changeset/78522>