Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability for Bazel >=5.0.0 users to use downloaded py interpreter in py_binary stub script #699

Closed
wants to merge 4 commits into from
Closed

Add ability for Bazel >=5.0.0 users to use downloaded py interpreter in py_binary stub script #699

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09e013d
Add ability for Bazel >=5.0.0 users to use downloaded py interpreter …
May 11, 2022
c1fd1d0
Merge branch 'main' into jonathon--stub_template-hermeticity
May 11, 2022
7fecaaa
remove obsolete comment
May 11, 2022
9d4e643
Merge branch 'main' into jonathon--stub_template-hermeticity
May 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions python/repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ def py_repositories():
# Remaining content of the file is only used to support toolchains.
########

# Parse the bazel version string from `native.bazel_version`.
# e.g.
# "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(".")])
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤷


def _python_repository_impl(rctx):
if rctx.attr.distutils and rctx.attr.distutils_content:
fail("Only one of (distutils, distutils_content) should be set.")
Expand Down Expand Up @@ -104,6 +111,12 @@ def _python_repository_impl(rctx):
fail(exec_result.stderr)

python_bin = "python.exe" if ("windows" in platform) else "bin/python3"
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +115 to +117
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will find the binary relative to the execroot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

else:
stub_shebang_assignment = ""

build_content = """\
# Generated by python/repositories.bzl
Expand Down Expand Up @@ -155,6 +168,7 @@ py_runtime(
files = [":files"],
interpreter = "{python_path}",
python_version = "PY3",
{stub_shebang_assignment}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

py_runtime_pair(
Expand All @@ -165,6 +179,7 @@ py_runtime_pair(
""".format(
python_path = python_bin,
python_version = python_short_version,
stub_shebang_assignment = stub_shebang_assignment,
)
rctx.file("BUILD.bazel", build_content)

Expand Down