Add ability for Bazel >=5.0.0 users to use downloaded py interpreter in py_binary stub script #699
Conversation
…in py_binary stub script. More hermetic
| @@ -154,6 +168,7 @@ py_runtime( | |||
| files = [":files"], | |||
| interpreter = "{python_path}", | |||
| python_version = "PY3", | |||
| {stub_shebang_assignment} | |||
There was a problem hiding this comment.
I think this functionality could be behind a default True configuration knob. I'm not confident it will be the right move in all scenarios.
When doing bazel run (or test) it will work, but if the py_binary is moved in any way, for example to layer it into a container, then the absolute path will not be valid.
With 'in-build' Python toolchains, I'm pretty sure the toolchain interpreter is moved with the rest of the application files. So it can be found at a relative path to the stub_script, for example foo.runfiles/python39_x86_64-apple-darwin/bin/python3.
| # "0.10.0rc1 abc123d" => (0, 10, 0) | ||
| # "0.3.0" => (0, 3, 0) | ||
| def _parse_native_bazel_version(bazel_version): | ||
| return tuple([int(n) for n in bazel_version.split(".")]) |
There was a problem hiding this comment.
I took this from https://github.com/bazelbuild/bazel-skylib/blob/main/lib/versions.bzl#L22 but I don't really want this file to take a dependency on skylib for such simple functionality. 🤷
| print(native.bazel_version) | ||
| if _parse_native_bazel_version(native.bazel_version) >= (5,0,0): | ||
| abs_python_bin_path = rctx.path(python_bin) | ||
| stub_shebang_assignment = "stub_shebang = \"#!{}\"".format(abs_python_bin_path) |
There was a problem hiding this comment.
I have been thinking about this recently, and setting the shebang line to an absolute path that changes from machine to machine will introduce non-deterministic inputs, resulting in lots of cache misses.
| if _parse_native_bazel_version(native.bazel_version) >= (5,0,0): | ||
| abs_python_bin_path = rctx.path(python_bin) | ||
| stub_shebang_assignment = "stub_shebang = \"#!{}\"".format(abs_python_bin_path) |
There was a problem hiding this comment.
| if _parse_native_bazel_version(native.bazel_version) >= (5,0,0): | |
| abs_python_bin_path = rctx.path(python_bin) | |
| stub_shebang_assignment = "stub_shebang = \"#!{}\"".format(abs_python_bin_path) | |
| if _parse_native_bazel_version(native.bazel_version) >= (5,0,0): | |
| execroot = "/".join(str(rctx.path(".")).replace("\\", "/").split("/")[:-2]) | |
| python_bin_path = str(rctx.path(python_bin)).removeprefix(execroot + "/") | |
| shebang_line = "#!./" + python_bin_path | |
| stub_shebang_assignment = "stub_shebang = '{}',".format(shebang_line) |
There was a problem hiding this comment.
This will find the binary relative to the execroot.
There was a problem hiding this comment.
I tried something similar, unfortunately this does not work for me (bazel 5.1.0, rules_python 0.9.0) as the python repository is not part of the linux sandbox dependencies when running the py_binary().
Did you run this successfully?
|
This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. |
github-actions
bot
added
the
Can Close?
|
This PR was automatically closed because it went 30 days without a reply since it was labeled "Can Close?" |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #691
What is the new behavior?
Bazel users on a version greater than or equal to version 5 will avoid using a system Python interpreter when running
py_binaryorpy_testtargets.Does this PR introduce a breaking change?
Other information