Thanks for the refactor Brad, The patches make sense.
>After your change to add a third component to CMAKE_HOST_SYSTEM_VERSION the >value of CMAKE_SYSTEM_VERSION on a Windows 10 host may have a third component. >Therefore we should check that the version starts with "10.0" rather than is >exactly this version. Actually perhaps we should use >cmSystemTools::VersionCompare to do actual integer version component checks. This was done deliberately to only force a value for CMAKE_WINDOWS_TARGET_PLATFORM_VERSION when the CMAKE_SYSTEM_VERSION is specified as 10.0. Otherwise any Windows 10 machine will use the Windows 10 SDK and not the Windows 8.1 SDK and I'm not sure that there is a way then target the Windows 8.1 SDK. -----Original Message----- From: Brad King [mailto:brad.k...@kitware.com] Sent: Thursday, September 24, 2015 13:35 To: Gilles Khouzam <gilles.khou...@microsoft.com> Cc: cmake-developers@cmake.org Subject: Re: [cmake-developers] [Patch] Adding Windows 10 support On 09/23/2015 06:48 PM, Gilles Khouzam wrote: > This adds only the WINDOWS_TARGET_PLATFORM_VERSION property as it > currently only supports the desktop scenario and is extracted from the > rest of the Windows 10 Store support. Thanks. While reviewing this much simpler patch I realized that the WindowsTargetPlatformVersion is more like PlatformToolset than I previously thought. This led me to another design, perhaps closer to one of your earlier patches, in which the VS 2015+ generators select the WindowsTargetPlatformVersion up front based either on CMAKE_WINDOWS_TARGET_PLATFORM_VERSION or on a computed default. Either way we should set CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION to report the selection to CMakeDetermineCompilerId for use in "CompilerId/VS-10.vcxproj.in". For now I decided to leave out support for per-target WINDOWS_TARGET_PLATFORM_VERSION properties. While testing these changes I found a bug that I've now fixed: cmCoreTryCompile: Fix internal argument vector construction http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=710bde43 Please try out the attached patches based on that version. Then provide fixup patches based on the comments below if needed. > If CMAKE_SYSTEM_VERSION is set to 10.0 then After your change to add a third component to CMAKE_HOST_SYSTEM_VERSION the value of CMAKE_SYSTEM_VERSION on a Windows 10 host may have a third component. Therefore we should check that the version starts with "10.0" rather than is exactly this version. Actually perhaps we should use cmSystemTools::VersionCompare to do actual integer version component checks. > the latest Windows 10 SDK but not more recent than the current build > of Windows. Rather than using VerifyVersionInfo for this, shouldn't the check simply compare the SDK version to the targeted CMAKE_SYSTEM_VERSION? If we are cross-compiling or otherwise specifying a specific target version of Windows then we do not want the SDK to exceed that regardless of what the host is running. Also, the sorting logic in GetWindows10SDKVersion appears to use lexicographic string ordering rather than a component-wise integer comparison. This will not always produce the correct result. This is another candidate for using cmSystemTools::VersionCompare. > There is one thing that I've changed that I want to make sure is the > right thing. As it stands, CMAKE_SYSTEM_VERSION is only valid when > cross-compiling, I've changed the CMakeDetermineSystem.cmake file to > not use the HOST_SYSTEM_VERSION when CMAKE_SYSTEM_VERSION is set. > Otherwise, we can only use this feature through CMAKE_SYSTEM_VERSION > on cross-compiling scenarios. That makes sense. I've split that out into its own commit and explained the motivation in the commit message. See attached patch. > I'm not sure what the best way to test this feature, it can be added > to any desktop project on Windows 10 and it should work properly. I've > tried it with CMake itself and it's working fine and building against > the Win10 SDK. We could have the test suite detect when it is building on a Windows 10 host and then add a test that sets CMAKE_WINDOWS_TARGET_PLATFORM_VERSION and verifies that the value ends up in a generated project file. We already have some tests that parse the generated .sln file to check for specific content. Also just simply by running on a Win 10 host then the entire test suite should build with CMAKE_SYSTEM_VERSION set automatically high enough to enable default SDK selection. Please look at setting up nightly testing submissions to the dashboard from such a host once the changes are integrated. Thanks, -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
