[build] Remove python venv_bin filegroup #22355
Conversation
jwnimmer-tri
added
priority: medium
release notes: none
jwnimmer-tri
force-pushed
the
repo-python-rm-jupyter-pants-test
branch
from
3cb2343 to
ec7896b
Compare
jwnimmer-tri
changed the title
jwnimmer-tri
force-pushed
the
repo-python-rm-jupyter-pants-test
branch
2 times, most recently
from
a00a497 to
95ed8f8
Compare
The filegroup unnecessarily couples our BUILD files to our repository rule, without the py_toolchain as an intermediary. Our goal is to use the toolchain to fully encapsulate the selected python interpreter. Previously the venv_bin had only two citations: (1) Jupyter executable lookup from a jupyter regression test. The test had no need for a subprocess; the production code that it's guarding (jupyter_bazel) imports jupyter from sys.path, it doesn't shell out. We rework the test to better match what it's guarding, and therefore can drop the venv_bin dependency. (2) Python interpreter lookup from installer regression test. The sys.executable used for testing already points to the venv python. The Rlocation lookup was vestigial, from an earlier draft that added the venv libraries with deps instead of with the toolchain.
jwnimmer-tri
force-pushed
the
repo-python-rm-jupyter-pants-test
branch
from
95ed8f8 to
2e2370f
Compare
There was a problem hiding this comment.
+@mwoehlke-kitware for feature review, please.
Reviewed 7 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, needs platform reviewer assigned, needs at least two assigned reviewers
a discussion (no related file):
Working
Once macOS CI is up and running again, we'll need to confirm that this passes on macOS CI before merging.
There was a problem hiding this comment.
Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee mwoehlke-kitware, needs platform reviewer assigned, needs at least two assigned reviewers
tools/workspace/python/repository.bzl line 128 at r2 (raw file):
os_name = repo_ctx.os.name # "linux" or "mac os x" if os_name != "mac os x": execute_or_fail(repo_ctx, ["mkdir", "bin"])
Shouldn't there be a corresponding change in venv_sync?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, needs platform reviewer assigned, needs at least two assigned reviewers
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Once macOS CI is up and running again, we'll need to confirm that this passes on macOS CI before merging.
Done.
tools/workspace/python/repository.bzl line 128 at r2 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Shouldn't there be a corresponding change in
venv_sync?
I don't see any problem. Can you elaborate?
As I understand it, creating the empty bin directory here was only necessary to satisfy the now-deleted glob() in package.BUILD.bazel.
Below, we still return repo_ctx.path("bin/python3") as PYTHON_BIN_PATH.
|
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Ah, right, I forgot about that... |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers
|
+@SeanCurtis-TRI for platform review per schedule, please. |
There was a problem hiding this comment.
Reviewed 6 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),mwoehlke-kitware
Towards #22346 and #20731.
This change is