Windows, launcher: support python/bash toolchain #8440

meteorcloudy · 2019-05-22T12:24:59Z

Previously, we assume the python/bash binary path passed to the launcher is an absolute path specified by --python_path or --shell_executable, but this is not true when using python/shell toolchain. When using the python toolchain, the given python binary path will be a runfile path, we can use Rlocation to find the absolute path.

Fixes #7947

Fixes bazelbuild#7947

laszlocsomor · 2019-05-22T12:34:19Z

src/tools/launcher/bash_launcher.cc

@@ -30,6 +30,10 @@ static constexpr const char* BASH_BIN_PATH = "bash_bin_path";

 ExitCode BashBinaryLauncher::Launch() {
  wstring bash_binary = this->GetLaunchInfoByKey(BASH_BIN_PATH);
+
+  // Rlocation returns the original path if bash_binary is an absolute path.
+  bash_binary = this->Rlocation(bash_binary, true);


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.

meteorcloudy · 2019-05-22T12:47:37Z

src/tools/launcher/python_launcher.cc

@@ -30,6 +30,10 @@ static constexpr const char* WINDOWS_STYLE_ESCAPE_JVM_FLAGS = "escape_args";

 ExitCode PythonBinaryLauncher::Launch() {
  wstring python_binary = this->GetLaunchInfoByKey(PYTHON_BIN_PATH);
+
+  // Rlocation returns the original path if python_binary is an absolute path.
+  python_binary = this->Rlocation(python_binary, true);


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. ;)

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!

src/tools/launcher/bash_launcher.cc

laszlocsomor · 2019-05-22T13:19:21Z

src/tools/launcher/python_launcher.cc

@@ -30,6 +30,12 @@ static constexpr const char* WINDOWS_STYLE_ESCAPE_JVM_FLAGS = "escape_args";

 ExitCode PythonBinaryLauncher::Launch() {
  wstring python_binary = this->GetLaunchInfoByKey(PYTHON_BIN_PATH);
+
+  if (GetBinaryPathWithoutExtension(python_binary) == L"python") {


What other values could python_binary have, and in what situations?

Similar to shell, we have

--python_path, gives an absolute path

--python_top, gives an runfile path

python toolchain (py_runtime), gives a runfile path

Default if nothing specified: python

@brandjon Can confirm this.

Thanks! Waiting for @brandjon to confirm before LGTM. Please add a comment in the code summarizing Jon's answer.

Will do that!

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

brandjon · 2019-05-22T21:49:56Z

I don't understand how this change is supposed to work. How do you rlocation a binary in runfiles if the runfiles are packed in a zip that has yet to be extracted (by the zip file's __main__.py entry point)? We'd need to move the extraction logic into the launcher, no?

I'm not sure how much test coverage for Python we have on windows. Some of the tests I wrote I've disabled because we haven't had both Python 2 and 3 on the test machines until recently. See #8411.

meteorcloudy · 2019-05-23T08:15:40Z

How do you rlocation a binary in runfiles if the runfiles are packed in a zip that has yet to be extracted (by the zip file's main.py entry point)?

We don't rlocation in runfiles, the rlocation function reads the MANIFEST file and maps a runfile path (relative path) to its absolute path. This way doesn't care if there is a runfiles tree or not, so it will also work when people using --build_python_zip=0 and --enable_runfiles.

Some of the tests I wrote I've disabled because we haven't had both Python 2 and 3 on the test machines until recently.

Yes, we definitely need to enable those test.

laszlocsomor · 2019-05-23T08:19:15Z

And I presume the code works with --build_python_zip=True too?

meteorcloudy · 2019-05-23T08:24:29Z

Yes, --build_python_zip=True is by default

laszlocsomor

LGTM, please wait for Jon's LGTM too

brandjon

LGTM but please wait for my fix to #8411 so we have some test coverage.

brandjon · 2019-05-23T15:42:14Z

src/tools/launcher/python_launcher.cc

@@ -30,6 +30,17 @@ static constexpr const char* WINDOWS_STYLE_ESCAPE_JVM_FLAGS = "escape_args";

 ExitCode PythonBinaryLauncher::Launch() {
  wstring python_binary = this->GetLaunchInfoByKey(PYTHON_BIN_PATH);
+
+  // The value of python_binary could be the following cases:


Rewrite comment as follows:

There are three kinds of values for python_binary:

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.

A runfile path to an in-workspace interpreter. This is the case if a py_runtime is used that has interpreter set.

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!

Previously, we assume the python/bash binary path passed to the launcher is an absolute path specified by `--python_path` or `--shell_executable`, but this is not true when using python/shell toolchain. When using the python toolchain, the given python binary path will be a runfile path, we can use `Rlocation` to find the absolute path. Fixes bazelbuild#7947 Closes bazelbuild#8440. PiperOrigin-RevId: 250569171

meteorcloudy requested a review from brandjon May 22, 2019 12:24

googlebot added the cla: yes label May 22, 2019

Windows, launcher: support python/bash toolchain

4e2762f

Fixes bazelbuild#7947

meteorcloudy force-pushed the win_launcher branch from dc91ade to 4e2762f Compare May 22, 2019 12:26

meteorcloudy requested a review from laszlocsomor May 22, 2019 12:26

meteorcloudy mentioned this pull request May 22, 2019

Neither python_top nor python toolchain works with starlark actions on windows #7947

Closed

laszlocsomor reviewed May 22, 2019

View reviewed changes

meteorcloudy commented May 22, 2019

View reviewed changes

meteorcloudy added 2 commits May 22, 2019 14:59

Check if we should do Rlocation

855fa18

Fix compiling error

e1f6681

laszlocsomor suggested changes May 22, 2019

View reviewed changes

meteorcloudy added 2 commits May 22, 2019 16:29

Fix a typo

a584521

Add comments

406661f

Add comments

9b1a75d

laszlocsomor approved these changes May 23, 2019

View reviewed changes

brandjon approved these changes May 23, 2019

View reviewed changes

Update comments

864ed9f

bazel-io closed this in b68d3c1 May 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows, launcher: support python/bash toolchain #8440

Windows, launcher: support python/bash toolchain #8440

meteorcloudy commented May 22, 2019

laszlocsomor May 22, 2019

meteorcloudy May 22, 2019

meteorcloudy May 22, 2019

laszlocsomor May 22, 2019

meteorcloudy May 22, 2019

meteorcloudy May 22, 2019

meteorcloudy May 22, 2019

laszlocsomor May 22, 2019

meteorcloudy May 22, 2019

laszlocsomor May 22, 2019

meteorcloudy May 22, 2019

brandjon May 22, 2019

meteorcloudy May 23, 2019

brandjon commented May 22, 2019

meteorcloudy commented May 23, 2019

laszlocsomor commented May 23, 2019

meteorcloudy commented May 23, 2019

laszlocsomor left a comment

brandjon left a comment

brandjon May 23, 2019

meteorcloudy May 23, 2019

Windows, launcher: support python/bash toolchain #8440

Windows, launcher: support python/bash toolchain #8440

Conversation

meteorcloudy commented May 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandjon commented May 22, 2019

meteorcloudy commented May 23, 2019

laszlocsomor commented May 23, 2019

meteorcloudy commented May 23, 2019

laszlocsomor left a comment

Choose a reason for hiding this comment

brandjon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment