Update windows VS detection code to account for new directory #18608

redsun82 · 2023-06-07T16:01:29Z

Windows VS 2022 version 17.6 introduced a new vcpkg directory underneath VC that is throwing off the toolchain autodetection code.

The checks now got renamed to a more approriate _is_vs_2017_or_newer and takes into account the possible presence of this vcpkg directory.

Closes #18592

Windows VS 2022 version 17.6 introduced a new `vspkg` directory underneath `VC` that is throwing off the toolchain autodetection code. The checks now got renamed to a more approriate `_is_vs_2017_or_newer` and takes into account the possible presence of this `vspkg` directory.

redsun82 · 2023-06-14T12:00:29Z

Tested this successfully on a recent VS 2022 Community install

Mizux · 2023-06-14T12:27:28Z

Should you also change

bazel/tools/cpp/windows_cc_configure.bzl

Lines 232 to 244 in 75334d1

    
           # 5. Check default directories for VC installation 
        
           auto_configure_warning_maybe(repository_ctx, "Looking for default Visual C++ installation directory") 
        
           for path in [ 
        
               "Microsoft Visual Studio\\2019\\Preview\\VC", 
        
               "Microsoft Visual Studio\\2019\\BuildTools\\VC", 
        
               "Microsoft Visual Studio\\2019\\Community\\VC", 
        
               "Microsoft Visual Studio\\2019\\Professional\\VC", 
        
               "Microsoft Visual Studio\\2019\\Enterprise\\VC", 
        
               "Microsoft Visual Studio\\2017\\BuildTools\\VC", 
        
               "Microsoft Visual Studio\\2017\\Community\\VC", 
        
               "Microsoft Visual Studio\\2017\\Professional\\VC", 
        
               "Microsoft Visual Studio\\2017\\Enterprise\\VC", 
        
               "Microsoft Visual Studio 14.0\\VC",

? (to add all 2022 VS C++ flavors)

ps: I'm using "Microsoft Visual Studio\2022\Preview\VC" (2022 Community preview 17.7.0)

redsun82 · 2023-06-14T14:43:36Z

Good catch @Mizux, but now it is kinda surprising me that even though 2022 is not on that list, it still gets picked up on my my machine 🤔

redsun82 · 2023-06-14T14:46:18Z

Ah, I see now, that is a kind of last resort hard-coded path check, following many other ways of finding out the VC path, which probably usually already work. But yes, then it makes sense to add 2022 to the mix there.

Mizux · 2023-06-15T09:02:00Z

tools/cpp/windows_cc_configure.bzl

        "Microsoft Visual Studio 14.0\\VC",
+        "Microsoft Visual Studio\\2019\\Preview\\VC",


Currently we need 2022\Preview\VC to build google/or-tools ^^;
or you could add it to the below comprehension list ?

Also order may matter imho i.e. prefer "Preview" before "Enterprise" before "Community" etc...
as well as for date 2022 > 2019 > 2017

ref:

CMake: Windows: VS Community 2022 can't build or-tools anymore (cl.exe fatal error C1001) google/or-tools#3807

in previous code the order was "Preview", "BuildTools", "Community", "Professional", "Enterprise". Should I keep that, or reverse it after "Preview" (i.e. "Preview", "Enterprise", "Professional", "Community", "BuildTools")?

Also, I guess adding "Microsoft Visual Studio\\2017\\Preview\\VC" is harmless even though that did not exist AFAIK, right?

(for the moment I preserved the order prior to this patch wrt editions, and added the 2017\Preview one, but that can still be tweaked)

tools/cpp/windows_cc_configure.bzl

redsun82 · 2023-06-16T13:03:08Z

@Mizux

Should you also change

bazel/tools/cpp/windows_cc_configure.bzl

Lines 232 to 244 in 75334d1

# 5. Check default directories for VC installation

auto_configure_warning_maybe(repository_ctx, "Looking for default Visual C++ installation directory")

for path in [

"Microsoft Visual Studio\\2019\\Preview\\VC",

"Microsoft Visual Studio\\2019\\BuildTools\\VC",

"Microsoft Visual Studio\\2019\\Community\\VC",

"Microsoft Visual Studio\\2019\\Professional\\VC",

"Microsoft Visual Studio\\2019\\Enterprise\\VC",

"Microsoft Visual Studio\\2017\\BuildTools\\VC",

"Microsoft Visual Studio\\2017\\Community\\VC",

"Microsoft Visual Studio\\2017\\Professional\\VC",

"Microsoft Visual Studio\\2017\\Enterprise\\VC",

"Microsoft Visual Studio 14.0\\VC",

? (to add all 2022 VS C++ flavors)
ps: I'm using "Microsoft Visual Studio\2022\Preview\VC" (2022 Community preview 17.7.0)

This change makes //src/test/py/bazel:bazel_windows_cpp_test fail for a reason I can't understand (those tests fail on my local machine already on master, so I'm having difficulties debugging that). This draft PR shows that reverting just that change makes the test happy. I'm not sure I can devote a lot of time debugging/fixing that test.

lblk · 2023-06-17T10:03:39Z

@redsun82 can u publish an release binary file for bazel of your branch ? just in you repo's release and name it bazel-vs2022.exe is ok. i'need it for some special work. thank u very much

Vertexwahn · 2023-06-18T12:09:00Z

Can we work around this issue by setting the BAZEL_VC environment variable?

mattip · 2023-06-18T12:31:08Z

Can we work around this issue by setting the BAZEL_VC environment variable?

I did not find one after long hours of debugging. There is no way around the failure other than:

remove the offending extra directory
fix the code to accept the new layout

lblk · 2023-06-20T07:15:27Z

Can we work around this issue by setting the environment variable?BAZEL_VC

this will not work on vs2022

redsun82 · 2023-06-20T07:18:29Z

@redsun82 can u publish an release binary file for bazel of your branch ? just in you repo's release and name it bazel-vs2022.exe is ok. i'need it for some special work. thank u very much

@lblk I'll launch the release build on my machine and let you know

redsun82 · 2023-06-20T07:55:16Z

As already noted, setting BAZEL_VC won't work. Another workaround that I just tested consists in

grabbing the patched tools/cpp directory (you can grab a zip here: https://download-directory.github.io/?url=https%3A%2F%2Fgithub.com%2Fredsun82%2Fbazel%2Ftree%2Ffix-vs-autodetection%2Ftools%2Fcpp)
dump it into your project
go into tools/cpp/cc_configure.bzl and replace all @bazel_tools with nothing
add the following to your WORKSPACE

load("//tools/cpp:cc_configure.bzl", "cc_configure")

cc_configure()

This will replace the built-in autodetection with the patched one.

redsun82 · 2023-06-20T08:03:17Z

@lblk here you go:

https://github.com/redsun82/bazel/releases/tag/vs-2022-fix-0.1

redsun82 · 2023-06-20T08:10:16Z

@Mizux what do you think about the change to the hardcoded VS paths that makes that one test fail? As I mentioned I cannot unfortunately devote much time to that currently, so either someone could take over trying to figure out that test failure, or for the moment we revert that change to quickly get something out that still works most of the time? Before this issue I don't think anyone has yet complained about VS 2022 detection not working, even though it's missing from that hard-coded list.

lblk · 2023-06-20T09:52:56Z

@lblk here you go:

https://github.com/redsun82/bazel/releases/tag/vs-2022-fix-0.1

thank you very much! so kind of u!

meteorcloudy

Thanks for fixing it, sorry for the late reply!

meteorcloudy · 2023-06-28T11:20:49Z

Let's me check if I can help with the test failure

meteorcloudy · 2023-06-28T12:21:42Z

tools/cpp/windows_cc_configure.bzl

    vc_path_contents = [d.basename.lower() for d in repository_ctx.path(vc_path).readdir()]
-    vc_path_contents = sorted(vc_path_contents)
-    return vc_path_contents == vc_2017_or_2019_contents
+    vc_path_contents = sorted([d for d in vc_path_contents if d in vc_2017_or_newer_contents])


I guess we can simplify the logic here:

vc_2017_or_newer_expected_dirs = ["auxiliary", "redist", "tools"] for dir in vc_2017_or_newer_expected_dirs: if not repository_ctx.path(vc_path).get_child(dir).exists: return False return True

From a comment above, VS2015 has this directory structure

C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO 14.0\VC ├───bin ├───crt ├───include ├───lib └───redist

So I think anything that checks for tools and auxiliary will work to differentiate VS2017+ from VS2015. The check for tools is the important one, since that is where the binaries are found.

Thanks, then this could be further simplified to

return repository_ctx.path(vc_path).get_child("tools").exists

@redsun82 Can you overtake the suggested change - would be nice if we could soon merge this in bring it to Bazel 6.3.0

I can fork this and add @meteorcloudy suggestion if @redsun82 does not have time/resources to work on this further...

I create a PR with this change -> #18945 @meteorcloudy, @redsun82

meteorcloudy · 2023-06-28T12:24:39Z

Maybe we can also remove support for VS2015 and prior to simplify the code?

meteorcloudy · 2023-06-28T12:25:51Z

@bazel-io fork 6.3.0

mattip · 2023-06-28T12:39:36Z

Maybe we can also remove support for VS2015 and prior to simplify the code?

That kind of change is beyond my paygrade, but I think it might be too disruptive on the 5.4 branch. Maybe there are some people using really old VS versions?

meteorcloudy · 2023-06-28T12:42:59Z

Yeah, that's a breaking change, we probably shouldn't do that if we plan to cherry pick the fix back to 6.x branches.

meteorcloudy · 2023-06-28T13:07:47Z

I think the test failure is a bug with the version of VC 2022 build tools we have on CI, I cannot reproduce with the latest VC 2022 build tool locally. We should update the VC 2022 version in the Windows VM.

In the mean time I have a workaround to use older VC version in integration tests to unblock this.

To work around [test failure](https://buildkite.com/bazel/bazel-bazel-github-presubmit/builds/16353#018901e5-b0f1-41be-a6f4-e12878711ad3) on #18608. This PR enables Bazel to detect the latest VC build tools installed, but apparently there is a test case which is failing with the new toolchain. Setting --test_env=BAZEL_VC ensures the integrations tests also uses the older VC build tools for now. PiperOrigin-RevId: 544037250 Change-Id: I4625da17ff2168acbe63813aa7a01e49e0cb459a

SuperKogito · 2023-06-28T16:33:50Z

When it comes to building Tensorflow from source with VC 2022 here is the result of my debugging:

Although, Bazel does not account for VS2022 yet but the if condition for VS2017 and VS2019 can still do the trick.
The difference between the old version and the new version is the ../Hostx64/.. label. This is lowercase in your case for example and mine too (VS2022). However, the windows_cc_configure.bz script: line 325 looks for an uppercase ../HostX64/.. label.

The solution here is either to edit ../Hostx64/.. label (https://github.com/bazelbuild/bazel/blob/9c96529fca4a135c162e35ce2882834b879438fb/tools/cpp/windows_cc_configure.bzl#L325C1-L326C1) and change the lines to

if _is_vs_2017_or_2019(repository_ctx, vc_path):
    lib = msvc_vars_x64["%{msvc_env_lib_x64}"]
    full_version = _get_vc_full_version(repository_ctx, vc_path)
    tools_path = "%s\\Tools\\MSVC\\%s\\bin\\Hostx64\\%s" % (vc_path, full_version, target_arch)

    # For native windows(10) on arm64 builds host toolchain runs in an emulated x86 environment
    if not repository_ctx.path(tools_path).exists:
        tools_path = "%s\\Tools\\MSVC\\%s\\bin\\Hostx86\\%s" % (vc_path, full_version, target_arch)

or you rename \bin\Hostx64\ to \bin\HostX64\

also it is vital that these environment variables point to the right paths:

BAZEL_SH=C:\msys64\usr\bin\bash.exe
BAZEL_VC=C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC
BAZEL_VC_FULL_VERSION=14.36.32532
This was tested with:
Windows 10
Bazel 6.2.1
Python 3.9.7
Tensorflow 2.12

Hopefully this helps.

meteorcloudy · 2023-07-17T13:22:27Z

you rename \bin\Hostx64\ to \bin\HostX64\

Is this really necessary? File name is supposed to be case insensitive on Windows.

@meteorcloudy

This is the same PR as #18608 but extended by the modification proposed by @meteorcloudy This should fix #18592 Should also be picked to 6.3.0 -> #18799 Closes #18945. PiperOrigin-RevId: 548725707 Change-Id: Iff0f972c9fc23491c8070ee2b12ec600a3d1ead9

@meteorcloudy

This is the same PR as bazelbuild#18608 but extended by the modification proposed by @meteorcloudy This should fix bazelbuild#18592 Should also be picked to 6.3.0 -> bazelbuild#18799 Closes bazelbuild#18945. PiperOrigin-RevId: 548725707 Change-Id: Iff0f972c9fc23491c8070ee2b12ec600a3d1ead9

To work around [test failure](https://buildkite.com/bazel/bazel-bazel-github-presubmit/builds/16353#018901e5-b0f1-41be-a6f4-e12878711ad3) on bazelbuild#18608. This PR enables Bazel to detect the latest VC build tools installed, but apparently there is a test case which is failing with the new toolchain. Setting --test_env=BAZEL_VC ensures the integrations tests also uses the older VC build tools for now. PiperOrigin-RevId: 544037250 Change-Id: I4625da17ff2168acbe63813aa7a01e49e0cb459a

SuperKogito · 2023-07-19T07:26:34Z

Is this really necessary? File name is supposed to be case insensitive on Windows.

Idk, as I opted for editing the path in the code to avoid messing any other dependencies.

@meteorcloudy

This is the same PR as #18608 but extended by the modification proposed by @meteorcloudy This should fix #18592 Should also be picked to 6.3.0 -> #18799 Closes #18945. PiperOrigin-RevId: 548725707 Change-Id: Iff0f972c9fc23491c8070ee2b12ec600a3d1ead9 Co-authored-by: Vertexwahn <julian.amann@tum.de>

meteorcloudy · 2023-07-19T08:16:09Z

#18945 has been merged and cherry picked back to 6.3.0, so we can close this one.

redsun82 mentioned this pull request Jun 7, 2023

Bazel C/C++ toolchain autodetection fails with newest version of Visual Studio on Windows (17.6.2) due to new directory layout #18592

Closed

redsun82 force-pushed the fix-vs-autodetection branch from 2f4ed3b to 75334d1 Compare June 13, 2023 08:09

redsun82 marked this pull request as ready for review June 14, 2023 11:59

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Rules-CPP Issues for C++ rules labels Jun 14, 2023

Update windows VS detection code hard-coded paths with the 2022 version

0da8c07

redsun82 closed this Jun 15, 2023

redsun82 reopened this Jun 15, 2023

Merge branch 'bazelbuild:master' into fix-vs-autodetection

298bbce

Mizux reviewed Jun 15, 2023

View reviewed changes

Reorder VS hardcoded paths for autodetection and add 2022 Preview

de58f41

mattip reviewed Jun 15, 2023

View reviewed changes

tools/cpp/windows_cc_configure.bzl Show resolved Hide resolved

make VS version check more future-proof

a010879

redsun82 mentioned this pull request Jun 16, 2023

Fix vs autodetection test (TEST PR for CI) #18705

Closed

meteorcloudy approved these changes Jun 28, 2023

View reviewed changes

meteorcloudy mentioned this pull request Jun 28, 2023

Visual Studio 2019 should be preferred over 2017 #10772

Closed

Merge branch 'master' into fix-vs-autodetection

9afb0ab

meteorcloudy requested changes Jun 28, 2023

View reviewed changes

bazel-io mentioned this pull request Jun 28, 2023

[6.3.0] Update windows VS detection code to account for new directory #18799

Closed

Merge branch 'master' into fix-vs-autodetection

e3a9246

meteorcloudy requested a review from oquenchil June 29, 2023 08:42

mattip mentioned this pull request Jul 2, 2023

Track upstream fix for Visual Studio 2022+ conda-forge/bazel-feedstock#196

Closed

1 task

oquenchil self-assigned this Jul 3, 2023

This was referenced Jul 9, 2023

chore(deps): update dependency bazel to v6.3.0 zaucy/rules_vulkan#18

Merged

Fix vs autodetection #18945

Closed

iancha1992 mentioned this pull request Jul 17, 2023

[6.3.0] Fix VS 2022 autodetection #18960

Merged

meteorcloudy closed this Jul 19, 2023

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jul 19, 2023

		"Microsoft Visual Studio 14.0\\VC",
		"Microsoft Visual Studio\\2019\\Preview\\VC",

Update windows VS detection code to account for new directory #18608

Update windows VS detection code to account for new directory #18608

Conversation

redsun82 commented Jun 7, 2023 • edited Loading

redsun82 commented Jun 14, 2023

Mizux commented Jun 14, 2023 • edited Loading

redsun82 commented Jun 14, 2023 • edited Loading

redsun82 commented Jun 14, 2023

Mizux Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

redsun82 Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

redsun82 Jun 15, 2023

Choose a reason for hiding this comment

redsun82 Jun 15, 2023

Choose a reason for hiding this comment

redsun82 commented Jun 16, 2023

lblk commented Jun 17, 2023

Vertexwahn commented Jun 18, 2023

mattip commented Jun 18, 2023

lblk commented Jun 20, 2023

redsun82 commented Jun 20, 2023

redsun82 commented Jun 20, 2023

redsun82 commented Jun 20, 2023

redsun82 commented Jun 20, 2023

lblk commented Jun 20, 2023

meteorcloudy left a comment

Choose a reason for hiding this comment

meteorcloudy commented Jun 28, 2023

meteorcloudy Jun 28, 2023

Choose a reason for hiding this comment

mattip Jun 28, 2023

Choose a reason for hiding this comment

meteorcloudy Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Vertexwahn Jul 5, 2023

Choose a reason for hiding this comment

Vertexwahn Jul 9, 2023

Choose a reason for hiding this comment

Vertexwahn Jul 14, 2023

Choose a reason for hiding this comment

meteorcloudy commented Jun 28, 2023

meteorcloudy commented Jun 28, 2023

mattip commented Jun 28, 2023

meteorcloudy commented Jun 28, 2023

meteorcloudy commented Jun 28, 2023

SuperKogito commented Jun 28, 2023

meteorcloudy commented Jul 17, 2023

SuperKogito commented Jul 19, 2023 • edited Loading

meteorcloudy commented Jul 19, 2023

redsun82 commented Jun 7, 2023 •

edited

Loading

Mizux commented Jun 14, 2023 •

edited

Loading

redsun82 commented Jun 14, 2023 •

edited

Loading

Mizux Jun 15, 2023 •

edited

Loading

redsun82 Jun 15, 2023 •

edited

Loading

meteorcloudy Jun 28, 2023 •

edited

Loading

SuperKogito commented Jul 19, 2023 •

edited

Loading