From a00a49712c611c461f22fd2a6545e842d352a7f9 Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Sat, 28 Dec 2024 08:53:09 -0800 Subject: [PATCH] [build] Remove python venv_bin filegroup 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. --- tools/install/BUILD.bazel | 5 +---- tools/install/install_test.py | 11 ++--------- tools/jupyter/BUILD.bazel | 6 +----- tools/jupyter/test/jupyter_notebook_test.py | 18 ------------------ tools/jupyter/test/jupyter_prereqs_test.py | 14 ++++++++++++++ tools/workspace/python/package.BUILD.bazel | 9 --------- tools/workspace/python/repository.bzl | 3 +-- 7 files changed, 19 insertions(+), 47 deletions(-) delete mode 100644 tools/jupyter/test/jupyter_notebook_test.py create mode 100644 tools/jupyter/test/jupyter_prereqs_test.py diff --git a/tools/install/BUILD.bazel b/tools/install/BUILD.bazel index 913ec149ddba..f5285d7b4ee7 100644 --- a/tools/install/BUILD.bazel +++ b/tools/install/BUILD.bazel @@ -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", diff --git a/tools/install/install_test.py b/tools/install/install_test.py index 15743c8df4f1..684a335b71f0 100644 --- a/tools/install/install_test.py +++ b/tools/install/install_test.py @@ -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. @@ -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) diff --git a/tools/jupyter/BUILD.bazel b/tools/jupyter/BUILD.bazel index 2b4d42782c82..7c9f93e6ca1a 100644 --- a/tools/jupyter/BUILD.bazel +++ b/tools/jupyter/BUILD.bazel @@ -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. diff --git a/tools/jupyter/test/jupyter_notebook_test.py b/tools/jupyter/test/jupyter_notebook_test.py deleted file mode 100644 index d02ca274aa8f..000000000000 --- a/tools/jupyter/test/jupyter_notebook_test.py +++ /dev/null @@ -1,18 +0,0 @@ -import subprocess -import sys -import unittest - -from python import runfiles - - -class TestJupyterNotebook(unittest.TestCase): - def test_help(self): - """Ensures that `jupyter notebook` is installed (#12042).""" - manifest = runfiles.Create() - if sys.platform == "darwin": - jupyter = manifest.Rlocation("python/bin/jupyter") - else: - jupyter = "jupyter" - - status = subprocess.call(args=[jupyter, "notebook", "--help"]) - self.assertEqual(status, 0) diff --git a/tools/jupyter/test/jupyter_prereqs_test.py b/tools/jupyter/test/jupyter_prereqs_test.py new file mode 100644 index 000000000000..c72fb37c2fcd --- /dev/null +++ b/tools/jupyter/test/jupyter_prereqs_test.py @@ -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()) diff --git a/tools/workspace/python/package.BUILD.bazel b/tools/workspace/python/package.BUILD.bazel index ef242811b8dc..568c51e11866 100644 --- a/tools/workspace/python/package.BUILD.bazel +++ b/tools/workspace/python/package.BUILD.bazel @@ -44,12 +44,3 @@ cc_library( ) exports_files(["requirements.txt"]) - -filegroup( - name = "venv_bin", - visibility = ["//visibility:public"], - data = [ - "bin", - "requirements.txt", - ], -) diff --git a/tools/workspace/python/repository.bzl b/tools/workspace/python/repository.bzl index 4c7bbb51a9aa..34e4ee067a9f 100644 --- a/tools/workspace/python/repository.bzl +++ b/tools/workspace/python/repository.bzl @@ -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