fix: correctly obtain relative path required for the venv created by …

…`--bootstrap_impl=script` (#2439) Computing the relative path from the venv interpreter to the underlying interpreter was incorrectly using the actual interpreter's directory depth, not the venv interpreter's directory depth, when computing the distance from the venv interpreter to the runfiles root. The net effect is the correct relative path would only be computed for binaries with the same directory depth as the actual interpreter (e.g. 2). This went undetected in CI because the tests for this logic just happen to have the same directory depth as the actual interpreter used. To fix, compute the relative path to the runfiles root using the venv interpreter directory. Also added a test in a more nested directory to test this case. Along the way: * Change relative path computation to compute a minimum relative path. * Fix the internals to pass a runfiles-root relative path, not main-repo relative path, for the actual interpreter, as intended. Fixes #2169 --------- Co-authored-by: Richard Levasseur <[email protected]> Co-authored-by: Richard Levasseur <[email protected]> Co-authored-by: Ignas Anikevicius <[email protected]>
bazelbuild · Nov 25, 2024 · 438b12e · 438b12e
1 parent 4ee7e25
commit 438b12e
Show file tree

Hide file tree

Showing 6 changed files with 191 additions and 11 deletions.
diff --git a/python/private/py_executable_bazel.bzl b/python/private/py_executable_bazel.bzl
@@ -323,7 +323,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_actual = _runfiles_root_path(ctx, venv.interpreter_actual_path)
+    python_binary_actual = venv.interpreter_actual_path
 
     # The location of this file doesn't really matter. It's added to
     # the zip file as the top-level __main__.py file and not included
@@ -344,6 +344,37 @@ def _create_zip_main(ctx, *, stage2_bootstrap, runtime_details, venv):
     )
     return output
 
+def relative_path(from_, to):
+    """Compute a relative path from one path to another.
+
+    Args:
+        from_: {type}`str` the starting directory. Note that it should be
+            a directory because relative-symlinks are relative to the
+            directory the symlink resides in.
+        to: {type}`str` the path that `from_` wants to point to
+
+    Returns:
+        {type}`str` a relative path
+    """
+    from_parts = from_.split("/")
+    to_parts = to.split("/")
+
+    # Strip common leading parts from both paths
+    n = min(len(from_parts), len(to_parts))
+    for _ in range(n):
+        if from_parts[0] == to_parts[0]:
+            from_parts.pop(0)
+            to_parts.pop(0)
+        else:
+            break
+
+    # Impossible to compute a relative path without knowing what ".." is
+    if from_parts and from_parts[0] == "..":
+        fail("cannot compute relative path from '%s' to '%s'", from_, to)
+
+    parts = ([".."] * len(from_parts)) + to_parts
+    return paths.join(*parts)
+
 # Create a venv the executable can use.
 # For venv details and the venv startup process, see:
 # * https://docs.python.org/3/library/venv.html
@@ -368,9 +399,15 @@ def _create_venv(ctx, output_prefix, imports, runtime_details):
         # in runfiles is always a symlink. An RBE implementation, for example,
         # may choose to write what symlink() points to instead.
         interpreter = ctx.actions.declare_symlink("{}/bin/{}".format(venv, py_exe_basename))
-        interpreter_actual_path = runtime.interpreter.short_path
-        parent = "/".join([".."] * (interpreter_actual_path.count("/") + 1))
-        rel_path = parent + "/" + interpreter_actual_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)),
+            to = interpreter_actual_path,
+        )
+
         ctx.actions.symlink(output = interpreter, target_path = rel_path)
     else:
         py_exe_basename = paths.basename(runtime.interpreter_path)
@@ -412,7 +449,7 @@ def _create_venv(ctx, output_prefix, imports, runtime_details):
 
     return struct(
         interpreter = interpreter,
-        # Runfiles-relative path or absolute path
+        # Runfiles root relative path or absolute path
         interpreter_actual_path = interpreter_actual_path,
         files_without_interpreter = [pyvenv_cfg, pth, site_init],
     )
@@ -462,12 +499,22 @@ def _create_stage2_bootstrap(
     )
     return output
 
-def _runfiles_root_path(ctx, path):
-    # The ../ comes from short_path for files in other repos.
-    if path.startswith("../"):
-        return path[3:]
+def _runfiles_root_path(ctx, short_path):
+    """Compute a runfiles-root relative path from `File.short_path`
+
+    Args:
+        ctx: current target ctx
+        short_path: str, a main-repo relative path from `File.short_path`
+
+    Returns:
+        {type}`str`, a runflies-root relative path
+    """
+
+    # The ../ comes from short_path is for files in other repos.
+    if short_path.startswith("../"):
+        return short_path[3:]
     else:
-        return "{}/{}".format(ctx.workspace_name, path)
+        return "{}/{}".format(ctx.workspace_name, short_path)
 
 def _create_stage1_bootstrap(
         ctx,
@@ -487,7 +534,7 @@ def _create_stage1_bootstrap(
         python_binary_path = runtime_details.executable_interpreter_path
 
     if is_for_zip and venv:
-        python_binary_actual = _runfiles_root_path(ctx, venv.interpreter_actual_path)
+        python_binary_actual = venv.interpreter_actual_path
     else:
         python_binary_actual = ""
 

diff --git a/python/private/stage1_bootstrap_template.sh b/python/private/stage1_bootstrap_template.sh
@@ -135,6 +135,7 @@ fi
 if [[ ! -x "$python_exe" ]]; then
   if [[ ! -e "$python_exe" ]]; then
     echo >&2 "ERROR: Python interpreter not found: $python_exe"
+    ls -l $python_exe >&2
     exit 1
   elif [[ ! -x "$python_exe" ]]; then
     echo >&2 "ERROR: Python interpreter not executable: $python_exe"

diff --git a/tests/bootstrap_impls/BUILD.bazel b/tests/bootstrap_impls/BUILD.bazel
@@ -14,6 +14,7 @@
 
 load("//python/private:util.bzl", "IS_BAZEL_7_OR_HIGHER")  # buildifier: disable=bzl-visibility
 load("//tests/support:sh_py_run_test.bzl", "py_reconfig_test", "sh_py_run_test")
+load(":venv_relative_path_tests.bzl", "relative_path_test_suite")
 
 _SUPPORTS_BOOTSTRAP_SCRIPT = select({
     "@platforms//os:windows": ["@platforms//:incompatible"],
@@ -87,3 +88,5 @@ sh_py_run_test(
     sh_src = "sys_executable_inherits_sys_path_test.sh",
     target_compatible_with = _SUPPORTS_BOOTSTRAP_SCRIPT,
 )
+
+relative_path_test_suite(name = "relative_path_tests")
diff --git a/tests/bootstrap_impls/a/b/c/BUILD.bazel b/tests/bootstrap_impls/a/b/c/BUILD.bazel
@@ -0,0 +1,15 @@
+load("//python/private:util.bzl", "IS_BAZEL_7_OR_HIGHER")  # buildifier: disable=bzl-visibility
+load("//tests/support:sh_py_run_test.bzl", "py_reconfig_test")
+
+_SUPPORTS_BOOTSTRAP_SCRIPT = select({
+    "@platforms//os:windows": ["@platforms//:incompatible"],
+    "//conditions:default": [],
+}) if IS_BAZEL_7_OR_HIGHER else ["@platforms//:incompatible"]
+
+py_reconfig_test(
+    name = "nested_dir_test",
+    srcs = ["nested_dir_test.py"],
+    bootstrap_impl = "script",
+    main = "nested_dir_test.py",
+    target_compatible_with = _SUPPORTS_BOOTSTRAP_SCRIPT,
+)
diff --git a/tests/bootstrap_impls/a/b/c/nested_dir_test.py b/tests/bootstrap_impls/a/b/c/nested_dir_test.py
@@ -0,0 +1,24 @@
+# Copyright 2024 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Test that the binary being a different directory depth than the underlying interpreter works."""
+
+import unittest
+
+
+class RunsTest(unittest.TestCase):
+    def test_runs(self):
+        pass
+
+
+unittest.main()
diff --git a/tests/bootstrap_impls/venv_relative_path_tests.bzl b/tests/bootstrap_impls/venv_relative_path_tests.bzl
@@ -0,0 +1,90 @@
+# Copyright 2023 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"Unit tests for relative_path computation"
+
+load("@rules_testing//lib:test_suite.bzl", "test_suite")
+load("//python/private:py_executable_bazel.bzl", "relative_path")  # buildifier: disable=bzl-visibility
+
+_tests = []
+
+def _relative_path_test(env):
+    # Basic test cases
+
+    env.expect.that_str(
+        relative_path(
+            from_ = "a/b",
+            to = "c/d",
+        ),
+    ).equals("../../c/d")
+
+    env.expect.that_str(
+        relative_path(
+            from_ = "a/b/c",
+            to = "a/d",
+        ),
+    ).equals("../../d")
+    env.expect.that_str(
+        relative_path(
+            from_ = "a/b/c",
+            to = "a/b/c/d/e",
+        ),
+    ).equals("d/e")
+
+    # Real examples
+
+    # external py_binary uses external python runtime
+    env.expect.that_str(
+        relative_path(
+            from_ = "other_repo~/python/private/_py_console_script_gen_py.venv/bin",
+            to = "rules_python~~python~python_3_9_x86_64-unknown-linux-gnu/bin/python3",
+        ),
+    ).equals(
+        "../../../../../rules_python~~python~python_3_9_x86_64-unknown-linux-gnu/bin/python3",
+    )
+
+    # internal py_binary uses external python runtime
+    env.expect.that_str(
+        relative_path(
+            from_ = "_main/test/version_default.venv/bin",
+            to = "rules_python~~python~python_3_9_x86_64-unknown-linux-gnu/bin/python3",
+        ),
+    ).equals(
+        "../../../../rules_python~~python~python_3_9_x86_64-unknown-linux-gnu/bin/python3",
+    )
+
+    # external py_binary uses internal python runtime
+    env.expect.that_str(
+        relative_path(
+            from_ = "other_repo~/python/private/_py_console_script_gen_py.venv/bin",
+            to = "_main/python/python_3_9_x86_64-unknown-linux-gnu/bin/python3",
+        ),
+    ).equals(
+        "../../../../../_main/python/python_3_9_x86_64-unknown-linux-gnu/bin/python3",
+    )
+
+    # internal py_binary uses internal python runtime
+    env.expect.that_str(
+        relative_path(
+            from_ = "_main/scratch/main.venv/bin",
+            to = "_main/python/python_3_9_x86_64-unknown-linux-gnu/bin/python3",
+        ),
+    ).equals(
+        "../../../python/python_3_9_x86_64-unknown-linux-gnu/bin/python3",
+    )
+
+_tests.append(_relative_path_test)
+
+def relative_path_test_suite(*, name):
+    test_suite(name = name, basic_tests = _tests)