Repository: bazelbuild/bazel
Issue: Windows: Bazel should be able to build non-c++ rules when VC is not installed correctly
Labels: type: feature request
P2
platform: windows

Bazel currently can ignore VC configuration if VC isn't installed at all. But a wrong VC installation could block the configuration even when we don't need to build C++ rules.
See https://github.com/bazelbuild/migration-tooling/issues/85



Issue comments:
/cc @davido 
I agree with this approach but I would also mention that it could be hard to try to detect/ignore all possible corrupted MSVC installations. And in context of another C++ based project: LibreOffice, we generally advice developers to always avoid messing around with multiple MSVC installations on the same Windows machine/VM. And we always tweaking this detection/checking machinery: https://github.com/LibreOffice/core/blob/master/configure.ac#L3176-L3305.

Pull request review comments:
opt: I think it'd be easier to read if you added "installation" after "build tools"
Please don't add `check_exists` here; instead, extract the error handling from this method to its call sites. Let this method just find the script (as its name says) or return None if not found, and let the caller deal with that (by either reporting it as an error or ignoring it).
Here again, please extract the error handling; don't add `check_exists`.
Done, thanks!
Make sense!
Done
It's easy to mistype this string and then this check would always fail. Could you change the code so `vc_path` is None when VC is not found?
Could you change `find_vc_path` to return `None` when the path is not found? See my other comment in `_check_vc_installation` why I prefer that.
Creating the "vc_path_not_found.bat" is a side-effect of this function, and I wouldn't expect this side effect when looking at the call site and the name of the function. Could you remove the side effect (the template expansion) and do it at the call site? This method should (`_check_vc_installation`) really just check things.
Same here: expanding the template is a side-effect, so the method does more than just checking the VC path. Please do this at the call site.
Good advice, I changed the logic, please take a look~
Done.

Patch that resolved the issue:
diff --git a/tools/cpp/vc_path_not_found.bat b/tools/cpp/vc_installation_error.bat.tpl
similarity index 89%
rename from tools/cpp/vc_path_not_found.bat
rename to tools/cpp/vc_installation_error.bat.tpl
index 18e3581f54b852..cb44b1413f77d7 100644
--- a/tools/cpp/vc_path_not_found.bat
+++ b/tools/cpp/vc_installation_error.bat.tpl
@@ -17,7 +17,8 @@
 
 echo. 1>&2
 echo The target you are compiling requires Visual C++ build tools. 1>&2
-echo Bazel couldn't find Visual C++ build tools on your machine. 1>&2
+echo Bazel couldn't find a valid Visual C++ build tools installation on your machine. 1>&2
+%{vc_error_message}
 echo Please check your installation following https://docs.bazel.build/versions/master/windows.html#using 1>&2
 echo. 1>&2
 
diff --git a/tools/cpp/windows_cc_configure.bzl b/tools/cpp/windows_cc_configure.bzl
index 7381c11bb40af9..1a49b2ca1471f4 100644
--- a/tools/cpp/windows_cc_configure.bzl
+++ b/tools/cpp/windows_cc_configure.bzl
@@ -124,7 +124,7 @@ def find_vc_path(repository_ctx):
 
   # 2. Check if VS%VS_VERSION%COMNTOOLS is set, if true then try to find and use
   # vcvarsqueryregistry.bat to detect VC++.
-  auto_configure_warning("Looking for VS%VERSION%COMNTOOLS environment variables," +
+  auto_configure_warning("Looking for VS%VERSION%COMNTOOLS environment variables, " +
                          "eg. VS140COMNTOOLS")
   for vscommontools_env in ["VS140COMNTOOLS", "VS120COMNTOOLS",
                             "VS110COMNTOOLS", "VS100COMNTOOLS", "VS90COMNTOOLS"]:
@@ -163,7 +163,7 @@ def find_vc_path(repository_ctx):
             vc_dir = line[line.find("REG_SZ") + len("REG_SZ"):].strip() + suffix
 
   if not vc_dir:
-    return "visual-studio-not-found"
+    return None
   auto_configure_warning("Visual C++ build tools found at %s" % vc_dir)
   return vc_dir
 
@@ -185,7 +185,8 @@ def _find_vcvarsall_bat_script(repository_ctx, vc_path):
     vcvarsall = vc_path + "\\VCVARSALL.BAT"
 
   if not repository_ctx.path(vcvarsall).exists:
-    auto_configure_fail(vcvarsall + " doesn't exist, please check your VC++ installation")
+    return None
+
   return vcvarsall
 
 
@@ -214,7 +215,7 @@ def find_msvc_tool(repository_ctx, vc_path, tool):
     # C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.10.24930\bin\HostX64\x64
     dirs = repository_ctx.path(vc_path + "\\Tools\\MSVC").readdir()
     if len(dirs) < 1:
-      auto_configure_fail("VC++ build tools directory not found under " + vc_path + "\\Tools\\MSVC")
+      return None
     # Normally there should be only one child directory under %VC_PATH%\TOOLS\MSVC,
     # but iterate every directory to be more robust.
     for path in dirs:
@@ -227,9 +228,22 @@ def find_msvc_tool(repository_ctx, vc_path, tool):
     tool_path = vc_path + "\\bin\\amd64\\" + tool
 
   if not repository_ctx.path(tool_path).exists:
-    auto_configure_fail(tool_path + " not found, please check your VC++ installation.")
+    return None
+
   return tool_path
 
+def _find_missing_vc_tools(repository_ctx, vc_path):
+  """Check if any required tool is missing under given VC path."""
+  missing_tools = []
+  if not _find_vcvarsall_bat_script(repository_ctx, vc_path):
+    missing_tools.append("VCVARSALL.BAT")
+
+  for tool in ["cl.exe", "link.exe", "lib.exe", "ml64.exe"]:
+    if not find_msvc_tool(repository_ctx, vc_path, tool):
+      missing_tools.append(tool)
+
+  return missing_tools
+
 
 def _is_support_whole_archive(repository_ctx, vc_path):
   """Run MSVC linker alone to see if it supports /WHOLEARCHIVE."""
@@ -296,9 +310,23 @@ def configure_windows_toolchain(repository_ctx):
   repository_ctx.symlink(Label("@bazel_tools//tools/cpp:BUILD.static"), "BUILD")
 
   vc_path = find_vc_path(repository_ctx)
-  if vc_path == "visual-studio-not-found":
-    vc_path_error_script = "vc_path_not_found.bat"
-    repository_ctx.symlink(Label("@bazel_tools//tools/cpp:vc_path_not_found.bat"), vc_path_error_script)
+  missing_tools = None
+  vc_installation_error_script = "vc_installation_error.bat"
+  if not vc_path:
+    tpl(repository_ctx, vc_installation_error_script, {"%{vc_error_message}" : ""})
+  else:
+    missing_tools = _find_missing_vc_tools(repository_ctx, vc_path)
+    if missing_tools:
+      tpl(repository_ctx, vc_installation_error_script, {
+        "%{vc_error_message}" : "\r\n".join([
+          "echo. 1>&2",
+          "echo Visual C++ build tools seems to be installed at %s 1>&2" % vc_path,
+          "echo But Bazel can't find the following tools: 1>&2",
+          "echo     %s 1>&2" % ", ".join(missing_tools),
+          "echo. 1>&2",
+      ])})
+
+  if not vc_path or missing_tools:
     tpl(repository_ctx, "CROSSTOOL", {
         "%{cpu}": "x64_windows",
         "%{default_toolchain_name}": "msvc_x64",
@@ -307,14 +335,18 @@ def configure_windows_toolchain(repository_ctx):
         "%{msvc_env_path}": "",
         "%{msvc_env_include}": "",
         "%{msvc_env_lib}": "",
-        "%{msvc_cl_path}": vc_path_error_script,
-        "%{msvc_link_path}": vc_path_error_script,
-        "%{msvc_lib_path}": vc_path_error_script,
+        "%{msvc_cl_path}": vc_installation_error_script,
+        "%{msvc_ml_path}": vc_installation_error_script,
+        "%{msvc_link_path}": vc_installation_error_script,
+        "%{msvc_lib_path}": vc_installation_error_script,
+        "%{dbg_mode_debug}": "/DEBUG",
+        "%{fastbuild_mode_debug}": "/DEBUG",
         "%{compilation_mode_content}": "",
         "%{content}": _get_escaped_windows_msys_crosstool_content(repository_ctx),
         "%{msys_x64_mingw_content}": _get_escaped_windows_msys_crosstool_content(repository_ctx, use_mingw = True),
         "%{opt_content}": "",
         "%{dbg_content}": "",
+        "%{link_content}": "",
         "%{cxx_builtin_include_directory}": "",
         "%{coverage}": "",
     })


Test patch: