mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 64697: Windows: Deleted unused and unnecessary OS version functions.
Date Tue, 19 Dec 2017 21:28:17 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message