Bug 54248 – new-run-webkit-tests: clean up version handling for the --platform argument

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: clean up version handling for the --platform argument

[Dirk Pranke]

Created attachment 82069 [details]
Patch

[Dirk Pranke]

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.

[Adam Roben (:aroben)]

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 "-"?

[Mihai Parparita]

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.

[Dirk Pranke]

Created attachment 82227 [details]
update w/ feedback from aroben, mihaip - remove the leading dash from port.version()

[Dirk Pranke]

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

[Mihai Parparita]

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?

[Dirk Pranke]

Committed r78522: <http://trac.webkit.org/changeset/78522>