incorporate feedback

Author: philsc

python/private/interpreter.bzl

@@ -18,7 +18,6 @@ load("@bazel_skylib//lib:paths.bzl", "paths")
 load("//python:py_runtime_info.bzl", "PyRuntimeInfo")
 load(":sentinel.bzl", "SentinelInfo")
 load(":toolchain_types.bzl", "TARGET_TOOLCHAIN_TYPE")
-load(":py_executable.bzl", "relative_path", "runfiles_root_path")
 
 def _interpreter_binary_impl(ctx):
     if SentinelInfo in ctx.attr.binary:
@@ -31,30 +30,28 @@ def _interpreter_binary_impl(ctx):
     # because of things like pyenv: they use $0 to determine what to
     # re-exec. If it's not a recognized name, then they fail.
     if runtime.interpreter:
-        # Option 1:
-        # Works locally, but not remotely.
-        #executable = ctx.actions.declare_file(runtime.interpreter.basename)
-        #ctx.actions.symlink(output = executable, target_file = runtime.interpreter)
-
-        # Option 2:
-        # Works locally, and remotely, but not on Windows.
+        # In order for this to work both locally and remotely, we create a
+        # shell script here that re-exec's into the real interpreter. Ideally,
+        # we'd just use a symlink, but that breaks under certain conditions. If
+        # we use a ctx.actions.symlink(target=...) then it fails under remote
+        # execution. If we use ctx.actions.symlink(target_path=...) then it
+        # behaves differently inside the runfiles tree and outside the runfiles
+        # tree.
+        #
+        # This currently does not work on Windows. Need to find a way to enable
+        # that.
         executable = ctx.actions.declare_file(runtime.interpreter.basename)
         ctx.actions.expand_template(
             template = ctx.file._template,
             output = executable,
             substitutions = {
+                # Since we never invoke this rule from within the interpreter's
+                # own repository, the short_path here should give us a
+                # predictable path of "../<repo>/<path within repo>".
                 "%target_file%": runtime.interpreter.short_path,
             },
             is_executable = True,
         )
-
-        # Option 3:
-        # Works locally and remotely, but only in runfiles, doesn't work via "bazel run".
-        #executable = ctx.actions.declare_symlink(runtime.interpreter.basename)
-        #ctx.actions.symlink(output = executable, target_path = relative_path(
-        #    from_ = paths.dirname(runfiles_root_path(ctx, executable.short_path)),
-        #    to = runfiles_root_path(ctx, runtime.interpreter.short_path),
-        #))
     else:
         executable = ctx.actions.declare_symlink(paths.basename(runtime.interpreter_path))
         ctx.actions.symlink(output = executable, target_path = runtime.interpreter_path)

python/private/interpreter_tmpl.sh

@@ -14,6 +14,8 @@ source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
 
 readonly TARGET_FILE="%target_file%"
 
+# Strip the leading "../" from "../<repo>/<path within repo>" so that we can do
+# a runfiles lookup.
 MAIN_BIN="$(rlocation "${TARGET_FILE#*/}")"
 
 exec "${MAIN_BIN}" "$@"

python/private/py_executable.bzl

@@ -447,7 +447,7 @@ def _create_executable(
     )
 
 def _create_zip_main(ctx, *, stage2_bootstrap, runtime_details, venv):
-    python_binary = runfiles_root_path(ctx, venv.interpreter.short_path)
+    python_binary = _runfiles_root_path(ctx, venv.interpreter.short_path)
     python_binary_actual = venv.interpreter_actual_path
 
     # The location of this file doesn't really matter. It's added to
@@ -522,7 +522,7 @@ def _create_venv(ctx, output_prefix, imports, runtime_details):
 
     if not venvs_use_declare_symlink_enabled:
         if runtime.interpreter:
-            interpreter_actual_path = runfiles_root_path(ctx, runtime.interpreter.short_path)
+            interpreter_actual_path = _runfiles_root_path(ctx, runtime.interpreter.short_path)
         else:
             interpreter_actual_path = runtime.interpreter_path
 
@@ -543,11 +543,11 @@ def _create_venv(ctx, output_prefix, imports, runtime_details):
         # may choose to write what symlink() points to instead.
         interpreter = ctx.actions.declare_symlink("{}/bin/{}".format(venv, py_exe_basename))
 
-        interpreter_actual_path = runfiles_root_path(ctx, runtime.interpreter.short_path)
+        interpreter_actual_path = _runfiles_root_path(ctx, runtime.interpreter.short_path)
         rel_path = relative_path(
             # dirname is necessary because a relative symlink is relative to
             # the directory the symlink resides within.
-            from_ = paths.dirname(runfiles_root_path(ctx, interpreter.short_path)),
+            from_ = paths.dirname(_runfiles_root_path(ctx, interpreter.short_path)),
             to = interpreter_actual_path,
         )
 
@@ -646,7 +646,7 @@ def _create_stage2_bootstrap(
     )
     return output
 
-def runfiles_root_path(ctx, short_path):
+def _runfiles_root_path(ctx, short_path):
     """Compute a runfiles-root relative path from `File.short_path`
 
     Args:
@@ -676,7 +676,7 @@ def _create_stage1_bootstrap(
     runtime = runtime_details.effective_runtime
 
     if venv:
-        python_binary_path = runfiles_root_path(ctx, venv.interpreter.short_path)
+        python_binary_path = _runfiles_root_path(ctx, venv.interpreter.short_path)
     else:
         python_binary_path = runtime_details.executable_interpreter_path
 

tests/interpreter/BUILD.bazel

@@ -12,47 +12,38 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-load("//tests/support:sh_py_run_test.bzl", "py_reconfig_test")
+load(":interpreter_tests.bzl", "py_reconfig_interpreter_tests", "PYTHON_VERSIONS_TO_TEST")
 
-PYTHON_VERSIONS_TO_TEST = (
-    "3.10",
-    "3.11",
-    "3.12",
-)
-
-[py_reconfig_test(
-    name = "python_version_%s_test" % python_version,
+py_reconfig_interpreter_tests(
+    name = "interpreter_version_test",
+    python_versions = PYTHON_VERSIONS_TO_TEST,
+    # Both the interpreter and the test itself are expected to run under
+    # the same version.
+    expected_interpreter_version = None,
     srcs = ["interpreter_test.py"],
     data = [
         "//python/bin:python",
     ],
     env = {
-        # Both the interpreter and the test itself are expected to run under
-        # the same version.
-        "EXPECTED_INTERPRETER_VERSION": python_version,
-        "EXPECTED_SELF_VERSION": python_version,
         "PYTHON_BIN": "$(rootpath //python/bin:python)",
     },
     main = "interpreter_test.py",
-    python_version = python_version,
-) for python_version in PYTHON_VERSIONS_TO_TEST]
+)
 
-[py_reconfig_test(
-    name = "python_src_%s_test" % python_version,
+py_reconfig_interpreter_tests(
+    name = "python_src_test",
     srcs = ["interpreter_test.py"],
     data = [
         "//python/bin:python",
     ],
     env = {
-        # Since we're grabbing the interpreter from a binary with a fixed
-        # version, we expect to always see that version. It doesn't matter what
-        # Python version the test itself is running with.
-        "EXPECTED_INTERPRETER_VERSION": "3.11",
-        # The test itself is still running under different Python versions.
-        "EXPECTED_SELF_VERSION": python_version,
         "PYTHON_BIN": "$(rootpath //python/bin:python)",
     },
+    python_versions = PYTHON_VERSIONS_TO_TEST,
+    # Since we're grabbing the interpreter from a binary with a fixed
+    # version, we expect to always see that version. It doesn't matter what
+    # Python version the test itself is running with.
+    expected_interpreter_version = "3.11",
     main = "interpreter_test.py",
     python_src = "//tools/publish:twine",
-    python_version = python_version,
-) for python_version in PYTHON_VERSIONS_TO_TEST]
+)