Possible bug with Windows 10 SDK headers

[dav...@google.com]

I'm getting reports of, and I am able to repro, some compile failures with bazel 0.13.0 on Windows 10, due to the system include paths not including the location of Windows.h and related headers.

After a bunch of digging, I have a hypothesis of what's wrong. However, I can't reproduce the errors the same way the initial reporter described it, and it's even a bit inconsistent on my machine. Seurat builds correctly for me, but a simple test project fails. I believe it's related to this recent change to Bazel:

"Windows: Expose setup_vc_env_vars function from windows_cc_configure.bzl"
https://github.com/bazelbuild/bazel/commit/a3dd7775d2dbf7e4368712626e11c15be7a23efd

Backing out part of this change in my (unpacked) Bazel install fixes the issue, at any rate. The commit listed above adds WINDOWSSDKDIR to the set of environment variables to be cleared when running gen_env.bat (line 203):

- {"PATH": "", "INCLUDE": "", "LIB": ""})
+ {"PATH": "", "INCLUDE": "", "LIB": "", "WINDOWSSDKDIR": ""})

If I remove WINDOWSSDKDIR from the set, everything works. I've logged the environment variables returned from get_env.bat and this corrects the value of the INCLUDE variable.

When get_env.bat runs vcvarsall.bat without WINDOWSSDKDIR , vcvarsamd64.bat then skips adding includes for the "@rem Set Windows SDK include/lib path" section.

I think this is the primary error, but there are a few related factors. It seems like bazel 0.13.0 patches over this problem if the Windows 8.1 Windows SDK installed, because the "vc_14_0_x64" CROSSTOOL cxx_builtin_include_dirs variables always includes the path to this folder:

https://github.com/bazelbuild/bazel/blob/6f1a2ecbc05a3d8f7864de8ab085e6c6ec5b9f1a/tools/cpp/CROSSTOOL#L577
cxx_builtin_include_directory: "C:/Program Files (x86)/Windows Kits/8.1/include/"

I cannot get it to reproduce on my copy of Seurat, but strangely I can get a repro on a simple Bazel test project. It's set up like this:

d:\Work\BazelTest\WORKSPACE:
workspace(name = "win_test")

d:\Work\BazelTest\BUILD:
cc_binary(
name = "win_test",
srcs=["main.cc"],
)

d:\Work\BazelTest\main.cc:
#include <iostream>
#include <windows.h>

int main()
{
std::cout << "Hello" << std::endl;
return 0;
}

d:\Work\BazelTest>d:\Bazel\bazel-0.13.0-windows-x86_64.exe build -c opt --subcommands :win_test

[László Csomor]

+Yun Peng 

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

--
You received this message because you are subscribed to the Google Groups "bazel-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-discus...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-discuss/89f3ad01-fee2-410c-aeb7-dd26102c3902%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Yun Peng]

Hi davejr,

Sorry, I couldn't reproduce the problem you described. As far as I know, vcvarsall.bat script should set up WINDOWSSDKDIR by itself. See the commands I ran below:

C:\Users\pcloudy>echo %WINDOWSSDKDIR%

%WINDOWSSDKDIR%

C:\Users\pcloudy>"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat" amd64

C:\Users\pcloudy>echo %WINDOWSSDKDIR%

C:\Program Files (x86)\Windows Kits\10\

https://github.com/bazelbuild/bazel/blob/6f1a2ecbc05a3d8f7864de8ab085e6c6ec5b9f1a/tools/cpp/CROSSTOOL#L577 is just an example of the CROSSTOOL, which is not the one used by default by Bazel. We are using the CROSSTOOL.tpl as the template and fill in it with detected toolchain info on your machine. So that's not the reason its working. So we don't have any special patch for Windows 8.1 Windows SDK.

Can you provide more information about which version of Visual Studio or VC++ Build tool you installed. And can you also try to run vcvarsall.bat, see what happens?

[Yun Peng]

Hi Dave, thank you so much for such detailed bug report! I confirmed this is a valid bug that we didn't know before. 

I filed https://github.com/bazelbuild/bazel/issues/5285 to track this bug.

On Sat, May 26, 2018 at 1:14 AM Dave Richardson <dav...@google.com> wrote:

I believe I have a root cause. 

TL;DR: The correct name of the variable is WindowsSdkDir, and using WINDOWSSDKDIR breaks #include of Windows.h. There seems to be a case-sensitivity issue in the launching of the VC settings detection shell scripts, however.

AFAICT there is a case sensitivity issue with the environment used to launch get_env.bat. I think I can say there is also a python or java library bug or design issue with setting up environment variables. It doesn't correctly handle Windows case-insensitivity, or has different semantics than Windows regarding case. Additionally, these differences preclude using the command prompt to directly reproduce this issue. I.e. you must repro through repository_context.execute or com.google.devtools.build.lib.shell.Command.execute.

windows_cc_configure.bzl launches the auto-generated get_env.bat, but with certain variables "cleared" as follows:

* PATH=;c:\Windows\system32

* LIB=""

* INCLUDE=""

* WINDOWSSDKDIR=""

N.B. there is a distinction between deleting the variable, as in with the command set WINDOWSSDKDIR=, and setting it to an empty string, via set  WINDOWSSDKDIR="". This is related to the repro of this bug. The environment in which these are launched apparently has the variables in existence but set to empty strings. I diagnosed this by adding "set W" to the generated get_env.bat and displaying the entire output of the running the get_env.bat subprocess (and turning echo on, and so on). This is the output start of executing get_env.bat. Note two copies of  WINDOWSSDKDIR with different case.

DEBUG: C:/users/davejr/_bazel_davejr/gwypexxy/external/bazel_tools/tools/cpp/windows_cc_configure.bzl:206:3: C:\users\davejr\_bazel_davejr\gwypexxy\external\local_config_cc>set W

WINDOWSSDKDIR=

WindowsLibPath=C:\Program Files (x86)\Windows Kits\10\UnionMetadata;C:\Program Files (x86)\Windows Kits\10\References

WindowsSDKLibVersion=10.0.14393.0\

WindowsSDKVersion=10.0.14393.0\

WindowsSDK_ExecutablePath_x64=C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.6.1 Tools\x64\

WindowsSDK_ExecutablePath_x86=C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.6.1 Tools\

WindowsSdkDir=C:\Program Files (x86)\Windows Kits\10\

windir=C:\Windows

Here's the steps of the bug:

1. get_env.bat calls vcvarsall.bat amd64.

2. vcvarsall calls vcvarsamd64 which calls a VCVarsQueryRegistry.bat.

3. VCVarsQueryRegistry auto-detects a value of WindowsSdkDir from the registry, and uses that to figure out a value for WindowsSdkVersion.

4. Inside function GetWindowsSdkDir (line 41) in VCVarsQueryRegistry, it attempts to clear WindowsSdkDir with set WindowsSdkDir= (line 42).

5. Since there are currently two copies of that variable, one is deleted, and one remains (add set W here to see this).

6. VCVarsQueryRegistry then attempts to delete the WindowsSDKVersion variable (line 43), but only deletes the first one it find.

7. It happens to be the case, on my machine at least, that the existing copy of WindowsSdkDir remains, with the value it had at Bazel launch time.

8. The existing value of  WindowsSdkDir gates subsequent code to autodetect of WindowSdkDir and WindowSDKVersion along with it. (lines 63,64).

9. The resulting auto configured INCLUDE headers paths for Windows 10 header files are missing the SDK version (they resolve to the folder one level above the right folder).

10. Bazel fails to build Seurat, or anything that #includes Windows.h or any of a variety of Windows headers.

This has happened to our users including people working directly with Daydream business partners, as well as me.

I hope this is sufficient information for you to agree on two possible fixes, or a similar alternative that resolves this bug:

1. Use WindowsSdkDir as the variable to clear and retrieve in get_env.bat.

2. Change the com.google.devtools.build.lib.shell.Command.execute environment variable semantics to handle case insensitivity on Windows (or optionally per-invocation).

On Fri, May 25, 2018 at 1:58 AM Yun Peng <pcl...@google.com> wrote:

Hi Dave,

Thanks for the investigation.

I think "C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat" is supposed to set WINDOWSSDKVERSIONt:

C:\Users\pcloudy>set WINDOWSSDKVERSION

Environment variable WINDOWSSDKVERSION not defined

C:\Users\pcloudy>"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat" amd64

C:\Users\pcloudy>set WINDOWSSDKVERSION

WindowsSDKVersion=10.0.14393.0\

Besides, I don't think Bazel could know which Windows SDK version available in advance, so it's impossible to add 10.0.10240.0 in the script.

Sorry I have no idea why it's happening since I couldn't reproduce the problem.

On Wed, May 23, 2018 at 7:18 PM Dave Richardson <dav...@google.com> wrote:

WindowsSdkVersion is not getting set (I echoed it after PATH,INCLUDE,LIB,WINDOWSSDKDIR) and that variable is included as a component in the path. That's why I get the wrong include paths. The vcvarsall usage comments at the end describe how the sdk version is supposed to be provided as a parameter to the script:

:usage

echo Error in script usage. The correct usage is:

echo     %0 [option]

echo   or

echo     %0 [option] store

echo   or

echo     %0 [option] [version number]

echo   or

echo     %0 [option] store [version number]

echo where [option] is: x86 ^| amd64 ^| arm ^| x86_amd64 ^| x86_arm ^| amd64_x86 ^| amd64_arm

echo where [version number] is either the full Windows 10 SDK version number or "8.1" to use the windows 8.1 SDK

echo :

echo The store parameter sets environment variables to support

echo   store (rather than desktop) development.

echo :

echo For example:

echo     %0 x86_amd64

echo     %0 x86_arm store

echo     %0 x86_amd64 10.0.10240.0

echo     %0 x86_arm store 10.0.10240.0

echo     %0 x64 8.1

echo     %0 x64 store 8.1

If I make this modification to windows_cc_configure.bzl, my simple example builds (and I can see the SDK version set correctly):

def setup_vc_env_vars(repository_ctx, vc_path):

  """Get environment variables set by VCVARSALL.BAT. Doesn't %-escape the result!"""

  vcvarsall = _find_vcvarsall_bat_script(repository_ctx, vc_path)

  if not vcvarsall:

    return None

  repository_ctx.file("get_env.bat",

                      "@echo off\n" +

                      "call \"" + vcvarsall + "\" amd64 10.0.10240.0 > NUL \n" +

                      "echo PATH=%PATH%,INCLUDE=%INCLUDE%,LIB=%LIB%,WINDOWSSDKDIR=%WINDOWSSDKDIR%,WINDOWSSDKVERSION=%WINDOWSSDKVERSION% \n", True)

I don't know why it doesn't infer the SDK version.

C:\Users\davejr>set WINDOWSSDKVERSION

Environment variable WINDOWSSDKVERSION not defined

C:\Users\davejr>@call "%VS140COMNTOOLS%VCVarsQueryRegistry.bat" No32bit 64bit

C:\Users\davejr>set WINDOWSSDKVERSION

WindowsSDKVersion=10.0.14393.0\

On Wed, May 23, 2018 at 9:57 AM Dave Richardson <dav...@google.com> wrote:

vcvarsall.bat works in a clean Windows command prompt:

C:\Users\davejr>set WINDOWSSDKDIR

Environment variable WINDOWSSDKDIR not defined

C:\Users\davejr>"c:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat" amd64

C:\Users\davejr>set WINDOWSSDKDIR

WindowsSdkDir=C:\Program Files (x86)\Windows Kits\10\

On Wed, May 23, 2018 at 7:30 AM Dave Richardson <dav...@google.com> wrote:

Hi Yun, thanks for getting back to me. I'm not at my computer yet but I'll get you the info you asked for.

I'm running Windows 10 and VS 2015 SP3.

I've run vcvarsall manually several times while diagnosing this problem. I recall manually clearing those same environment variables and running vcvarsall reproduced the INCLUDE problem. I'll send you the results of that when I make it to my desk.

[Dave Richardson]

You're welcome!

Might this make 0.14?

[Yun Peng]

0.14.0 is going out today and this is not a regression introduced in 0.14.0. So I plan not to block the release. We'll cut 0.15.0 ASAP ;)

[Dave Richardson]

Thanks for letting us know!