Bug 179621 – webkitpy: Better name-version mapping

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Currently, we map version numbers to version names in platforminfo.py.  This has a few problems: first, it confuses version number (10.13.0) with version name (High Sierra) because on some platforms (like iOS) the two are the same (version 11.0 is iOS 11). Second, by storing the version name instead of the version number, scripts can't differentiate between minor versions (So 10.12 and 10.12.3 look exactly the same). Third, as discussed in <https://bugs.webkit.org/show_bug.cgi?id=179534>, we can't iterate through available versions if name-version mapping is in platforminfo.py.

[Radar WebKit Bug Importer]

<rdar://problem/35589585>

[Jonathan Bedard]

Created attachment 327067 [details]
Patch

[Jonathan Bedard]

Created attachment 327068 [details]
Patch

[Jonathan Bedard]

The first part of this patch places all version name-version object mapping in a single location.

The second part of this patch will have platform info representing all versions as version objects and then consumers converting to version name if appropriate.

I am separating these two because the first part of this change is the part that contains the new code, the second part will be a refactor.  The second part of the change is low-risk, while the first pretty significantly changes the logic used for determining version names on some ports.

[Daniel Bates]

Comment on attachment 327068 [details]
Patch

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

I have not thought about the correctness of this patch.

> Tools/Scripts/webkitpy/common/version_name_map.py:44
> +                'Leopard': Version('10.5'),

I suggest that we standardize on instantiating a Version object where the components are passed as numeric types. A string can contain arbitrary characters and requires the Version class to parse the string and have a strategy for handling a bad parse. We can avoid the need to parse strings and avoid the need to pick a bad parse strategy by having Version take numeric types on instantiation.

[Jonathan Bedard]

Comment on attachment 327068 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/version_name_map.py:44
>> +                'Leopard': Version('10.5'),
> 
> I suggest that we standardize on instantiating a Version object where the components are passed as numeric types. A string can contain arbitrary characters and requires the Version class to parse the string and have a strategy for handling a bad parse. We can avoid the need to parse strings and avoid the need to pick a bad parse strategy by having Version take numeric types on instantiation.

This suggesting is probably better discussed here <https://bugs.webkit.org/show_bug.cgi?id=179677>.

Because of Python's type system, we need a strategy for handling malformed data even if we're expecting numeric types.  Furthermore, we can't avoid the need to parse strings because we frequently compute versions from the output of commands.

[David Kilzer (:ddkilzer)]

Comment on attachment 327068 [details]
Patch

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

r=me with a few comments, but Dan Bates expressed a concern, so please sync up with him before landing.

My opinion is that it's an improvement to have all this logic in one place (and tested), and it will make it easy to add new versions in the future.

> Tools/Scripts/webkitpy/common/version.py:103
> +    # 11.2 is contained in 11, but 11 is not contained in 11.2
> +    def contained_in(self, version):
> +        match_flag = False
> +        for i in xrange(len(self)):
> +            if self[i] != version[i]:
> +                match_flag = True
> +            if match_flag and version[i] != 0:
> +                return False
> +        return True

I'm having a hard time with the name 'match_flag'.  I'm sure you wanted to avoid double-negatives, but maybe a name like this might read better?

    last_number_did_not_match
    did_not_match

Or reverse the logic and use 'did_match' instead.

It would also be nice to use something other than "number", but not sure what to call each set of digits between periods.  (Probably not "sub_version", though. :)  Maybe "intermediate_version", although that's really long.

I guess that's probably why you chose 'match_flag'.

Not necessary to fix before landing, but something to consider.

> Tools/Scripts/webkitpy/common/version_name_map.py:77
> +            'linux': {
> +                'Lucid': Version('10.4'),
> +                'Maverick': Version('10.10'),
> +                'Natty': Version('11.4'),
> +                'Precise': Version('12.4'),
> +                'Quantal': Version('12.10'),
> +                'Raring': Version('13.4'),
> +                'Saucy': Version('13.10'),
> +                'Utopic': Version('14.10'),
> +                'Vivid': Version('15.04'),
> +                'Wily': Version('15.10'),
> +                'Yakkety': Version('16.10'),
> +                'Bionic': Version('18.04'),
> +            },

Are these all Ubuntu names?  Haven't heard of them before.

> Tools/Scripts/webkitpy/common/version_name_map.py:112
> +        if ' ' in name:
> +            try:
> +                name = '{}{}'.format(name.split(' ')[0], Version(''.join(name.split(' ')[1:])).major)

I think this will fail for "El Capitan 10.11".

Probably better to split on space, then pop off the last item for Version parsing, then join the remaining ones with spaces for name parsing.

Oh, but maybe that will never be used because we don't use version numbers with macOS names, though?  If so, no changes needed here.

> Tools/Scripts/webkitpy/common/version_name_map_unittest.py:42
> +    def test_mac_version_by_name(self):
> +        map = VersionNameMap()
> +        self.assertEqual(map.from_name('Sierra'), ('mac', Version('10.12')))
> +        self.assertEqual(map.from_name('sierra'), ('mac', Version('10.12')))
> +        self.assertEqual(map.from_name('El Capitan'), ('mac', Version('10.11')))
> +        self.assertEqual(map.from_name('elcapitan'), ('mac', Version('10.11')))

Maybe add unit test:

    self.assertEqual(map.from_name('el Capitan'), ('mac', Version('10.11')))

Maybe reverse arguments here from (actual, expected) to (expected, actual)?

> Tools/Scripts/webkitpy/common/version_name_map_unittest.py:48
> +    def test_ios_version_by_name(self):
> +        map = VersionNameMap()
> +        self.assertEqual(map.from_name('iOS 11'), ('ios', Version(11)))
> +        self.assertEqual(map.from_name('ios11'), ('ios', Version(11)))
> +        self.assertEqual(map.from_name('iOS 11.2'), ('ios', Version(11)))

Should these work?

    self.assertEqual(map.from_name('ios11.2'), ('ios', Version(11)))
    self.assertEqual(map.from_name('iOS11.2'), ('ios', Version(11)))

Maybe reverse arguments here from (actual, expected) to (expected, actual)?

> Tools/Scripts/webkitpy/common/version_unittest.py:131
> +    def test_contained_in(self):
> +        self.assertTrue(Version('11.1').contained_in(Version('11')))
> +        self.assertTrue(Version('11.1.2').contained_in(Version('11.1')))
> +        self.assertFalse(Version('11').contained_in(Version('11.1')))

Maybe add more test cases:

self.assertTrue(Version('11').contained_in(Version('11.1.2')))
self.assertTrue(Version('11.1').contained_in(Version('11.1.2')))
self.assertTrue(Version('11').contained_in(Version('11')))
self.assertTrue(Version('11.1').contained_in(Version('11.1')))
self.assertTrue(Version('11.1.2').contained_in(Version('11.1.2')))
self.assertTrue(Version('11').contained_in(Version('11.0')))
self.assertTrue(Version('11.0').contained_in(Version('11')))
self.assertTrue(Version('11').contained_in(Version('11.0.0')))
self.assertTrue(Version('11.0.0').contained_in(Version('11')))
self.assertTrue(Version('11.1').contained_in(Version('11.1.0')))
self.assertTrue(Version('11.1.0').contained_in(Version('11.1')))

> Tools/Scripts/webkitpy/common/system/platforminfo.py:65
> +        else:
> +            # Linux and iOS both return conforming version strings.
> +            self.os_version = VersionNameMap.map(self).to_name(Version(platform_module.release()), table='public')

Replace the comment with:

        assert self.os_name == 'ios' or self.os_name == 'linux'

[Jonathan Bedard]

Comment on attachment 327068 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/version_name_map.py:77
>> +            },
> 
> Are these all Ubuntu names?  Haven't heard of them before.

Yes.  There was actually a FIXME in PlatformInfo._determine_linux_version about this.

<https://wiki.ubuntu.com/Releases>

Some how I missed Artful, Zesty,  Xenial and Trusty, though.

>> Tools/Scripts/webkitpy/common/version_name_map.py:112
>> +                name = '{}{}'.format(name.split(' ')[0], Version(''.join(name.split(' ')[1:])).major)
> 
> I think this will fail for "El Capitan 10.11".
> 
> Probably better to split on space, then pop off the last item for Version parsing, then join the remaining ones with spaces for name parsing.
> 
> Oh, but maybe that will never be used because we don't use version numbers with macOS names, though?  If so, no changes needed here.

It would fail with 'El Capitan 10.11'.  And we don't use version number with macOS.

That being said, I think your suggestion is probably better.  The current code relies off of a naming convention which has historically been true, but may not remain so in the future.

>> Tools/Scripts/webkitpy/common/version_name_map_unittest.py:48
>> +        self.assertEqual(map.from_name('iOS 11.2'), ('ios', Version(11)))
> 
> Should these work?
> 
>     self.assertEqual(map.from_name('ios11.2'), ('ios', Version(11)))
>     self.assertEqual(map.from_name('iOS11.2'), ('ios', Version(11)))
> 
> Maybe reverse arguments here from (actual, expected) to (expected, actual)?

Those will also work.

I'll add unit tests for them as well.

>> Tools/Scripts/webkitpy/common/system/platforminfo.py:65
>> +            self.os_version = VersionNameMap.map(self).to_name(Version(platform_module.release()), table='public')
> 
> Replace the comment with:
> 
>         assert self.os_name == 'ios' or self.os_name == 'linux'

This code-path is also meant to handle future platforms which conform to this.  I will update the comment to clarify this.

[Jonathan Bedard]

Created attachment 327249 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 327249 [details]
Patch

Clearing flags on attachment: 327249

Committed r225016: <https://trac.webkit.org/changeset/225016>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

Hmmm, this breaks Debian Linux environment, which returns Linux version string like "4.13.0-1-amd64". The last int("0-1-amd64") raises ValueError.

$ uname -a
Linux gochiusa 4.13.0-1-amd64 #1 SMP Debian 4.13.10-1 (2017-10-30) x86_64 GNU/Linux

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #12)
> Hmmm, this breaks Debian Linux environment, which returns Linux version
> string like "4.13.0-1-amd64". The last int("0-1-amd64") raises ValueError.
> 
> $ uname -a
> Linux gochiusa 4.13.0-1-amd64 #1 SMP Debian 4.13.10-1 (2017-10-30) x86_64
> GNU/Linux

This also does not work in Ubuntu 16.04.

$ uname -a
Linux sakura-trick 4.8.0-53-generic #56~16.04.1-Ubuntu SMP Tue May 16 01:18:56 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

webkitpy.common.system.logutils: [DEBUG] Debug logging enabled.
Traceback (most recent call last):
  File "Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/home/yusukesuzuki/dev/WebKit/Tools/Scripts/webkitpy/tool/main.py", line 57, in __init__
    Host.__init__(self)
  File "/home/yusukesuzuki/dev/WebKit/Tools/Scripts/webkitpy/common/host.py", line 48, in __init__
    SystemHost.__init__(self)
  File "/home/yusukesuzuki/dev/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py", line 40, in __init__
    self.user = user.User()
  File "/home/yusukesuzuki/dev/WebKit/Tools/Scripts/webkitpy/common/system/user.py", line 61, in __init__
    self._platforminfo = platforminfo or PlatformInfo(sys, platform, Executive())
  File "/home/yusukesuzuki/dev/WebKit/Tools/Scripts/webkitpy/common/system/platforminfo.py", line 65, in __init__
    self.os_version = VersionNameMap.map(self).to_name(Version(platform_module.release()), table='public')
  File "/home/yusukesuzuki/dev/WebKit/Tools/Scripts/webkitpy/common/version.py", line 39, in __init__
    self[i] = ver.split('.')[i]
  File "/home/yusukesuzuki/dev/WebKit/Tools/Scripts/webkitpy/common/version.py", line 80, in __setitem__
    self.tiny = int(value)
ValueError: invalid literal for int() with base 10: '0-53-generic'

[Yusuke Suzuki]

Reverted r225016 for reason:

Break webkit-patch on Ubuntu and Debian Linux

Committed r225033: <https://trac.webkit.org/changeset/225033>

[Yusuke Suzuki]

Comment on attachment 327249 [details]
Patch

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

> Tools/Scripts/webkitpy/common/version_name_map.py:83
> +            'linux': {
> +                'Lucid': Version('10.4'),
> +                'Maverick': Version('10.10'),
> +                'Natty': Version('11.4'),
> +                'Precise': Version('12.4'),
> +                'Quantal': Version('12.10'),
> +                'Raring': Version('13.4'),
> +                'Saucy': Version('13.10'),
> +                'Trusty': Version('14.4'),
> +                'Utopic': Version('14.10'),
> +                'Vivid': Version('15.04'),
> +                'Wily': Version('15.10'),
> +                'Xenial': Version('16.4'),
> +                'Yakkety': Version('16.10'),
> +                'Zesty': Version('17.4'),
> +                'Artful': Version('17.10'),
> +                'Bionic': Version('18.04'),
> +            },

Linux has many distributions. And current WebKitGTK+ supports Fedora, Debian and Ubuntu at least.
Listing all the versions of the distributions is not scalable. And making webkit-patch unavailable in unsupported Linux distributions is not good.
I think we should have a fallback Version() which is used if the current environment is not recognized well by this VersionMap.

[Yusuke Suzuki]

One way to get codename of Linux is using `lsb_release -c` and `lsb_release -i`.

On Ubuntu 16.04 xenial,

$ lsb_release -c
Codename:       xenial
$ lsb_release -i
Distributor ID: Ubuntu

On Debian unstable (sid),

$ lsb_release -c
Codename:       sid

$ lsb_release -i
Distributor ID: Debian

Not sure on Fedora.

I'm not sure what offers version string in Linux in webkitpy right now. I guess we are using `uname`. But it returns the version of Linux kernel, not distribution's version.

But `lsb_release` would not be installed. 
So anyway, I think we should have a fallback Version if we failed to get version information. Currently, VersionMap to_name returns None if it fails to find an appropriate version. And `assert self.os_version is not None` fires.

[Yusuke Suzuki]

It seems that current Linux version is offered by python's platform module. `platform.release()` returns the version of Linux kernel. So it is not intended one I think.
The closest one is `platform.dist()`, which returns `('Ubuntu', '16.04', 'xenial')` on Ubuntu 16.04 environment. But it is noted that this function is deprecated and removed in python 3.7[1]. So using this is not a good choice.

Additional note is that some Linux channels does not have a specific version number. For example, Debian unstable (sid) and Fedora rawhide are rolling release. It is always `unstable` or `rawhide`. It does not have a version number. If we execute the above `platform.dist()` on Debian unstable, it returns `('debian', 'buster/sid', '')`.
So, we should have a Version which number is unknown.

[1]: https://docs.python.org/3.6/library/platform.html#platform.dist

[Michael Catanzaro]

Yeah, it needs to work on any Linux.

Don't rely on lsb_release; LSB is very dead, and I wouldn't count on that existing everywhere in the future. If you *really* need to get an OS name/version for some reason, then I would look in /etc/os-release. But we also need to support the case where that file does not exist, or (unlikely) does exist but does not contain name/version fields.

(Hopefully this functionality is not actually used for anything on Linux?)

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #18)
> (Hopefully this functionality is not actually used for anything on Linux?)

I also wonder, which is the use case of this for Linux ports?
AFAIK the tooling used by the GTK or WPE ports doesn't need to know the name or version of the distribution where it runs.

[Jonathan Bedard]

Right now, we're using version in test expectations for Windows and Mac/iOS.  I don't think it is actually used in any meaningful way for Linux at the moment, we should be able to get away with the default version (as we've been doing) or perhaps even setting the os_version to None if it cannot be determined.

[Jonathan Bedard]

Created attachment 327644 [details]
Patch

[Jonathan Bedard]

attachment 327644 [details] removes Linux versions entirely, since we aren't using them for anything (nor do I think is it likely we will ever do so).

If you grep for os_version in webkitpy and ignore unit tests, you will find we only use that variable in Windows, Mac and iOS code paths.

[David Kilzer (:ddkilzer)]

Comment on attachment 327644 [details]
Patch

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

r=me if all bots pass with this patch.

> Tools/Scripts/webkitpy/common/system/platforminfo.py:62
> -        if self.os_name == 'freebsd' or self.os_name == 'openbsd' or self.os_name == 'netbsd' or self.os_name == 'ios':
> -            self.os_version = platform_module.release()
> +        self.os_version = None
> +
>          if self.os_name.startswith('mac'):
> -            self.os_version = self._determine_mac_version(Version(platform_module.mac_ver()[0]))
> -        if self.os_name.startswith('win'):
> -            self.os_version = self._determine_win_version(self._win_version(sys_module))
> +            version = Version(platform_module.mac_ver()[0])
> +        elif self.os_name.startswith('win'):
> +            version = self._win_version(sys_module)
> +        elif self.os_name == 'freebsd':
> +            version = platform_module.release().split('-')[0]

This removes support for 'openbsd' and 'netbsd'.  Will anyone care?  I suppose it depends on whether anyone is running WebKit tests on those platforms.

[Jonathan Bedard]

Comment on attachment 327644 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/platforminfo.py:62
>> +            version = platform_module.release().split('-')[0]
> 
> This removes support for 'openbsd' and 'netbsd'.  Will anyone care?  I suppose it depends on whether anyone is running WebKit tests on those platforms.

Not exactly.

'openbsd' and 'netbsd' will use the 'else:' codepath here.  They will end up throwing an exception when they aren't in the VersionNameMap, so perhaps we should give these platforms the same treatment as linux.

[Michael Catanzaro]

Yes, give them the same treatment as Linux, please.

[Jonathan Bedard]

Created attachment 327651 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 327651 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/platforminfo.py:64
> +        elif self.os_name == 'freebsd':
> +            version = platform_module.release().split('-')[0]
> +        elif self.os_name == 'linux' or self.os_name == 'openbsd' or self.os_name == 'netbsd':
> +            version = None

What about FreeBSD?
If anyone is running tests there, then it is likely using the GTK port (AFAIK the only port that builds on that OS), so I think we should give to FreeBSD the same treatment than to Linux.

[Jonathan Bedard]

(In reply to Carlos Alberto Lopez Perez from comment #27)
> Comment on attachment 327651 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327651&action=review
> 
> > Tools/Scripts/webkitpy/common/system/platforminfo.py:64
> > +        elif self.os_name == 'freebsd':
> > +            version = platform_module.release().split('-')[0]
> > +        elif self.os_name == 'linux' or self.os_name == 'openbsd' or self.os_name == 'netbsd':
> > +            version = None
> 
> What about FreeBSD?
> If anyone is running tests there, then it is likely using the GTK port
> (AFAIK the only port that builds on that OS), so I think we should give to
> FreeBSD the same treatment than to Linux.

Looked back at when I had landed this previously.  FreeBSD also needs the same treatment.  I'm a little surprised that GTK EWS didn't catch this, this the rest of the CI certainly did <https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/4275>.

[Carlos Alberto Lopez Perez]

(In reply to Jonathan Bedard from comment #28)
> Looked back at when I had landed this previously.  FreeBSD also needs the
> same treatment.  I'm a little surprised that GTK EWS didn't catch this, this
> the rest of the CI certainly did
> <https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/4275>.

Currently the GTK EWS only tests to build WebKitGTK+, but it doesn't run any test.

[Jonathan Bedard]

Created attachment 327660 [details]
Patch for landing

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #30)
> Created attachment 327660 [details]
> Patch for landing

Just realized that will break future iOS ports.

[Jonathan Bedard]

Created attachment 327685 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 327685 [details]
Patch for landing

Clearing flags on attachment: 327685

Committed r225199: <https://trac.webkit.org/changeset/225199>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

I filed a bug for Cygwin.
 Bug 180069 – webkitpy: PlatformInfo raises AssertionError "assert self.os_version is not None" in Cygwin since Bug 179621

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 328706 [details]
Patch

[EWS Watchlist]

Attachment 328706 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:496:  [RemoveConfigurationsTest.test_remove_line] Too many positional arguments for function call  [pylint/E1121] [5]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 329134 [details]
Patch

[EWS Watchlist]

Attachment 329134 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:496:  [RemoveConfigurationsTest.test_remove_line] Too many positional arguments for function call  [pylint/E1121] [5]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Comment on attachment 329134 [details]
Patch

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

r=me with a few questions below.

> Tools/Scripts/webkitpy/common/system/platforminfo_unittest.py:81
> -        self.assertNotEquals(info.os_version, '')
> +        if info.is_mac() or info.is_win():
> +            self.assertNotEquals(info.os_version, None)

Would this make more sense or be easier to read?

    if info.is_mac() or info.is_win():
        self.assertIsNotNone(info.os_version)

> Tools/Scripts/webkitpy/port/base.py:776
> -    def version(self):
> +    def version_name(self):
>          """Returns a string indicating the version of a given platform, e.g.
>          'leopard' or 'xp'.
>  
>          This is used to help identify the exact port when parsing test
>          expectations, determining search paths, and logging information."""
> -        return self._version
> +        if self._os_version is None:
> +            return None
> +        return VersionNameMap.map(self.host.platform).to_name(self._os_version, table=PUBLIC_TABLE)

Can this be @memoized, or will it change during the life of the process?

> Tools/Scripts/webkitpy/port/factory_unittest.py:59
> -        self.assert_port(port_name='mac', os_name='mac', os_version='lion', cls=mac.MacPort)
> -        self.assert_port(port_name=None,  os_name='mac', os_version='lion', cls=mac.MacPort)
> +        self.assert_port(port_name='mac', os_name='mac', os_version=Version(10, 7), cls=mac.MacPort)
> +        self.assert_port(port_name=None,  os_name='mac', os_version=Version(10, 7), cls=mac.MacPort)

I think 'lion' is a bit more descriptive here, but the alternative seems to be this, which is way too verbose:

    VersionNameMap.map().from_name('lion')

I guess I wish there was some kind of a (static) helper function like this:

    Version.fromName('lion')

This isn't necessary to fix; just thinking about whether this change would make the tests easier or harder to update (or to simply read) later.

I guess one benefit of doing this would be not having to import PUBLIC_TABLE, VersionNameMap in as many places.  (I'm not sure if this is possible given the current dependencies and use of apple_additions(), though.)

Or maybe this is what you warned me about when you told me about the patch, and I forgot! :)

> Tools/Scripts/webkitpy/port/factory_unittest.py:66
> -        self.assert_port(port_name='win', os_name='win', os_version='xp', cls=win.WinPort)
> -        self.assert_port(port_name=None, os_name='win', os_version='xp', cls=win.WinPort)
> -        self.assert_port(port_name=None, os_name='win', os_version='xp', options=self.webkit_options, cls=win.WinPort)
> +        self.assert_port(port_name='win', os_name='win', os_version=Version(5, 1), cls=win.WinPort)
> +        self.assert_port(port_name=None, os_name='win', os_version=Version(5, 1), cls=win.WinPort)
> +        self.assert_port(port_name=None, os_name='win', os_version=Version(5, 1), options=self.webkit_options, cls=win.WinPort)

I don't think I'd know/remember that Windows XP was version 5.1.  (See above. :)

> Tools/Scripts/webkitpy/port/ios.py:41
> +    VERSION_MIN = Version(11)
> +    VERSION_MAX = Version(12)

Is VERSION_MAX correct here?  (I ask because I asked about the mac port below using (10, 14).)

> Tools/Scripts/webkitpy/port/ios_simulator.py:250
> -            if mac_os_version in ['elcapitan', 'yosemite', 'mavericks']:
> +            if mac_os_version < Version(10, 11):
>                  time.sleep(2.5)

Here's where I wish Version.fromName('elcapitan') existed, too.

Shouldn't this be change to the following (since the old check was inclusive of El Capitan, but the new check is exclusive of El Capitan):

        if mac_os_version < Version(10, 12):

> Tools/Scripts/webkitpy/port/mac.py:48
> -    VERSION_FALLBACK_ORDER = ['mac-snowleopard', 'mac-lion', 'mac-mountainlion', 'mac-mavericks', 'mac-yosemite', 'mac-elcapitan', 'mac-sierra', 'mac-highsierra']
> +    VERSION_MIN = Version(10, 6)
> +    VERSION_MAX = Version(10, 14)

Is VERSION_MAX correct here?  The old VERSION_FALLBACK_ORDER stopped at (10, 13).

> Tools/Scripts/webkitpy/port/win.py:-439
> -    VERSION_FALLBACK_ORDER = ["wincairo-" + os_name.lower() for os_name in VersionNameMap.map().names()]

Does removing this mean that folder names on disk have to change, too?

[Jonathan Bedard]

Comment on attachment 329134 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/factory_unittest.py:59
>> +        self.assert_port(port_name=None,  os_name='mac', os_version=Version(10, 7), cls=mac.MacPort)
> 
> I think 'lion' is a bit more descriptive here, but the alternative seems to be this, which is way too verbose:
> 
>     VersionNameMap.map().from_name('lion')
> 
> I guess I wish there was some kind of a (static) helper function like this:
> 
>     Version.fromName('lion')
> 
> This isn't necessary to fix; just thinking about whether this change would make the tests easier or harder to update (or to simply read) later.
> 
> I guess one benefit of doing this would be not having to import PUBLIC_TABLE, VersionNameMap in as many places.  (I'm not sure if this is possible given the current dependencies and use of apple_additions(), though.)
> 
> Or maybe this is what you warned me about when you told me about the patch, and I forgot! :)

I like this suggestion replace similar calls with Version.from_name().

>> Tools/Scripts/webkitpy/port/win.py:-439
>> -    VERSION_FALLBACK_ORDER = ["wincairo-" + os_name.lower() for os_name in VersionNameMap.map().names()]
> 
> Does removing this mean that folder names on disk have to change, too?

No.

This will adopt the WinPort's version numbers (because they're the same) but use the WinCairoPort's names because we aren't converting version to name until the default_baseline_search_path(...) below.

[Jonathan Bedard]

Comment on attachment 329134 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/ios.py:41
>> +    VERSION_MAX = Version(12)
> 
> Is VERSION_MAX correct here?  (I ask because I asked about the mac port below using (10, 14).)

Yes, Mac is a bit special because of the 'future' idiom and inherited expectations.

iOS is a bit more straight-forward.

>> Tools/Scripts/webkitpy/port/mac.py:48
>> +    VERSION_MAX = Version(10, 14)
> 
> Is VERSION_MAX correct here?  The old VERSION_FALLBACK_ORDER stopped at (10, 13).

If you notice in constructing the version fallback order, I handle this distinction (by using -1 so 10.14 isn't in the fallback order).

Essentially, the VERSION_MIN and VERSION_MAX can both be used to calculate the VERSION_FALLBACK_ORDER and used to determine which versions tests can be ran on.

[Jonathan Bedard]

Created attachment 329181 [details]
Patch

[Jonathan Bedard]

The plan is to land attachment 329181 [details] tomorrow morning (Dec 13th) after it has cleared EWS and I am able to repair any unexpected fallout.

[EWS Watchlist]

Attachment 329181 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:496:  [RemoveConfigurationsTest.test_remove_line] Too many positional arguments for function call  [pylint/E1121] [5]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 329219 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 329219 [details]:

svg/animations/svgtransform-animation-1.html bug 179354 (authors: [e-mail], [e-mail], [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 329219 [details]
Patch for landing

Clearing flags on attachment: 329219

Committed r225856: <https://trac.webkit.org/changeset/225856>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

(In reply to WebKit Commit Bot from comment #49)
> Comment on attachment 329219 [details]
> Patch for landing
> 
> Clearing flags on attachment: 329219
> 
> Committed r225856: <https://trac.webkit.org/changeset/225856>

I got this ugly warning when trying to upload a patch

$ Tools/Scripts/webkit-patch upload -g HEAD --suggest-reviewers --request-commit
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/style/main.py", line 155, in main
    patch_checker.check(patch)
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/style/patchreader.py", line 84, in check
    self._text_file_reader.do_association_check(fs.getcwd())
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/style/filereader.py", line 152, in do_association_check
    self._processor.do_association_check(self._files, cwd, host=host)
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/style/checker.py", line 963, in do_association_check
    TestExpectationsChecker.lint_test_expectations(files, self._configuration, cwd, self._increment_error_count, host=host)
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 129, in lint_test_expectations
    for expectations_file in port.expectations_dict().keys():
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/port/base.py", line 1128, in expectations_dict
    for path in self.expectations_files():
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/port/base.py", line 1160, in expectations_files
    return [self.path_to_generic_test_expectations_file()] + self._port_specific_expectations_files()
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/port/darwin.py", line 56, in _port_specific_expectations_files
    return list(reversed([self._filesystem.join(self._webkit_baseline_path(p), 'TestExpectations') for p in self.baseline_search_path()]))
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/port/base.py", line 205, in baseline_search_path
    return self.get_option('additional_platform_directory', []) + self._compare_baseline() + self.default_baseline_search_path()
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/common/memoized.py", line 45, in __call__
    result = self._function(*args)
  File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/port/mac.py", line 73, in default_baseline_search_path
    version_fallback = sorted_versions[sorted_versions.index(self._os_version):-1]
ValueError: <webkitpy.common.version.Version object at 0x7f1b8cea4b50> is not in list
Are you sure you want to continue? [Y/n]: 

Reverting 225856 locally makes this warning go away.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #51)

> list(reversed([self._filesystem.join(self._webkit_baseline_path(p),
> 'TestExpectations') for p in self.baseline_search_path()]))
>   File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/port/base.py",
> line 205, in baseline_search_path
>     return self.get_option('additional_platform_directory', []) +
> self._compare_baseline() + self.default_baseline_search_path()
>   File
> "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/common/memoized.py", line
> 45, in __call__
>     result = self._function(*args)
>   File "/home/clopez/webkit/webkit/Tools/Scripts/webkitpy/port/mac.py", line
> 73, in default_baseline_search_path
>     version_fallback =
> sorted_versions[sorted_versions.index(self._os_version):-1]
> ValueError: <webkitpy.common.version.Version object at 0x7f1b8cea4b50> is
> not in list
> Are you sure you want to continue? [Y/n]: 
> 
> Reverting 225856 locally makes this warning go away.

Also note that I run Linux.. I have no idea why the code ends in "webkitpy/port/mac.py". Something is wrong with r225856

[Jonathan Bedard]

(In reply to Carlos Alberto Lopez Perez from comment #52)
> (In reply to Carlos Alberto Lopez Perez from comment #51)
> 
> ...

I’ll fix this tomorrow morning.

The code is being run to check that the test expectations are correct, so it’s correct to be calling into mac code from Linux. But clearly we aren’t instantiating everything the MacPort needs.

[Jonathan Bedard]

Committed r225904: <https://trac.webkit.org/changeset/225904>

[Jonathan Bedard]

Future baseline search paths for Mac were not being handled correctly: <https://trac.webkit.org/changeset/226786>