Set the Windows SDK version selected by vcvarsall.bat to VCPKG_CMAKE_SYSTEM_VERSION.

[thegoodtgg]

According to VCPKG_CMAKE_SYSTEM_VERSION not work. #41723, pass value of VCPKG_CMAKE_SYSTEM_VERSION as target to "vcvarsall.bat"

[BillyONeal]

The documentation for VCPKG_CMAKE_SYSTEM_VERSION is that we pass it to CMake as CMAKE_SYSTEM_VERSION. Can you explain the relationship between CMAKE_SYSTEM_VERSION and vcvarsall.bat that suggests this is the correct outcome?

Thanks for your contribution!

[thegoodtgg]

Yes. I see the desc about VCPKG_CMAKE_SYSTEM_VERSION from documentation.
When using with "ninja" generator, AFAIK，CMAKE_SYSTEM_VERSION make no sense. People who want using a different "Windows SDK Version", have to specify a target explicitly to vcvarsall.bat.
And when using CMake with "Visual Studio" generator, specify CMAKE_SYSTEM_VERSION would definitely change the "Windows SDK Version" the proj using.
To make vcpkg build packages using a different version of "Windows SDK", VCPKG_CMAKE_SYSTEM_VERSION maybe a choice.

[BillyONeal]

I have not analyzed everything this code is doing, but returning "store" in one path and returning a version number in another path is almost certainly wrong.

Moreover, this does not meet the ZStringView requirements because it's returning a pointer that it claims is null terminated with inputs that are not null terminated.

[BillyONeal]

If you want this to select the Windows SDK, I believe you need something like this instead?

auto vcvarsall_options = toolset.vcvarsall_options;
if (!pre_build_info.cmake_system_version.empty()) {
    vcvarsall_options.push_back(fmt::format("-winsdk={}", pre_build_info.cmake_system_version));
}

based on

[thegoodtgg]

I think "vcvarsall.bat" is enough. And I have test it locally.
Using Visual Studio 2022:

Using Visual Studio 2019:

I believe on the old or new version of visual studio, "vcvarsall.bat" can achieve the same function.
Your suggestion about using "Toolset::vcvarsall_options" is more reasonable.

[BillyONeal]

This 'request changes' is for the ZStringView out of contract use. I'm going to confirm with other maintainers if the overall change seems correct but it seems reasonable to me given the documentation page you mentioned.

[thegoodtgg]

Windows SDK Version doesn't belong to msvc toolset. And as positional arguments, "winsdk_version" should be placed after "platform_type". So using ”vcvarsall_options“ maybe not be a good choice.

        const auto arch = to_vcvarsall_toolchain(pre_build_info.target_architecture, toolset, pre_build_info.triplet);
        const auto target = to_vcvarsall_target(pre_build_info.cmake_system_name);
        // New function
        const auto winsdk = to_vcvarsall_winsdk(pre_build_info.cmake_system_version);

        return vcpkg::Command{"cmd"}.string_arg("/d").string_arg("/c").raw_arg(
            fmt::format(R"("{}" {} {} {} {} {} 2>&1 <NUL)",
                        toolset.vcvarsall,
                        Strings::join(" ", toolset.vcvarsall_options),
                        arch,
                        target,
                        winsdk,
                        tonull));

[BillyONeal]

Windows SDK Version doesn't belong to msvc toolset.

Getting it into the environment does belong to vcvarsall.bat though.

And as positional arguments, "winsdk_version" should be placed after "platform_type".

Sure, I apologize for being unclear. I was posting that as a 'for example', the main point being that this needs to go into a -winsdk={} parameter instead of listed as a plain version number on vcvarsall.bat's command line.

There does not appear to be a positional parameter that sets the Windows SDK version.

// New function
const auto winsdk = to_vcvarsall_winsdk(pre_build_info.cmake_system_version);

Sure, separating into a separate function could be fine. I don't think it's required though.

[thegoodtgg]

If we don't check the specified version of windows sdk installed, we could use

const auto winsdk = pre_build_info.cmake_system_version;