Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1830253003:
[Telemetry] Update the code that detect windows version to use CurrentMajorVersionNumber (Closed)
|
Created:
4 years, 9 months ago by nednguyen Modified:
4 years, 9 months ago Reviewers:
brucedawson CC:
catapult-reviews_chromium.org Base URL:
https://github.com/catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Telemetry] Update the code that detect windows version to use CurrentMajorVersionNumber
Since Windows 8, platform.uname()'s third field starts with 6.2, hence to detect
Windows 10, we have to use CurrentMajorVersionNumber from registry.
BUG=chromium:596640
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/586fde52438f283f7bafb27e4c85ccaee593c4b0
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address Bruce comment #
Total comments: 2
Patch Set 3 : Add win8.1 #
Messages
Total messages: 15 (3 generated)
nednguyen@google.com changed reviewers: + brucedawson@chromium.org
See comments below. Also note that the change description should probably acknowledge the manifest-based reason for why the version number is capped. https://codereview.chromium.org/1830253003/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/platform/win_platform_backend.py (right): https://codereview.chromium.org/1830253003/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/platform/win_platform_backend.py:39: import _winreg # pylint: disable=import-error Note that this will fail with cygwin. See https://codereview.chromium.org/1614663003 for an example of importing _winreg in a cygwin compatible way. https://codereview.chromium.org/1830253003/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/platform/win_platform_backend.py:242: # Since Windows 8, the os_version_module always starts with 6.2, hence This comment is misleading. It is worth mentioning the *reason* that platform.uname() stops at 6.2. The reason is that the version of python.exe that we are using is only marked (through a manifest) as being compatible with Windows versions up to 8. Therefore Windows *lies* to python about the version number. This is important because if we ever upgrade Python then we might start seeing different information in platform.uname(). One option would be to use _winreg to retrieve CurrentVersion from the registry, which goes up to 6.3 (Windows 8.1) and then use _winreg to retrieve CurrentMajorVersionNumber of 6.3 is found. This shows an example of retrieving versions up to 8.1. Adding 10 would be straightforward. https://codereview.chromium.org/1588673004/diff/180001/win_toolchain/get_tool...
https://codereview.chromium.org/1830253003/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/platform/win_platform_backend.py (right): https://codereview.chromium.org/1830253003/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/platform/win_platform_backend.py:39: import _winreg # pylint: disable=import-error On 2016/03/24 20:37:58, brucedawson wrote: > Note that this will fail with cygwin. See > https://codereview.chromium.org/1614663003 for an example of importing _winreg > in a cygwin compatible way. Done. But telemetry does not support cygwin, so I will leave that part out. https://codereview.chromium.org/1830253003/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/platform/win_platform_backend.py:242: # Since Windows 8, the os_version_module always starts with 6.2, hence On 2016/03/24 20:37:58, brucedawson wrote: > This comment is misleading. It is worth mentioning the *reason* that > platform.uname() stops at 6.2. The reason is that the version of python.exe that > we are using is only marked (through a manifest) as being compatible with > Windows versions up to 8. Therefore Windows *lies* to python about the version > number. > > This is important because if we ever upgrade Python then we might start seeing > different information in platform.uname(). > > One option would be to use _winreg to retrieve CurrentVersion from the registry, > which goes up to 6.3 (Windows 8.1) and then use _winreg to retrieve > CurrentMajorVersionNumber of 6.3 is found. > > This shows an example of retrieving versions up to 8.1. Adding 10 would be > straightforward. > > https://codereview.chromium.org/1588673004/diff/180001/win_toolchain/get_tool... Updated comment. I keep the code that sniff Windows 7 & below the same to make sure that I don't regress the old logic.
On 2016/03/24 22:56:24, nednguyen wrote: > https://codereview.chromium.org/1830253003/diff/1/telemetry/telemetry/interna... > File telemetry/telemetry/internal/platform/win_platform_backend.py (right): > > https://codereview.chromium.org/1830253003/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/platform/win_platform_backend.py:39: import _winreg > # pylint: disable=import-error > On 2016/03/24 20:37:58, brucedawson wrote: > > Note that this will fail with cygwin. See > > https://codereview.chromium.org/1614663003 for an example of importing _winreg > > in a cygwin compatible way. > > Done. But telemetry does not support cygwin, so I will leave that part out. > > https://codereview.chromium.org/1830253003/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/platform/win_platform_backend.py:242: # Since > Windows 8, the os_version_module always starts with 6.2, hence > On 2016/03/24 20:37:58, brucedawson wrote: > > This comment is misleading. It is worth mentioning the *reason* that > > platform.uname() stops at 6.2. The reason is that the version of python.exe > that > > we are using is only marked (through a manifest) as being compatible with > > Windows versions up to 8. Therefore Windows *lies* to python about the version > > number. > > > > This is important because if we ever upgrade Python then we might start seeing > > different information in platform.uname(). > > > > One option would be to use _winreg to retrieve CurrentVersion from the > registry, > > which goes up to 6.3 (Windows 8.1) and then use _winreg to retrieve > > CurrentMajorVersionNumber of 6.3 is found. > > > > This shows an example of retrieving versions up to 8.1. Adding 10 would be > > straightforward. > > > > > https://codereview.chromium.org/1588673004/diff/180001/win_toolchain/get_tool... > > Updated comment. I keep the code that sniff Windows 7 & below the same to make > sure that I don't regress the old logic. Ping Bruce again
Sorry for the delayed reply - for some reason I didn't see the updated version yesterday. https://codereview.chromium.org/1830253003/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/win_platform_backend.py (right): https://codereview.chromium.org/1830253003/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/win_platform_backend.py:255: elif os_version.startswith('6.2.'): Can we add an elif case for 6.3 that returns either WIN8 or WIN81? That way we will avoid having this code break in the future if python gets upgraded. Currently that would cause the NotImplementedError case to be hit.
Thanks for your reviews! https://codereview.chromium.org/1830253003/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/win_platform_backend.py (right): https://codereview.chromium.org/1830253003/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/win_platform_backend.py:255: elif os_version.startswith('6.2.'): On 2016/03/25 21:50:17, brucedawson wrote: > Can we add an elif case for 6.3 that returns either WIN8 or WIN81? That way we > will avoid having this code break in the future if python gets upgraded. > Currently that would cause the NotImplementedError case to be hit. Good call. Done.
lgtm
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1830253003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830253003/40001
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Update the code that detect windows version to use CurrentMajorVersionNumber Since Windows 8, platform.uname()'s third field starts with 6.2, hence to detect Windows 10, we have to use CurrentMajorVersionNumber from registry. BUG=chromium:596640 ========== to ========== [Telemetry] Update the code that detect windows version to use CurrentMajorVersionNumber Since Windows 8, platform.uname()'s third field starts with 6.2, hence to detect Windows 10, we have to use CurrentMajorVersionNumber from registry. BUG=chromium:596640 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
thanks for doing this FYI - python with no manifest (i.e. the version we use) on Windows 8.1 will have previously reported the OS version as 8.0 Now Windows 8.1 will correctly report 8.1 so if you have any rules that were previously "win8" you will probably want them updated to "win8.1" as that's what all the bots on the waterfall actually run.
Message was sent while issue was closed.
On 2016/03/26 06:09:04, Will Harris wrote: > thanks for doing this > > FYI - python with no manifest (i.e. the version we use) on Windows 8.1 will have > previously reported the OS version as 8.0 > > Now Windows 8.1 will correctly report 8.1 > > so if you have any rules that were previously "win8" you will probably want them > updated to "win8.1" as that's what all the bots on the waterfall actually run. Thanks for reminding me! However previously, the branching conditions stopped at '6.2', hence if there was win8.1 before, it would have crashed :-)
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1832423002/ by oysteine@chromium.org. The reason for reverting is: https://bugs.chromium.org/p/chromium/issues/detail?id=598359. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
