Bug 179426

<rdar://problem/35415191>

Created attachment 326334 [details] Patch

Comment on attachment 326334 [details] Patch Looks like it broke some iOS Sim stuff.

Created attachment 326341 [details] Patch

Created attachment 326377 [details] Patch

Created attachment 326411 [details] Patch

Comment on attachment 326411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326411&action=review r=me You don't have to make any of the suggested changes before landing, but at least consider: - Tightening up __len__ and parsing of versions with more than 3 integers (maybe throw an exception due to "loss" of data)? - Add tests for __len__ (whether or not the above is done) and __setitem__ to make sure they work. - Consider using 'tiny' instead of 'build'. (The word 'build' is very over-loaded, and 'build version' means something different than the 3rd integer in a dotted version string at Apple.) > Tools/ChangeLog:20 > + (PlatformInfo.__init__): Convert mac version string to version object and > + use _win_version instead of _win_version_tuple. > + (PlatformInfo.xcode_sdk_version): Convert SDK version string to Version object > + before returning it. > + (PlatformInfo.xcode_version): Return version object instead of version string. > + (PlatformInfo._determine_mac_version): Accept version object instead of string, > + eliminate parsing. > + (PlatformInfo._determine_win_version): Accept version object instead of tuple. Nit: Not using capital "Version object" consistently. Doesn't have to be fixed for landing. > Tools/Scripts/webkitpy/common/version.py:29 > + self.major = 0 > + self.minor = 0 > + self.build = 0 In Source/JavaScriptCore/Configurations/Version.xcconfig,the following names are used for each position: MAJOR_VERSION = 605; MINOR_VERSION = 1; TINY_VERSION = 13; MICRO_VERSION = 0; NANO_VERSION = 0; Not sure this class needs to support 5 dotted versions, but you might consider renaming 'build' to 'tiny' for consistency. (I actually don't know what the history of those names is, though.) Not required to land the patch. > Tools/Scripts/webkitpy/common/version.py:43 > + raise ValueError('Version expected to be int, str, tuple or list of ints') Nit: Spell out "ints" as "integers". > Tools/Scripts/webkitpy/common/version.py:46 > + def __len__(self): > + return 3 This doesn't return a correct value if one of the for loops in the __init__ method is used as it will set keys for '3' and '4' (or more). May not matter currently, but it should do the right thing if we support passing in "1.2.3.4" or "1.2.3.4.5". Hmm...then again, we raise an exception in __getitem__ and __setitem__ if the key is larger than '2'. Not sure what you want to do here. > Tools/Scripts/webkitpy/common/version.py:98 > + def __str__(self): > + result = '{}'.format(self.major) > + if self.minor or self.build: > + result += '.{}'.format(self.minor) > + if self.build: > + result += '.{}'.format(self.build) > + return result If we support more than 3 dotted numbers (see above comments), may want to make this more generic? > Tools/Scripts/webkitpy/common/version_unittest.py:28 > +class VersionTestCase(unittest.TestCase): One of these tests should verify __len__. You should also have a test for using __setitem__ with both numbers (0, 1, 2) and strings (major, minor, build/tiny). > Tools/Scripts/webkitpy/common/system/platforminfo.py:61 > if self.os_name.startswith('mac'): > - self.os_version = self._determine_mac_version(platform_module.mac_ver()[0]) > + 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_tuple(sys_module)) > + self.os_version = self._determine_win_version(self._win_version(sys_module)) Kind of weird how platform_module.mac_ver() doesn't return a Version object here, but this doesn't block the patch from landing. Maybe too much to change for this patch (which is already pretty large)? > Tools/Scripts/webkitpy/port/ios_simulator.py:191 > def use_multiple_simulator_apps(self): > - return int(self.host.platform.xcode_version().split('.')[0]) < 9 > + return int(self.host.platform.xcode_version()[0]) < 9 After reading the patch up to this point, I find that using major/minor/build is probably more readable that 0/1/2. Not necessary to fix before landing, though.

Comment on attachment 326411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326411&action=review >> Tools/Scripts/webkitpy/common/version.py:29 >> + self.build = 0 > > In Source/JavaScriptCore/Configurations/Version.xcconfig,the following names are used for each position: > > MAJOR_VERSION = 605; > MINOR_VERSION = 1; > TINY_VERSION = 13; > MICRO_VERSION = 0; > NANO_VERSION = 0; > > Not sure this class needs to support 5 dotted versions, but you might consider renaming 'build' to 'tiny' for consistency. > > (I actually don't know what the history of those names is, though.) > > Not required to land the patch. I like those names better, I will use them. >> Tools/Scripts/webkitpy/common/version.py:46 >> + return 3 > > This doesn't return a correct value if one of the for loops in the __init__ method is used as it will set keys for '3' and '4' (or more). > > May not matter currently, but it should do the right thing if we support passing in "1.2.3.4" or "1.2.3.4.5". > > Hmm...then again, we raise an exception in __getitem__ and __setitem__ if the key is larger than '2'. Not sure what you want to do here. I haven't seen us passing any 4 or 5 indexed versions around. The current code will throw an exception if we tried to construct with that size (because of the exception in __getitem__ and __setitem__) Since we have JavaScriptCore code which reference micro and nano versions as well, I will allow for those too. >> Tools/Scripts/webkitpy/common/system/platforminfo.py:61 >> + self.os_version = self._determine_win_version(self._win_version(sys_module)) > > Kind of weird how platform_module.mac_ver() doesn't return a Version object here, but this doesn't block the patch from landing. > > Maybe too much to change for this patch (which is already pretty large)? This will be addressed in a future patch. Ultimately, we need an os_version -> os_name mapping to solve this.

Created attachment 326498 [details] Patch

Comment on attachment 326498 [details] Patch Clearing flags on attachment: 326498 Committed r224657: <https://trac.webkit.org/changeset/224657>

All reviewed patches have been landed. Closing bug.

Committed r224658: <https://trac.webkit.org/changeset/224658>

Windows seems to return a namedtuple instead of a standard tuple.

5th element of getwindowsversion() is ''. This causes another error. Should be truncated like sys.getwindowsversion()[0:3]. > ActivePython 2.7.10.12 (ActiveState Software Inc.) based on > Python 2.7.10 (default, Aug 21 2015, 12:07:58) [MSC v.1500 64 bit (AMD64)] on win32 > Type "help", "copyright", "credits" or "license" for more information. > >>> import sys > >>> sys.getwindowsversion() > sys.getwindowsversion(major=6, minor=2, build=9200, platform=2, service_pack='') > >>> sys.getwindowsversion()[0:3] > (6, 2, 9200) > >>>

Filed Bug 179520 for ActivePython issue.

r224657 breaks Cygwin Python, too. > 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 "/cygdrive/c/webkit/ga/Tools/Scripts/webkitpy/tool/main.py", line 57, in __init__ > Host.__init__(self) > File "/cygdrive/c/webkit/ga/Tools/Scripts/webkitpy/common/host.py", line 48, in __init__ > SystemHost.__init__(self) > File "/cygdrive/c/webkit/ga/Tools/Scripts/webkitpy/common/system/systemhost.py", line 40, in __init__ > self.user = user.User() > File "/cygdrive/c/webkit/ga/Tools/Scripts/webkitpy/common/system/user.py", line 61, in __init__ > self._platforminfo = platforminfo or PlatformInfo(sys, platform, Executive()) > File "/cygdrive/c/webkit/ga/Tools/Scripts/webkitpy/common/system/platforminfo.py", line 61, in __init__ > self.os_version = self._determine_win_version(self._win_version(sys_module)) > File "/cygdrive/c/webkit/ga/Tools/Scripts/webkitpy/common/system/platforminfo.py", line 197, in _win_version > return Version(self._executive.run_command(['cmd', '/c', 'ver'], decode_output=False)) > File "/cygdrive/c/webkit/ga/Tools/Scripts/webkitpy/common/version.py", line 38, in __init__ > self[i] = ver.split('.')[i] > File "/cygdrive/c/webkit/ga/Tools/Scripts/webkitpy/common/version.py", line 73, in __setitem__ > self.major = int(value) > ValueError: invalid literal for int() with base 10: 'Microsoft Windows [Version 10' This is the output of ver command. > C:\>ver > > Microsoft Windows [Version 10.0.14393]

One more issue. Bug 179522 – check-webkit-style: AttributeError: 'NoneType' object has no attribute 'major'

The overloaded Version constructors with suboptimal parsing of version numbers given as a string and inconsistent usage of Version constructors throughout this patch just make me sad.

(In reply to Daniel Bates from comment #18) > The overloaded Version constructors with suboptimal parsing of version > numbers given as a string and inconsistent usage of Version constructors > throughout this patch just make me sad. Can you be more specific about what exactly you'd change? For example: - What is suboptimal about the parsing of the version numbers given as a string? - How would you fix the overloading of the constructors? - How could Version constructor usage be made more consistent (or does this go hand-in-hand with overloading of the constructor)?

I talked with Dan today (11/14). His complaint is that the Version class is too permissive and that we should pick which method of Version parsing we would like to standardize. I think we should standardize the string method. With that being said, I still think that there should be sane defaults when passing None, String and Version. Tracking this work here: <https://bugs.webkit.org/show_bug.cgi?id=179677>.

Filed for Cygwin. Bug 180069 – webkitpy: PlatformInfo raises AssertionError "assert self.os_version is not None" in Cygwin since Bug 179426