new-run-webkit-tests fails on the Lion seed. Possibly a Python versioning issue? $ new-run-webkit-tests Traceback (most recent call last): File "/Volumes/SnowLeopard/Users/mjs/Work/src/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 439, in <module> sys.exit(main()) File "/Volumes/SnowLeopard/Users/mjs/Work/src/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 433, in main port_obj = port.get(options.platform, options) File "/Volumes/SnowLeopard/Users/mjs/Work/src/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/factory.py", line 66, in get return _get_kwargs(**kwargs) File "/Volumes/SnowLeopard/Users/mjs/Work/src/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/factory.py", line 128, in _get_kwargs return maker(**kwargs) File "/Volumes/SnowLeopard/Users/mjs/Work/src/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/mac.py", line 78, in __init__ self._version = os_version(os_version_string) File "/Volumes/SnowLeopard/Users/mjs/Work/src/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/mac.py", line 55, in os_version version_string = version_strings[release_version] KeyError: 7
NRWT hard codes the o/s versions it supports. I imagine it is bailing out here because it doesn't recognize 10.7 as a supported version, which is trivial to fix. Obviously, the error message sucks, but I have a bunch of patches I need to post to clean up all of the error handling, so it'll get better. I'll have to poke around and see if I can get a Lion seed installed, but failing that, hopefully someone from Apple will be able to help me get this working?
I can help out with testing this for you.
It would be preferable if it didn’t hard-code the supported OS versions. There obviously needs to be a mapping from major version to name for layout test results (e.g., 10.6 -> "mac-snowleopard"), but newer versions should simply be mapped to “mac”.
Created attachment 87317 [details] Something like this… Here’s what that could look like.
This patch looks more or less fine. While I agree that "future" is a nice generic solution, it would probably be nice if "lion" worked as well. Do you want to wait until it's actually released and we can actually be sure that it is called Lion, or would you feel safe changing that now.
(In reply to comment #5) > This patch looks more or less fine. While I agree that "future" is a nice generic solution, it would probably be nice if "lion" worked as well. Do you want to wait until it's actually released and we can actually be sure that it is called Lion, or would you feel safe changing that now. The approach that we take to Mac OS X versions elsewhere in WebKit, including in run-webkit-tests, is for the current Mac OS X version to be unnamed and the default path, with special cases added for older Mac OS X versions. I think we should continue with that approach in new-run-webkit-tests.
(In reply to comment #6) > (In reply to comment #5) > > This patch looks more or less fine. While I agree that "future" is a nice generic solution, it would probably be nice if "lion" worked as well. Do you want to wait until it's actually released and we can actually be sure that it is called Lion, or would you feel safe changing that now. > > The approach that we take to Mac OS X versions elsewhere in WebKit, including in run-webkit-tests, is for the current Mac OS X version to be unnamed and the default path, with special cases added for older Mac OS X versions. I think we should continue with that approach in new-run-webkit-tests. new-run-webkit-tests can distinguish between "I need the mac port" and "I need version X of the mac port". We need to distinguish between the two in order to do things like check the test expectations across all of the support combinations of values and to manage baselines properly in the rebaselining tool. For example, The way the code is currently written, 'chromium-mac' indicates that you know you want a chromium mac port but the code should attempt to figure out the "right" version (usually the version it is being run on). 'chromium-mac-snowleopard' means "explicitly use the snowleopard' version, which also happens to use the 'chromium-mac' platform dir. This is part of a more general convention that uses the platform name to specify a combination of values (port implementation, operating system, os version, accelerated vs unaccelerated graphics, 32-bit vs 64-bit so far), where if you don't specify a value explicitly we attempt to do something sane, but you can always be explicit. Accordingly, I think it also makes sense to have a 'chromium-mac-lion' that specifies the version to use explicitly. It may also make sense to support a 'chromium-mac-future' value. Does that make more sense? Although after having thought about the naming for a bit, I actually like 'latest' better than 'future'.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > This patch looks more or less fine. While I agree that "future" is a nice generic solution, it would probably be nice if "lion" worked as well. Do you want to wait until it's actually released and we can actually be sure that it is called Lion, or would you feel safe changing that now. > > > > The approach that we take to Mac OS X versions elsewhere in WebKit, including in run-webkit-tests, is for the current Mac OS X version to be unnamed and the default path, with special cases added for older Mac OS X versions. I think we should continue with that approach in new-run-webkit-tests. > > new-run-webkit-tests can distinguish between "I need the mac port" and "I need version X of the mac port". We need to distinguish between the two in order to do things like check the test expectations across all of the support combinations of values and to manage baselines properly in the rebaselining tool. > > For example, The way the code is currently written, 'chromium-mac' indicates that you know you want a chromium mac port but the code should attempt to figure out the "right" version (usually the version it is being run on). 'chromium-mac-snowleopard' means "explicitly use the snowleopard' version, which also happens to use the 'chromium-mac' platform dir. This is part of a more general convention that uses the platform name to specify a combination of values (port implementation, operating system, os version, accelerated vs unaccelerated graphics, 32-bit vs 64-bit so far), where if you don't specify a value explicitly we attempt to do something sane, but you can always be explicit. > > Accordingly, I think it also makes sense to have a 'chromium-mac-lion' that specifies the version to use explicitly. It may also make sense to support a 'chromium-mac-future' value. > > Does that make more sense? I understand the design. As best as I can tell nothing about the design necessitates using “lion” over a generic name like “latest” or “future”. A generic name is highly preferable. In general people at Apple will be working on newer versions of Mac OS X than are publicly available. It’s untenable to require that specific names for future versions of Mac OS X be baked in to an open source script in order for us to use said script. > Although after having thought about the naming for a bit, I actually like 'latest' better than 'future'. I picked “future” as to the average WebKit developer Mac OS X 10.6 is the latest available version of Mac OS X. When Mac OS X 10.7 becomes the latest publicly available version of Mac OS X then we can add “lion” to the explicit mapping of version numbers to names, at which point “future” will be used on post-10.7 versions of Mac OS X.
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #5) > > > > This patch looks more or less fine. While I agree that "future" is a nice generic solution, it would probably be nice if "lion" worked as well. Do you want to wait until it's actually released and we can actually be sure that it is called Lion, or would you feel safe changing that now. > > > > > > The approach that we take to Mac OS X versions elsewhere in WebKit, including in run-webkit-tests, is for the current Mac OS X version to be unnamed and the default path, with special cases added for older Mac OS X versions. I think we should continue with that approach in new-run-webkit-tests. > > > > new-run-webkit-tests can distinguish between "I need the mac port" and "I need version X of the mac port". We need to distinguish between the two in order to do things like check the test expectations across all of the support combinations of values and to manage baselines properly in the rebaselining tool. > > > > For example, The way the code is currently written, 'chromium-mac' indicates that you know you want a chromium mac port but the code should attempt to figure out the "right" version (usually the version it is being run on). 'chromium-mac-snowleopard' means "explicitly use the snowleopard' version, which also happens to use the 'chromium-mac' platform dir. This is part of a more general convention that uses the platform name to specify a combination of values (port implementation, operating system, os version, accelerated vs unaccelerated graphics, 32-bit vs 64-bit so far), where if you don't specify a value explicitly we attempt to do something sane, but you can always be explicit. > > > > Accordingly, I think it also makes sense to have a 'chromium-mac-lion' that specifies the version to use explicitly. It may also make sense to support a 'chromium-mac-future' value. > > > > Does that make more sense? > > I understand the design. As best as I can tell nothing about the design necessitates using “lion” over a generic name like “latest” or “future”. Correct. > A generic name is highly preferable. In general people at Apple will be working on newer versions of Mac OS X than are publicly available. It’s untenable to require that specific names for future versions of Mac OS X be baked in to an open source script in order for us to use said script. > The counter argument is that you will often have bots configured to run a specific version of the operating system, even when the underlying directories change. For example, both Chromium and the base Mac ports have a "Snow Leopard" bot. Presumably when Lion ships, "mac" starts to refer to "lion" and you will now need a "snow leopard" indicator. Put differently, if you support "snowleopard", you don't need to know or change anything when "snowleopard" no longer indicates "latest". > > Although after having thought about the naming for a bit, I actually like 'latest' better than 'future'. > > I picked “future” as to the average WebKit developer Mac OS X 10.6 is the latest available version of Mac OS X. When Mac OS X 10.7 becomes the latest publicly available version of Mac OS X then we can add “lion” to the explicit mapping of version numbers to names, at which point “future” will be used on post-10.7 versions of Mac OS X. Right, and this is actually why I prefer "latest". Saying that "mac" (which is currently where the 10.6 bots find most of its baselines) refers to "future" doesn't make sense, but "latest" does.
(In reply to comment #9) > The counter argument is that you will often have bots configured to run a specific version of the operating system, even when the underlying directories change. For example, both Chromium and the base Mac ports have a "Snow Leopard" bot. Presumably when Lion ships, "mac" starts to refer to "lion" and you will now need a "snow leopard” indicator. We already make this distinction. There is already a mac-snowleopard directory containing results for use on SnowLeopard-and-earlier. > Put differently, if you support "snowleopard", you don't need to know or change anything when "snowleopard" no longer indicates "latest”. The term “latest” here is ambiguous so I’m not clear on what you mean. > > > Although after having thought about the naming for a bit, I actually like 'latest' better than 'future'. > > > > I picked “future” as to the average WebKit developer Mac OS X 10.6 is the latest available version of Mac OS X. When Mac OS X 10.7 becomes the latest publicly available version of Mac OS X then we can add “lion” to the explicit mapping of version numbers to names, at which point “future” will be used on post-10.7 versions of Mac OS X. > > Right, and this is actually why I prefer "latest". Saying that "mac" (which is currently where the 10.6 bots find most of its baselines) refers to "future" doesn't make sense, but "latest" does. I don’t understand what you’re saying here. Bots working with 10.6 would presumably deal with results for 10.6, not results for 10.7. I don’t see how the name used for an OS version they aren’t working with is relevant to them.
I talked with Dirk on IRC about this and we came to agreement that “future” is a reasonable name (due to the ambiguity as to what the “latest” version is) and that “mac-lion” being an acceptable version identifier will be done when Lion is released.
Created attachment 87434 [details] Patch
Comment on attachment 87434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87434&action=review > Tools/Scripts/webkitpy/layout_tests/port/mac.py:52 > 6: 'snowleopard', Do you need to add a 7: 'future', here? If it's not here, I would expect the assert on line 110 of mac_unittest.py to fail. Of course you could code this as if release_version > max(version_strings.keys()) or something as well. Patch looks good otherwise. I'd R+ it assuming the above thing is fixed, but I'm not a reviewer...
Attachment 87434 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #13) > (From update of attachment 87434 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87434&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:52 > > 6: 'snowleopard', > > Do you need to add a 7: 'future', here? If it's not here, I would expect the assert on line 110 of mac_unittest.py to fail. > > Of course you could code this as if release_version > max(version_strings.keys()) or something as well. This is handled by: 55 version_string = version_strings.get(release_version, 'future’) If release_version is not found in version_strings then “future” is used.
(In reply to comment #15) > (In reply to comment #13) > > (From update of attachment 87434 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=87434&action=review > > > > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:52 > > > 6: 'snowleopard', > > > > Do you need to add a 7: 'future', here? If it's not here, I would expect the assert on line 110 of mac_unittest.py to fail. > > > > Of course you could code this as if release_version > max(version_strings.keys()) or something as well. > > This is handled by: > > 55 version_string = version_strings.get(release_version, 'future’) > > If release_version is not found in version_strings then “future” is used. Ah, right. I'm blind. Yeah, that looks good.
Comment on attachment 87434 [details] Patch Looks fine to me, and I trust Dirk's unofficial review.
Landed in r82752.