Repository: bazelbuild/bazel
Issue: Ensure Python tests run under Windows
Labels: P1
type: process
team-Rules-Python

We [currently disable](https://github.com/bazelbuild/bazel/blob/bdc6c10b674757df38982e71752aab8eddab47a0/src/test/shell/bazel/BUILD#L724-L725) some Python tests under Windows because until recently our CI machines didn't have both Python 2 and Python 3 available. Now that that's resolved, we should enable them.

P1 because this blocks fixing #7947, which in turn is needed to enable toolchains on Windows.

Separate from #6868, which concerns adding or OSSing test cases.

Issue comments:

Pull request review comments:
Can the input `bash_binary` ever be just `bash.exe` for example, which would mean "whatever is on the PATH"?
I think there are two ways to tell which shell binary to use, `BAZEL_SH` or `--shell_executable`. Both way requires user to set an absolute path. The only case `bash_binary` is a relative path (runfile path) is that when using shell toolchain (similar to python toolchain), but I'm not sure it's implemented.
But here, `python_binary` will just be `python` if nothing is specified.. I need to fix this.
Thanks, let me know when the PR is ready for another review.
Thanks for catching the problem! Please take a look again. ;)
Just checking: `bash_binary` can either be an absolute Bash path (from `BAZEL_SH` or `--shell_executable`), or (theoretically perhaps) a bash toolchain's runfiles-relative path. Correct?

And it always ends with "bash" or "bash.exe" (so just "bash" without the extension), right?

Any other option I'm missing?
What other values could `python_binary` have, and in what situations?
Everything you said I believe is true, I don't think there's any other option.
Similar to shell, we have

1. `--python_path`, gives an absolute path
2. `--python_top`, gives an runfile path
3. python toolchain (`py_runtime`), gives a runfile path
4. Default if nothing specified: `python`

@brandjon Can confirm this.
Thanks for confirming that!
Could you please add a comment about this?
Thanks! Waiting for @brandjon to confirm before LGTM. Please add a comment in the code summarizing Jon's answer.
Will do that!
Oh, just realized I had a typo. It should be `GetBinaryPathWithoutExtension(bash_binary) != L"bash")`, `GetBinaryPathWithoutExtension` just strips out the ending `.exe` if there is any. I wanted to check if bash_binary is already `bash` or `bash.exe`. Similar for the python case.
Will add comment for this!
Both `--python_top` and Python toolchains select a `py_runtime` to use. `py_runtime` provides a runfiles path if its `interpreter` attribute is set, and an absolute path if its `interpreter_path` attribute is set (these attributes are exclusive).

`--python_path` is validated to be an absolute path. If nothing is specified, the `python` default is passed along the same code path that `--python_path` uses but skips this validation check as a special case. It is intended that both of these approaches are deprecated and replaced by toolchains, with the `python` PATH lookup behavior provided by an autodetecting toolchain.
Thanks for the confirmation, I'll add those to comment
Rewrite comment as follows:

> There are three kinds of values for `python_binary`:
> 1. An absolute path to a system interpreter. This is the case if `--python_path` is set by the user, or if a `py_runtime` is used that has `interpreter_path` set.
> 2. A runfile path to an in-workspace interpreter. This is the case if a `py_runtime` is used that has `interpreter` set.
> 3. The special constant, "python". This is the default case if neither of the above apply.
> Rlocation resolves runfiles paths to absolute paths, and if given an absolute path it leaves it alone, so it's suitable for cases 1 and 2.

There's a slight detail that the default toolchain's py_runtime on windows contains a sentinel value hack that tells bazel to use legacy --python_path, but I don't think we need to mention that in this comment.
Thanks for help with the comment!

Patch that resolved the issue:
diff --git a/src/tools/launcher/bash_launcher.cc b/src/tools/launcher/bash_launcher.cc
index 0020d5f92d55d3..67f0a5d53a2b51 100644
--- a/src/tools/launcher/bash_launcher.cc
+++ b/src/tools/launcher/bash_launcher.cc
@@ -30,6 +30,14 @@ static constexpr const char* BASH_BIN_PATH = "bash_bin_path";
 
 ExitCode BashBinaryLauncher::Launch() {
   wstring bash_binary = this->GetLaunchInfoByKey(BASH_BIN_PATH);
+
+  // If bash_binary is already "bash" or "bash.exe", that means we want to
+  // rely on the shell binary in PATH, no need to do Rlocation.
+  if (GetBinaryPathWithoutExtension(bash_binary) != L"bash") {
+    // Rlocation returns the original path if bash_binary is an absolute path.
+    bash_binary = this->Rlocation(bash_binary, true);
+  }
+
   if (DoesFilePathExist(bash_binary.c_str())) {
     wstring bash_bin_dir = GetParentDirFromPath(bash_binary);
     wstring path_env;
diff --git a/src/tools/launcher/python_launcher.cc b/src/tools/launcher/python_launcher.cc
index c4d2d5c04670fd..be655cf1c4eeb2 100644
--- a/src/tools/launcher/python_launcher.cc
+++ b/src/tools/launcher/python_launcher.cc
@@ -30,6 +30,20 @@ static constexpr const char* WINDOWS_STYLE_ESCAPE_JVM_FLAGS = "escape_args";
 
 ExitCode PythonBinaryLauncher::Launch() {
   wstring python_binary = this->GetLaunchInfoByKey(PYTHON_BIN_PATH);
+
+  // There are three kinds of values for `python_binary`:
+  // 1. An absolute path to a system interpreter. This is the case if `--python_path` is set by the
+  //    user, or if a `py_runtime` is used that has `interpreter_path` set.
+  // 2. A runfile path to an in-workspace interpreter. This is the case if a `py_runtime` is used
+  //    that has `interpreter` set.
+  // 3. The special constant, "python". This is the default case if neither of the above apply.
+  // Rlocation resolves runfiles paths to absolute paths, and if given an absolute path it leaves
+  // it alone, so it's suitable for cases 1 and 2.
+  if (GetBinaryPathWithoutExtension(python_binary) != L"python") {
+    // Rlocation returns the original path if python_binary is an absolute path.
+    python_binary = this->Rlocation(python_binary, true);
+  }
+
   // If specified python binary path doesn't exist, then fall back to
   // python.exe and hope it's in PATH.
   if (!DoesFilePathExist(python_binary.c_str())) {


Test patch: