fix: handle a system interpreter path on the toolchain (#313)

Handles the path being given to the Python toolchain being a system interpreter. Closes #302 --- ### Type of change - Bug fix (change which fixes an issue) **For changes visible to end-users** - Suggested release notes are provided below: Correctly handle a system interpreter path being set on a Python toolchain. ### Test plan - Covered by existing test cases - Manual testing; please provide instructions so we can reproduce: Manual testing with local interpreter. --------- Co-authored-by: Alex Eagle <[email protected]>
aspect-build · Mar 26, 2024 · 9cc52e0 · 9cc52e0
1 parent f45f1ed
commit 9cc52e0
Show file tree

Hide file tree

Showing 11 changed files with 85 additions and 8 deletions.
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -20,15 +20,16 @@ jobs:
   test:
     uses: bazel-contrib/.github/.github/workflows/bazel.yaml@v6
     with:
-      folders: '[".", "e2e/smoke", "e2e/repository-rule-deps"]'
+      folders: '[".", "e2e/smoke", "e2e/repository-rule-deps", "e2e/system-interpreter"]'
       # TODO: Build Windows tools and add to toolchain
       # TODO(alex): switch the root folder to bzlmod
       exclude: |
         [
           {"os": "windows-latest"},
           {"folder": ".", "bazelversion": "6.4.0"},
           {"folder": ".", "bzlmodEnabled": true},
-          {"folder": "e2e/repository-rule-deps", "bzlmodEnabled": false}
+          {"folder": "e2e/repository-rule-deps", "bzlmodEnabled": false},
+          {"folder": "e2e/system-interpreter", "bzlmodEnabled": false}
         ]
 
   verify-bcr-patches:

diff --git a/e2e/system-interpreter/.bazelrc b/e2e/system-interpreter/.bazelrc
@@ -0,0 +1,2 @@
+# Under Bazel 7 or later, this flag is ignored as the PyRuntimeInfo gives this information.
+common --@aspect_rules_py//py:interpreter_version=3.9.6
diff --git a/e2e/system-interpreter/.bazelversion b/e2e/system-interpreter/.bazelversion
@@ -0,0 +1 @@
+../../.bazelversion
diff --git a/e2e/system-interpreter/BUILD.bazel b/e2e/system-interpreter/BUILD.bazel
@@ -0,0 +1,30 @@
+load("@aspect_rules_py//py:defs.bzl", "py_test")
+load("@rules_python//python:defs.bzl", "py_runtime", "py_runtime_pair")
+
+py_runtime(
+    name = "py3_runtime",
+    interpreter_path = "/usr/bin/python3",
+    interpreter_version_info = {
+        "major": "3",
+        "minor": "9",
+        "micro": "6",
+    },
+    python_version = "PY3",
+)
+
+py_runtime_pair(
+    name = "py_runtime_pair",
+    py2_runtime = None,
+    py3_runtime = ":py3_runtime",
+)
+
+toolchain(
+    name = "default",
+    toolchain = ":py_runtime_pair",
+    toolchain_type = "@bazel_tools//tools/python:toolchain_type",
+)
+
+py_test(
+    name = "bin",
+    srcs = ["__main__.py"],
+)
diff --git a/e2e/system-interpreter/MODULE.bazel b/e2e/system-interpreter/MODULE.bazel
@@ -0,0 +1,25 @@
+"Bazel dependencies"
+bazel_dep(name = "aspect_rules_py", dev_dependency = True, version = "0.0.0")
+
+local_path_override(
+    module_name = "aspect_rules_py",
+    path = "../..",
+)
+
+# Because we use a prerelease of rules_py, we must compile the rust tools from source.
+bazel_dep(name = "rules_rust", version = "0.40.0")
+
+rust = use_extension("@rules_rust//rust:extensions.bzl", "rust")
+rust.toolchain(
+    edition = "2021",
+    versions = ["1.74.1"],
+)
+use_repo(rust, "rust_toolchains")
+register_toolchains("@rust_toolchains//:all")
+
+#---SNIP--- Below here is re-used in the snippet published on releases
+# Minimum version needs:
+# feat: add interpreter_version_info to py_runtime by @mattem in #1671
+bazel_dep(name = "rules_python", dev_dependency = True, version = "0.29.0")
+
+register_toolchains("//:default")
diff --git a/e2e/system-interpreter/README.md b/e2e/system-interpreter/README.md
@@ -0,0 +1,4 @@
+# Test system interperter
+
+This is a minimal test that rules_py can use a toolchain that relies on the system interpreter.
+Note that this is setup to run on Aspect's CI, and locally on a MacOS, paths may vary on your system if running this.
diff --git a/e2e/system-interpreter/__main__.py b/e2e/system-interpreter/__main__.py
@@ -0,0 +1 @@
+assert 1 == 1
diff --git a/py/private/py_binary.bzl b/py/private/py_binary.bzl
@@ -70,7 +70,7 @@ def _py_binary_rule_impl(ctx):
             "{{BASH_RLOCATION_FN}}": BASH_RLOCATION_FUNCTION,
             "{{INTERPRETER_FLAGS}}": " ".join(py_toolchain.flags),
             "{{VENV_TOOL}}": to_rlocation_path(ctx, venv_toolchain.bin),
-            "{{ARG_PYTHON}}": to_rlocation_path(ctx, py_toolchain.python),
+            "{{ARG_PYTHON}}": to_rlocation_path(ctx, py_toolchain.python) if py_toolchain.runfiles_interpreter else py_toolchain.python.path,
             "{{ARG_VENV_NAME}}": ".{}.venv".format(ctx.attr.name),
             "{{ARG_VENV_PYTHON_VERSION}}": "{}.{}.{}".format(
                 py_toolchain.interpreter_version_info.major,
@@ -83,6 +83,7 @@ def _py_binary_rule_impl(ctx):
             "{{EXEC_PYTHON_BIN}}": "python{}".format(
                 py_toolchain.interpreter_version_info.major,
             ),
+            "{{RUNFILES_INTERPRETER}}": str(py_toolchain.runfiles_interpreter).lower(),
         },
         is_executable = True,
     )

diff --git a/py/private/py_semantics.bzl b/py/private/py_semantics.bzl
@@ -69,7 +69,7 @@ def _resolve_toolchain(ctx):
     py3_toolchain = toolchain_info.py3_runtime
 
     interpreter = None
-    uses_interpreter_path = False
+    runfiles_interpreter = True
 
     if py3_toolchain.interpreter != None:
         files = depset([py3_toolchain.interpreter], transitive = [py3_toolchain.files])
@@ -81,7 +81,7 @@ def _resolve_toolchain(ctx):
             short_path = py3_toolchain.interpreter_path,
         )
         files = depset([])
-        uses_interpreter_path = True
+        runfiles_interpreter = False
 
     # Bazel 7 has this field on the PyRuntimeInfo
     if hasattr(py3_toolchain, "interpreter_version_info"):
@@ -107,7 +107,7 @@ def _resolve_toolchain(ctx):
         files = files,
         python = interpreter,
         interpreter_version_info = interpreter_version_info,
-        uses_interpreter_path = uses_interpreter_path,
+        runfiles_interpreter = runfiles_interpreter,
         flags = _INTERPRETER_FLAGS,
     )
 

diff --git a/py/private/py_venv.bzl b/py/private/py_venv.bzl
@@ -44,7 +44,7 @@ def _py_venv_rule_imp(ctx):
             "{{BASH_RLOCATION_FN}}": BASH_RLOCATION_FUNCTION,
             "{{INTERPRETER_FLAGS}}": " ".join(py_toolchain.flags),
             "{{VENV_TOOL}}": to_rlocation_path(ctx, venv_toolchain.bin),
-            "{{ARG_PYTHON}}": to_rlocation_path(ctx, py_toolchain.python),
+            "{{ARG_PYTHON}}": to_rlocation_path(ctx, py_toolchain.python) if py_toolchain.runfiles_interpreter else py_toolchain.python.path,
             "{{ARG_VENV_LOCATION}}": paths.join(ctx.attr.location, ctx.attr.venv_name),
             "{{ARG_VENV_PYTHON_VERSION}}": "{}.{}.{}".format(
                 py_toolchain.interpreter_version_info.major,
@@ -55,6 +55,7 @@ def _py_venv_rule_imp(ctx):
             "{{EXEC_PYTHON_BIN}}": "python{}".format(
                 py_toolchain.interpreter_version_info.major,
             ),
+            "{{RUNFILES_INTERPRETER}}": str(py_toolchain.runfiles_interpreter).lower(),
         },
         is_executable = True,
     )

diff --git a/py/private/run.tmpl.sh b/py/private/run.tmpl.sh
@@ -20,13 +20,24 @@ function alocation {
   fi
 }
 
+function python_location {
+  local PYTHON="{{ARG_PYTHON}}"
+  local RUNFILES_INTERPRETER="{{RUNFILES_INTERPRETER}}"
+
+  if [[ "${RUNFILES_INTERPRETER}" == "true" ]]; then
+    echo -n "$(alocation $(rlocation ${PYTHON}))"
+  else
+    echo -n "${PYTHON}"
+  fi
+}
+
 VENV_TOOL="$(rlocation {{VENV_TOOL}})"
 VIRTUAL_ENV="$(alocation "${RUNFILES_DIR}/{{ARG_VENV_NAME}}")"
 export VIRTUAL_ENV
 
 "${VENV_TOOL}" \
     --location "${VIRTUAL_ENV}" \
-    --python "$(alocation $(rlocation {{ARG_PYTHON}}))" \
+    --python "$(python_location)" \
     --python-version "{{ARG_VENV_PYTHON_VERSION}}" \
     --pth-file "$(rlocation {{ARG_PTH_FILE}})"