Repository: bazelbuild/bazel
Issue: Switch Python rules to use toolchains
Labels: type: feature request
P1
team-Rules-Python

This bug tracks designing, implementing, and migrating built-in Python rules to use the toolchain mechanism for resolving the Python runtime. This replaces the legacy mechanism of specifying `--python_top` to globally make all Python targets look at a specific `py_runtime`, or omitting `--python_top` to use the system default "python" command.

This is prioritized. Design doc to follow.

Issue comments:
(Plan is to use this work to also address #4815, rather than implement an ad hoc solution to that bug.)
Design doc [here](https://github.com/bazelbuild/rules_python/blob/master/proposals/2019-02-12-design-for-a-python-toolchain.md).

Discussion can proceed in this thread. Initial reviewers are @katre (Bazel, Toolchains), @mrovner (Python), and @nlopezgi (Remote Execution).
Overall, this looks good. A few points:

1. Get rid of the host_python_toolchain, just use the autodetecting version. I don't think the time spent running the autodetection script will really hurt python performance, will it?
1. In the **Backwards Compatbility** section, you mention deleting several flags: I do not think flags can be conditionally deleted based on another flag. Ignoring the values should be sufficient, and they can be deleted in a post-migration cleanup.
1. Ok, I'll remove `host_python_toolchain`. I don't have a strong opinion about its impact on performance, but it's worth remembering that it will still be superseded by any other applicable toolchain that the user registers.

2. By "delete" I really mean "fail-fast if the user tries to specify them while the incompatible flag is enabled". This can be implemented by checking the flag value for null, and logging an error in the fragment's `reportInvalidOptions()` (see [example](https://github.com/bazelbuild/bazel/blob/ea956ae20bac5be1e9917faabdcac3cab2965c12/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java#L99)). The true deletion would happen around the time the old code paths and incompatible flag are removed.
I think the distinction between "delete" and "ignore with a warning" is worth calling out.
(See also Nicolas's comments [here](https://github.com/bazelbuild/rules_python/pull/161#pullrequestreview-202814381).)
Replying to @nlopezgi's concern about toolchains and extensibility here:

> I think you need something similar as there are rules that execute the native py_binary (e.g., rules_docker does here: https://github.com/bazelbuild/rules_docker/blob/master/python/image.bzl#L84) and there might be something special that these type of users need to do to work with the new py toolchains properly.

Users who operate at the level of BUILD targets and macros only need to worry about having definitions for their platforms and python toolchains, i.e. the `py_runtime_pair` target and an associated `toolchain` target referencing it, with appropriate constraints. Whatever the build graph looks like, it should Just Work.

Users who are writing new rules that interoperate with the Python rules may need to manipulate Python toolchains. To receive a Python toolchain, they should declare `@bazel_tools//tools/python:toolchain_type` as a required toolchain type in their rule and receive it with `ctx.toolchains[<same label>]`. This would give them back the `ToolchainInfo` with the schema described in the doc. This would be necessary for instance to write a replica of `py_binary` in Starlark. Conversely, users can write a replacement for `py_runtime` or `py_runtime_pair` by simply returning an appropriate `PyRuntimeInfo` / `ToolchainInfo` provider.

I didn't go into too much detail about how to write rules that produce or consume Python toolchains (aside from a description of the provider schema) because this is already covered by the general toolchain documentation linked in the doc's abstract.
(please do not assign issues to more than one `team-*`)
Let me add my 5 coins as an end user
I would like to see similar naming convention and signatures for toolchain types as is done for all other toolchains
```
py_runtime_pair -> py_toolchain
```
Also related the section
> Why not split Python 2 and Python 3 into two separate toolchain types?
> The naming convention for toolchain types is to literally name the target "toolchain_type",

Based on jdk toolchain implementation it doesn't look like a problem - [tools/jdk/BUILD](https://github.com/bazelbuild/bazel/blob/be9483129401a7c9f1a86a40a3d7770662248576/tools/jdk/BUILD#L25)
Also 
```
@bazel_tools//tools/python2:toolchain_type
@bazel_tools//tools/python3:toolchain_type
```
doesn't break the convention but helps avoids problems with a missed py_runtime attribute in the pair and provide an opportunity to force version as 
```
--python3_toolchain .../python:remote3.7.15 --python2_toolchain ../python:remote2.7.13
```
Also it allows to rid of python_version attribute from `py_binary`/`py_test` rules and select runtime based on provided toolchain type

Hi Guro, thanks for the comments. There's a few reasons I didn't organize it that way.

> py_runtime_pair -> py_toolchain

It's not clear what the ideal way to deal with multiple Python runtimes will be long-term. Currently the rules are organized around this PY2/PY3 distinction, but in the future that will become less important and there may be more emphasis on choosing minor versions. Building the current schema into the rule name gives us a little more room to redesign it in the future without extra breaking changes.

>> Why not split Python 2 and Python 3 into two separate toolchain types?
>
> Based on jdk toolchain implementation it doesn't look like a problem

Again, future-proofing is part of the reason. It's easier to migrate the schema of a `ToolchainInfo` to use some other layout rather than `py2_runtime = ..., py3_runtime = ...`, and even to migrate a toolchain rule to have different attributes, than it is to migrate Python rules to accept different toolchain types.
 
For platforms that do not provide an implementation for one of these toolchains, we'd still have to define a dummy toolchain that fails at analysis time, since even if a target does not use the toolchain its rule type will still require both toolchain types.

Also, creating `.../python[2|3]:toolchain_type` implies the creation of two additional directories and `BUILD` files whose only purpose is to provide naming disambiguation for the toolchain types. I'd sooner just break convention and name them `.../python:py[2|3]_toolchain_type`.

> provide an opportunity to force version

I don't believe there's anything in the toolchain framework that would allow you to directly force the toolchain implementation to use for a given toolchain type, as opposed to just letting Bazel select one based on constraints, though I could be wrong.

> Also it allows to rid of `python_version` attribute from `py_binary`/`py_test` rules and select runtime based on provided toolchain type

I checked with @katre before starting the design doc, and it turns out that currently toolchains can really only differentiate themselves based on platform constraints, not other rule logic constraints. You can't easily express that a given py_binary has a constraint of "I want Python 2".

Looks good to me.
Thanks for addressing my question about extending python rules. It would be ideal to get this info as part of user docs whenever the right time for that comes.

Looks good to me. 
@brandjon is it really not possible to express a version constraint for e.g. a py_binary using toolchains? There is an example here: https://docs.bazel.build/versions/master/platforms.html#defining-constraints-and-platforms talking about defining Version as a constraint. That could not be used?

This is sidetracking a bit but I am also curios about this as I was planning to implement to add a constraint for the nodejs toolchains so it would be possible to use different versions of nodejs in the same workspace and one could choose on nodejs_binary level. 
In that example the platform either supports one version or the other, and targets build/run on whichever platform has the version they require. Here the same platform is generally expected to provide both Python versions. Currently all values for a constraint setting are considered mutually exclusive.

I think the bigger issue is that Python version is a property of the target, not the platform. @katre could comment more.
Ah yeah I see, so I can say a platform is compatible with a certain version, but I can not say that a platform is compatible with multiple versions and then just choose the version on a per-target level. It would be nice to hear more details about how to make such a use-case possible in a generic way from @katre. As I imagine this could come up again and again. I am also happy to pull this in a separate issue or bazel-discuss as this is just tangentially related to this issue.

I also do wonder how `rules_go` manages to allow the setting toolchain attributes on a per-target basis, e.g. I can set `go_binary(os="windows",...)` and then that will compile for windows.
We don't yet have a good story for handling constraints on specific toolchain versions. I know that rules_scala also has problems with this, but I am not sure what the best UX would be (ie, what is the easiest way for users to express the idea that "this target needs nodejs X.Y to build").

I don't think there's an existing issue in Github to track this, so feel free to open one and make any suggestions as to how you think it should be handled.
I'm running into some bootstrapping issues with the new provider definition that I won't have time to address before we cut the baseline for 0.24. So this feature and the associated --incompatible change to preview it will be available in 0.25.
Legacy code cleanup tracked by #8169.
Reopening this because the flag flip failed due to downstream breakages. Follow the migration tracking bug #7899.
This seems to be resolved, but as a casual user of Python and new user of rules_python, it would be helpful to have simple documentation explaining how to do a hermetic build.

(Somehow I am able to run my py_binary targets at the moment even though I know the dependencies in the BUILD.bazel files are inadequate.)

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: