Bug 179621

<rdar://problem/35589585>

Created attachment 327067 [details] Patch

Created attachment 327068 [details] Patch

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.

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.

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.

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'

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.

Created attachment 327249 [details] Patch

Comment on attachment 327249 [details] Patch Clearing flags on attachment: 327249 Committed r225016: <https://trac.webkit.org/changeset/225016>

All reviewed patches have been landed. Closing bug.

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

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

Reverted r225016 for reason: Break webkit-patch on Ubuntu and Debian Linux Committed r225033: <https://trac.webkit.org/changeset/225033>

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.

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.

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

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

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

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.

Created attachment 327644 [details] Patch

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.

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.

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.

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

Created attachment 327651 [details] Patch

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.

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

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

Created attachment 327660 [details] Patch for landing

(In reply to Jonathan Bedard from comment #30) > Created attachment 327660 [details] > Patch for landing Just realized that will break future iOS ports.

Created attachment 327685 [details] Patch for landing

Comment on attachment 327685 [details] Patch for landing Clearing flags on attachment: 327685 Committed r225199: <https://trac.webkit.org/changeset/225199>

All reviewed patches have been landed. Closing bug.

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

Reopening to attach new patch.

Created attachment 328706 [details] Patch

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.

Created attachment 329134 [details] Patch

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.

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?

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.

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.

Created attachment 329181 [details] Patch

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.

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.

Created attachment 329219 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 329219 [details]: svg/animations/svgtransform-animation-1.html bug 179354 (authors: ap@webkit.org, arv@chromium.org, krit@webkit.org, mark.lam@apple.com, and zimmermann@kde.org) The commit-queue is continuing to process your patch.

Comment on attachment 329219 [details] Patch for landing Clearing flags on attachment: 329219 Committed r225856: <https://trac.webkit.org/changeset/225856>

All reviewed patches have been landed. Closing bug.

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

(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

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

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

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