You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2017/12/19 02:07:01 UTC
Review Request 64697: Windows: Deleted unused and unnecessary OS
version functions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64697/
-----------------------------------------------------------
Review request for mesos, Akash Gupta, Jeff Coffler, and Joseph Wu.
Bugs: MESOS-7781
https://issues.apache.org/jira/browse/MESOS-7781
Repository: mesos
Description
-------
The Windows API `GetVersionEx` was deprecated in Windows 8. Using it in
the `os.hpp` header caused hundreds of warnings that it was deprecated
to be emitted when building. While the function still exists, it stopped
returning the correct value when it was deprecated (so the version it
returns is 6.3, that is, Windows 8).
However, replacing it was less than straightforward. The recommended
replacement is to use the "version helper functions", but these are not
capable of providing the version information of the system. The intent
of the deprecation and the new APIs is to prevent developers from using
the version of Windows as a feature check. The new APIs are all of the
form `bool IsWindows10OrGreater()`. While it is possible to use
`IsWindowsServer()` to determine if we're on a client or server version
of Windows, no current user-mode API is provided to query the build
information of the OS. This is unfortunate, as retrieving this
information is a valid use case of the now deprecated API.
An explored alternative was to query the registry, but this was
discarded as it was not consistently available.
Another alternative was to parse the output of `systeminfo`, which can
return CSV formatted version information. However, we chose not to do
this currently as it is slow (on the order of seconds to invoke the
command).
There still exists a kernel-mode API to retrieve the version
information. However, to use it would entail either including WDK (which
is massive and not something we're going to do), or to invoke it
dynamically by getting the address from `nt.dll`. This is possible, but
it would be hacky, and was not necessary.
The chosen route was to simply delete the use of `GetVersionEx`. After
reviewing the use of these functions, it turned out to be entirely
possible to `delete` them.
Note that this means the entirety of `systems_tests.cpp` was rendered
pointless for Windows.
Diffs
-----
3rdparty/stout/include/stout/os.hpp 1a81db61faa55d7043d75a012fe1e66b49bf601c
3rdparty/stout/include/stout/windows/os.hpp 26a1a049c7a4e1b6a3b6767a9c2c86e7538f6012
3rdparty/stout/tests/CMakeLists.txt 940fc9f6997695cf6c181ecbc72d6fd6e72dc7e7
3rdparty/stout/tests/os/systems_tests.cpp 286d34edacab932aaecacfa6259a0bc549f01b1b
Diff: https://reviews.apache.org/r/64697/diff/1/
Testing
-------
`ctest -V -C Debug` on Windows 10, `make check` on CentOS 7.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 64697: Windows: Deleted unused and unnecessary OS
version functions.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64697/#review194116
-----------------------------------------------------------
PASS: Mesos patch 64697 was successfully built and tested.
Reviews applied: `['64697']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64697
- Mesos Reviewbot Windows
On Dec. 18, 2017, 9:07 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64697/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2017, 9:07 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jeff Coffler, and Joseph Wu.
>
>
> Bugs: MESOS-7781
> https://issues.apache.org/jira/browse/MESOS-7781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Windows API `GetVersionEx` was deprecated in Windows 8. Using it in
> the `os.hpp` header caused hundreds of warnings that it was deprecated
> to be emitted when building. While the function still exists, it stopped
> returning the correct value when it was deprecated (so the version it
> returns is 6.3, that is, Windows 8).
>
> However, replacing it was less than straightforward. The recommended
> replacement is to use the "version helper functions", but these are not
> capable of providing the version information of the system. The intent
> of the deprecation and the new APIs is to prevent developers from using
> the version of Windows as a feature check. The new APIs are all of the
> form `bool IsWindows10OrGreater()`. While it is possible to use
> `IsWindowsServer()` to determine if we're on a client or server version
> of Windows, no current user-mode API is provided to query the build
> information of the OS. This is unfortunate, as retrieving this
> information is a valid use case of the now deprecated API.
>
> An explored alternative was to query the registry, but this was
> discarded as it was not consistently available.
>
> Another alternative was to parse the output of `systeminfo`, which can
> return CSV formatted version information. However, we chose not to do
> this currently as it is slow (on the order of seconds to invoke the
> command).
>
> There still exists a kernel-mode API to retrieve the version
> information. However, to use it would entail either including WDK (which
> is massive and not something we're going to do), or to invoke it
> dynamically by getting the address from `nt.dll`. This is possible, but
> it would be hacky, and was not necessary.
>
> The chosen route was to simply delete the use of `GetVersionEx`. After
> reviewing the use of these functions, it turned out to be entirely
> possible to `delete` them.
>
> Note that this means the entirety of `systems_tests.cpp` was rendered
> pointless for Windows.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os.hpp 1a81db61faa55d7043d75a012fe1e66b49bf601c
> 3rdparty/stout/include/stout/windows/os.hpp 26a1a049c7a4e1b6a3b6767a9c2c86e7538f6012
> 3rdparty/stout/tests/CMakeLists.txt 940fc9f6997695cf6c181ecbc72d6fd6e72dc7e7
> 3rdparty/stout/tests/os/systems_tests.cpp 286d34edacab932aaecacfa6259a0bc549f01b1b
>
>
> Diff: https://reviews.apache.org/r/64697/diff/1/
>
>
> Testing
> -------
>
> `ctest -V -C Debug` on Windows 10, `make check` on CentOS 7.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 64697: Windows: Deleted unused and unnecessary OS
version functions.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64697/#review194181
-----------------------------------------------------------
Ship it!
This is reasonable. The reason why we need these methods on Posix is to differentiate between Darwin and Linux, or check the kernel version. On Windows, we have the `#ifdef __WINDOWS__`, so we shouldn't need runtime OS version checks at all. (And we only support the latest Windows version anyway).
- Joseph Wu
On Dec. 18, 2017, 6:07 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64697/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2017, 6:07 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jeff Coffler, and Joseph Wu.
>
>
> Bugs: MESOS-7781
> https://issues.apache.org/jira/browse/MESOS-7781
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Windows API `GetVersionEx` was deprecated in Windows 8. Using it in
> the `os.hpp` header caused hundreds of warnings that it was deprecated
> to be emitted when building. While the function still exists, it stopped
> returning the correct value when it was deprecated (so the version it
> returns is 6.3, that is, Windows 8).
>
> However, replacing it was less than straightforward. The recommended
> replacement is to use the "version helper functions", but these are not
> capable of providing the version information of the system. The intent
> of the deprecation and the new APIs is to prevent developers from using
> the version of Windows as a feature check. The new APIs are all of the
> form `bool IsWindows10OrGreater()`. While it is possible to use
> `IsWindowsServer()` to determine if we're on a client or server version
> of Windows, no current user-mode API is provided to query the build
> information of the OS. This is unfortunate, as retrieving this
> information is a valid use case of the now deprecated API.
>
> An explored alternative was to query the registry, but this was
> discarded as it was not consistently available.
>
> Another alternative was to parse the output of `systeminfo`, which can
> return CSV formatted version information. However, we chose not to do
> this currently as it is slow (on the order of seconds to invoke the
> command).
>
> There still exists a kernel-mode API to retrieve the version
> information. However, to use it would entail either including WDK (which
> is massive and not something we're going to do), or to invoke it
> dynamically by getting the address from `nt.dll`. This is possible, but
> it would be hacky, and was not necessary.
>
> The chosen route was to simply delete the use of `GetVersionEx`. After
> reviewing the use of these functions, it turned out to be entirely
> possible to `delete` them.
>
> Note that this means the entirety of `systems_tests.cpp` was rendered
> pointless for Windows.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os.hpp 1a81db61faa55d7043d75a012fe1e66b49bf601c
> 3rdparty/stout/include/stout/windows/os.hpp 26a1a049c7a4e1b6a3b6767a9c2c86e7538f6012
> 3rdparty/stout/tests/CMakeLists.txt 940fc9f6997695cf6c181ecbc72d6fd6e72dc7e7
> 3rdparty/stout/tests/os/systems_tests.cpp 286d34edacab932aaecacfa6259a0bc549f01b1b
>
>
> Diff: https://reviews.apache.org/r/64697/diff/1/
>
>
> Testing
> -------
>
> `ctest -V -C Debug` on Windows 10, `make check` on CentOS 7.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>