Repository: bazelbuild/bazel
Issue: Windows: BAZEL_* envvars should support quotes
Labels: type: feature request
P2
area-Windows
team-OSS

### Description of the problem / feature request:

Allow `BAZEL_VC` and other `BAZEL_*` envvars to have quotes in them.

Quotes are never allows in Windows paths, so the quote cannot be part of the path, so de-quoting is safe.

Quotes appear when using TAB-completion in cmd.exe, as one sets the envvar. A tab-completion sequence typically looks like:

1. `C:\>set BAZEL_VC=C:\Progra<TAB><TAB>`
1. `C:\>set BAZEL_VC="C:\Program Files (x86)"`
1. `C:\>set BAZEL_VC="C:\Program Files (x86)"\Micros<TAB>...<TAB>`
1. `C:\>set BAZEL_VC="C:\Program Files (x86)\Microsoft Visual Studio 14.0"`
1. `C:\>set BAZEL_VC="C:\Program Files (x86)\Microsoft Visual Studio 14.0"\V<TAB>`
1. `C:\>set BAZEL_VC="C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC"<ENTER>`

The shell preserves these quotes so they become part of the envvar value, unless I manually delete them, which is annoying. Bazel could do this itself.

### What operating system are you running Bazel on?

Windows 10

### What's the output of `bazel info release`?

0.25.0

Issue comments:
@meteorcloudy : do you have time to fix this or shall I?
Can you fix this? Thanks!
Will do, thanks!

Pull request review comments:
Can we set default to None here? If so, we don't need enable_warning = False, either.
We can't, because then `_get_path_env_var` fail when SYSTEMROOT is undefined.
But we could just set default to `c:\windows`, let me do that.
Sounds good, but for "BAZEL_VC" and "BAZEL_VS", I guess we can set the default to None? It will make the code a bit cleaner.
Done.
You're right! Done. Much nicer indeed!

Patch that resolved the issue:
diff --git a/tools/cpp/windows_cc_configure.bzl b/tools/cpp/windows_cc_configure.bzl
index 84f755b43ea4bb..35d0d664a173db 100644
--- a/tools/cpp/windows_cc_configure.bzl
+++ b/tools/cpp/windows_cc_configure.bzl
@@ -26,6 +26,36 @@ load(
     "resolve_labels",
 )
 
+def _get_path_env_var(repository_ctx, name):
+    """Returns a path from an environment variable.
+
+    Removes quotes, replaces '/' with '\', and strips trailing '\'s."""
+    if name in repository_ctx.os.environ:
+        value = repository_ctx.os.environ[name]
+        if value[0] == "\"":
+            if len(value) == 1 or value[-1] != "\"":
+                auto_configure_fail("'%s' environment variable has no trailing quote" % name)
+            value = value[1:-1]
+        if "/" in value:
+            value = value.replace("/", "\\")
+        if value[-1] == "\\":
+            value = value.rstrip("\\")
+        return value
+    else:
+        return None
+
+def _get_temp_env(repository_ctx):
+    """Returns the value of TMP, or TEMP, or if both undefined then C:\Windows."""
+    tmp = _get_path_env_var(repository_ctx, "TMP")
+    if not tmp:
+        tmp = _get_path_env_var(repository_ctx, "TEMP")
+    if not tmp:
+        tmp = "C:\\Windows\\Temp"
+        auto_configure_warning(
+            "neither 'TMP' nor 'TEMP' environment variables are set, using '%s' as default" % tmp,
+        )
+    return tmp
+
 def _auto_configure_warning_maybe(repository_ctx, msg):
     """Output warning message when CC_CONFIGURE_DEBUG is enabled."""
     if is_cc_configure_debug(repository_ctx):
@@ -33,13 +63,16 @@ def _auto_configure_warning_maybe(repository_ctx, msg):
 
 def _get_escaped_windows_msys_starlark_content(repository_ctx, use_mingw = False):
     """Return the content of msys cc toolchain rule."""
-    bazel_sh = get_env_var(repository_ctx, "BAZEL_SH", "", False).replace("\\", "/").lower()
-    tokens = bazel_sh.rsplit("/", 1)
     msys_root = ""
-    if tokens[0].endswith("/usr/bin"):
-        msys_root = tokens[0][:len(tokens[0]) - len("usr/bin")]
-    elif tokens[0].endswith("/bin"):
-        msys_root = tokens[0][:len(tokens[0]) - len("bin")]
+    bazel_sh = _get_path_env_var(repository_ctx, "BAZEL_SH")
+    if bazel_sh:
+        bazel_sh = bazel_sh.replace("\\", "/").lower()
+        tokens = bazel_sh.rsplit("/", 1)
+        if tokens[0].endswith("/usr/bin"):
+            msys_root = tokens[0][:len(tokens[0]) - len("usr/bin")]
+        elif tokens[0].endswith("/bin"):
+            msys_root = tokens[0][:len(tokens[0]) - len("bin")]
+
     prefix = "mingw64" if use_mingw else "usr"
     tool_path_prefix = escape_string(msys_root) + prefix
     tool_bin_path = tool_path_prefix + "/bin"
@@ -69,10 +102,14 @@ def _get_escaped_windows_msys_starlark_content(repository_ctx, use_mingw = False
 
 def _get_system_root(repository_ctx):
     """Get System root path on Windows, default is C:\\\Windows. Doesn't %-escape the result."""
-    if "SYSTEMROOT" in repository_ctx.os.environ:
-        return escape_string(repository_ctx.os.environ["SYSTEMROOT"])
-    _auto_configure_warning_maybe(repository_ctx, "SYSTEMROOT is not set, using default SYSTEMROOT=C:\\Windows")
-    return "C:\\Windows"
+    systemroot = _get_path_env_var(repository_ctx, "SYSTEMROOT")
+    if not systemroot:
+        systemroot = "C:\\Windows"
+        _auto_configure_warning_maybe(
+            repository_ctx,
+            "SYSTEMROOT is not set, using default SYSTEMROOT=C:\\Windows",
+        )
+    return escape_string(systemroot)
 
 def _add_system_root(repository_ctx, env):
     """Running VCVARSALL.BAT and VCVARSQUERYREGISTRY.BAT need %SYSTEMROOT%\\\\system32 in PATH."""
@@ -85,13 +122,38 @@ def find_vc_path(repository_ctx):
     """Find Visual C++ build tools install path. Doesn't %-escape the result."""
 
     # 1. Check if BAZEL_VC or BAZEL_VS is already set by user.
-    if "BAZEL_VC" in repository_ctx.os.environ:
-        return repository_ctx.os.environ["BAZEL_VC"]
+    bazel_vc = _get_path_env_var(repository_ctx, "BAZEL_VC")
+    if bazel_vc:
+        if repository_ctx.path(bazel_vc).exists:
+            return bazel_vc
+        else:
+            _auto_configure_warning_maybe(
+                repository_ctx,
+                "%BAZEL_VC% is set to non-existent path, ignoring.",
+            )
 
-    if "BAZEL_VS" in repository_ctx.os.environ:
-        return repository_ctx.os.environ["BAZEL_VS"] + "\\VC\\"
-    _auto_configure_warning_maybe(repository_ctx, "'BAZEL_VC' is not set, " +
-                                                  "start looking for the latest Visual C++ installed.")
+    bazel_vs = _get_path_env_var(repository_ctx, "BAZEL_VS")
+    if bazel_vs:
+        if repository_ctx.path(bazel_vs).exists:
+            bazel_vc = bazel_vs + "\\VC"
+            if repository_ctx.path(bazel_vc).exists:
+                return bazel_vc
+            else:
+                _auto_configure_warning_maybe(
+                    repository_ctx,
+                    "No 'VC' directory found under %BAZEL_VS%, ignoring.",
+                )
+        else:
+            _auto_configure_warning_maybe(
+                repository_ctx,
+                "%BAZEL_VS% is set to non-existent path, ignoring.",
+            )
+
+    _auto_configure_warning_maybe(
+        repository_ctx,
+        "Neither %BAZEL_VC% nor %BAZEL_VS% are set, start looking for the latest Visual C++" +
+        " installed.",
+    )
 
     # 2. Check if VS%VS_VERSION%COMNTOOLS is set, if true then try to find and use
     # vcvarsqueryregistry.bat / VsDevCmd.bat to detect VC++.
@@ -108,7 +170,7 @@ def find_vc_path(repository_ctx):
     ]:
         if vscommontools_env not in repository_ctx.os.environ:
             continue
-        script = repository_ctx.os.environ[vscommontools_env] + "\\" + script
+        script = _get_path_env_var(repository_ctx, vscommontools_env) + "\\" + script
         if not repository_ctx.path(script).exists:
             continue
         repository_ctx.file(
@@ -148,7 +210,13 @@ def find_vc_path(repository_ctx):
 
     # 4. Check default directories for VC installation
     _auto_configure_warning_maybe(repository_ctx, "Looking for default Visual C++ installation directory")
-    program_files_dir = get_env_var(repository_ctx, "PROGRAMFILES(X86)", default = "C:\\Program Files (x86)", enable_warning = True)
+    program_files_dir = _get_path_env_var(repository_ctx, "PROGRAMFILES(X86)")
+    if not program_files_dir:
+        program_files_dir = "C:\\Program Files (x86)"
+        _auto_configure_warning_maybe(
+            repository_ctx,
+            "'PROGRAMFILES(X86)' environment variable is not set, using '%s' as default" % program_files_dir,
+        )
     for path in [
         "Microsoft Visual Studio\\2019\\Preview\\VC",
         "Microsoft Visual Studio\\2019\\BuildTools\\VC",
@@ -266,8 +334,9 @@ def find_llvm_path(repository_ctx):
     """Find LLVM install path."""
 
     # 1. Check if BAZEL_LLVM is already set by user.
-    if "BAZEL_LLVM" in repository_ctx.os.environ:
-        return repository_ctx.os.environ["BAZEL_LLVM"]
+    bazel_llvm = _get_path_env_var(repository_ctx, "BAZEL_LLVM")
+    if bazel_llvm:
+        return bazel_llvm
 
     _auto_configure_warning_maybe(repository_ctx, "'BAZEL_LLVM' is not set, " +
                                                   "start looking for LLVM installation on machine.")
@@ -290,7 +359,13 @@ def find_llvm_path(repository_ctx):
 
     # 3. Check default directories for LLVM installation
     _auto_configure_warning_maybe(repository_ctx, "Looking for default LLVM installation directory")
-    program_files_dir = get_env_var(repository_ctx, "PROGRAMFILES", default = "C:\\Program Files", enable_warning = True)
+    program_files_dir = _get_path_env_var(repository_ctx, "PROGRAMFILES")
+    if not program_files_dir:
+        program_files_dir = "C:\\Program Files"
+        _auto_configure_warning_maybe(
+            repository_ctx,
+            "'PROGRAMFILES' environment variable is not set, using '%s' as default" % program_files_dir,
+        )
     path = program_files_dir + "\\LLVM"
     if repository_ctx.path(path).exists:
         llvm_dir = path
@@ -416,9 +491,7 @@ def configure_windows_toolchain(repository_ctx):
     escaped_paths = escape_string(env["PATH"])
     escaped_include_paths = escape_string(env["INCLUDE"])
     escaped_lib_paths = escape_string(env["LIB"])
-    escaped_tmp_dir = escape_string(
-        get_env_var(repository_ctx, "TMP", "C:\\Windows\\Temp").replace("\\", "\\\\"),
-    )
+    escaped_tmp_dir = escape_string(_get_temp_env(repository_ctx).replace("\\", "\\\\"))
 
     llvm_path = ""
     if _use_clang_cl(repository_ctx):


Test patch: