Skip to content

Commit

Permalink
[build] Remove python venv_bin filegroup
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jwnimmer-tri committed Dec 28, 2024
1 parent 30baad1 commit a00a497
Show file tree
Hide file tree
Showing 7 changed files with 19 additions and 47 deletions.
5 changes: 1 addition & 4 deletions tools/install/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,7 @@ drake_py_library(
name = "install_test_helper",
testonly = 1,
srcs = ["install_test_helper.py"],
data = [
"//:install",
"@python//:venv_bin",
],
data = ["//:install"],
imports = ["."],
deps = [
"@rules_python//python/runfiles",
Expand Down
11 changes: 2 additions & 9 deletions tools/install/install_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,8 @@ def test_basic_paths(self):
self.assertSetEqual(set(['bin', 'include', 'lib', 'share']), content)

def _run_one_command(self, test_command):
# On macOS, we must run the install tests against venv's interpreter,
# so that necessary libraries are available. On Ubuntu, the libraries
# are installed system-wide (without a venv).
if sys.platform == "darwin":
manifest = runfiles.Create()
python = manifest.Rlocation("python/bin/python3")
else:
python = install_test_helper.get_python_executable()
assert test_command.endswith(".py")
python = install_test_helper.get_python_executable()

# Our launched processes should be independent, not inherit their
# runfiles from the install_test.py runner.
Expand All @@ -47,7 +41,6 @@ def _run_one_command(self, test_command):

# Execute the test_command.
print("+ {}".format(test_command), file=sys.stderr)
assert test_command.endswith(".py")
subprocess.check_call(
[python, os.path.join(os.getcwd(), test_command)],
env=env)
Expand Down
6 changes: 1 addition & 5 deletions tools/jupyter/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,7 @@ drake_jupyter_py_binary(
)

drake_py_unittest(
name = "jupyter_notebook_test",
data = ["@python//:venv_bin"],
deps = [
"@rules_python//python/runfiles",
],
name = "jupyter_prereqs_test",
)

# Add example failing notebook.
Expand Down
18 changes: 0 additions & 18 deletions tools/jupyter/test/jupyter_notebook_test.py

This file was deleted.

14 changes: 14 additions & 0 deletions tools/jupyter/test/jupyter_prereqs_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import unittest

from jupyter_core.command import list_subcommands


class TestJupyterPrereqs(unittest.TestCase):
"""Sanity checks that our install_prereqs was sufficient."""

def test_notebook(self):
"""Ensures that `jupyter notebook` is installed (#12042). That
subcommand is required by _jupyter_bazel_notebook_main when running
interactively (i.e., when in non-test mode).
"""
self.assertIn("notebook", list_subcommands())
9 changes: 0 additions & 9 deletions tools/workspace/python/package.BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,3 @@ cc_library(
)

exports_files(["requirements.txt"])

filegroup(
name = "venv_bin",
visibility = ["//visibility:public"],
data = [
"bin",
"requirements.txt",
],
)
3 changes: 1 addition & 2 deletions tools/workspace/python/repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ dynamic libraries to be used as Python modules.
interpreter as a library into an executable with a C++ main() function.
For part (b) the environment is used in all python rules and tests by default,
but if a test needs to shell out to a venv binary, the `@python//:venv_bin`
can be used to put the binaries' path into runfiles.
because the python toolchain's interpreter is the venv python3 binary.
If the {macos,linux}_interpreter_path being used only mentions the python major
version (i.e., it is "/path/to/python3" not "/path/to/python3.##") and if the
Expand Down

0 comments on commit a00a497

Please sign in to comment.