fix(bootstrap): manual runfiles path construction when using submodules (#3636)

This fixes #3563.
Verified by running the repro in
https://github.com/mering/reproduction_rules_python_1_7.

The identified regression in
b8e32c4
is problematic because prepending `ctx.workspace_name` to `short_path`
results in paths containing `..` (e.g., `_main/../sub+/path/to/file.py`)
when building from a root module that includes other modules. This
causes the `_find_runfiles_root`
[logic](https://github.com/faximan/rules_python/blob/eb6ac472eb86cc263acc336a2b73982043069aae/python/private/site_init_template.py#L73),
which _counts slashes_, to incorrectly calculate the runfiles root.

The fix is simply using the available `runfiles_root_path` function
instead. In the example above, this makes the path simply
`sub+/path/to/file.py`.

---------

Co-authored-by: Ignas Anikevicius <240938+aignas@users.noreply.github.com>

Author: faximan

CHANGELOG.md

@@ -80,6 +80,9 @@ Other changes:
   configuration. See the {any}`RULES_PYTHON_PYCACHE_DIR` environment variable
   for more information.
   ([#3643](https://github.com/bazel-contrib/rules_python/issues/3643)).
+* (bootstrap) Fixed incorrect runfiles path construction in bootstrap
+  scripts when binary is defined in another bazel module
+  ([#3563](https://github.com/bazel-contrib/rules_python/issues/3563)).
 
 {#v0-0-0-added}
 ### Added

examples/bzlmod/other_module/other_module/pkg/BUILD.bazel

@@ -1,5 +1,6 @@
 load("@rules_python//python:py_binary.bzl", "py_binary")
 load("@rules_python//python:py_library.bzl", "py_library")
+load("@rules_python//python/zipapp:py_zipapp_binary.bzl", "py_zipapp_binary")
 
 py_library(
     name = "lib",
@@ -26,4 +27,13 @@ py_binary(
     ],
 )
 
+# This is used for regression testing runfiles paths in submodules.
+# https://github.com/bazel-contrib/rules_python/issues/3563.
+py_zipapp_binary(
+    name = "bin_zipapp",
+    testonly = True,
+    binary = ":bin",
+    visibility = ["//visibility:public"],
+)
+
 exports_files(["data/data.txt"])

examples/bzlmod/tests/other_module/BUILD.bazel

@@ -5,10 +5,24 @@
 # in the root module.
 
 load("@bazel_skylib//rules:build_test.bzl", "build_test")
+load("@rules_python//python:py_test.bzl", "py_test")
 
 build_test(
     name = "other_module_bin_build_test",
     targets = [
         "@our_other_module//other_module/pkg:bin",
     ],
 )
+
+py_test(
+    name = "other_module_import_test",
+    srcs = ["other_module_import_test.py"],
+    data = ["@our_other_module//other_module/pkg:bin_zipapp"],
+    env = {"ZIPAPP_PATH": "$(location @our_other_module//other_module/pkg:bin_zipapp)"},
+    # For now, skip this test on Windows because it fails for reasons
+    # other than the code path being tested.
+    target_compatible_with = select({
+        "@platforms//os:windows": ["@platforms//:incompatible"],
+        "//conditions:default": [],
+    }),
+)

examples/bzlmod/tests/other_module/other_module_import_test.py

@@ -0,0 +1,22 @@
+"""Regression test for https://github.com/bazel-contrib/rules_python/issues/3563"""
+import os
+import subprocess
+import sys
+
+def main():
+    # The rlocation path for the bin_zipapp. It is in the "our_other_module" repository.
+    zipapp_path = os.environ.get("ZIPAPP_PATH")
+    print(f"Running bin_zipapp at: {zipapp_path}")
+
+    result = subprocess.run([zipapp_path], capture_output=True, text=True)
+    print("--- bin_zippapp stdout ---")
+    print(result.stdout)
+    print("--- bin_zippapp stderr ---")
+    print(result.stderr)
+
+    if result.returncode != 0:
+        print(f"bin_zippapp failed with return code {result.returncode}")
+        sys.exit(result.returncode)
+
+if __name__ == "__main__":
+    main()

python/private/py_executable.bzl

@@ -510,10 +510,7 @@ def _create_zip_main(ctx, *, stage2_bootstrap, runtime_details, venv):
         substitutions = {
             "%python_binary%": python_binary,
             "%python_binary_actual%": python_binary_actual,
-            "%stage2_bootstrap%": "{}/{}".format(
-                ctx.workspace_name,
-                stage2_bootstrap.short_path,
-            ),
+            "%stage2_bootstrap%": runfiles_root_path(ctx, stage2_bootstrap.short_path),
             "%workspace_name%": ctx.workspace_name,
         },
     )
@@ -616,7 +613,7 @@ def _create_venv(ctx, output_prefix, imports, runtime_details, add_runfiles_root
             "%add_runfiles_root_to_sys_path%": add_runfiles_root_to_sys_path,
             "%coverage_tool%": _get_coverage_tool_runfiles_path(ctx, runtime),
             "%import_all%": "True" if read_possibly_native_flag(ctx, "python_import_all_repositories") else "False",
-            "%site_init_runfiles_path%": "{}/{}".format(ctx.workspace_name, site_init.short_path),
+            "%site_init_runfiles_path%": runfiles_root_path(ctx, site_init.short_path),
             "%workspace_name%": ctx.workspace_name,
         },
         computed_substitutions = computed_subs,
@@ -671,10 +668,7 @@ def _get_coverage_tool_runfiles_path(ctx, runtime):
     if (ctx.configuration.coverage_enabled and
         runtime and
         runtime.coverage_tool):
-        return "{}/{}".format(
-            ctx.workspace_name,
-            runtime.coverage_tool.short_path,
-        )
+        return runfiles_root_path(ctx, runtime.coverage_tool.short_path)
     else:
         return ""
 
@@ -777,10 +771,7 @@ def _create_stage1_bootstrap(
         if (ctx.configuration.coverage_enabled and
             runtime and
             runtime.coverage_tool):
-            coverage_tool_runfiles_path = "{}/{}".format(
-                ctx.workspace_name,
-                runtime.coverage_tool.short_path,
-            )
+            coverage_tool_runfiles_path = runfiles_root_path(ctx, runtime.coverage_tool.short_path)
         else:
             coverage_tool_runfiles_path = ""
         if runtime:
@@ -793,7 +784,7 @@ def _create_stage1_bootstrap(
         subs["%coverage_tool%"] = coverage_tool_runfiles_path
         subs["%import_all%"] = ("True" if read_possibly_native_flag(ctx, "python_import_all_repositories") else "False")
         subs["%imports%"] = ":".join(imports.to_list())
-        subs["%main%"] = "{}/{}".format(ctx.workspace_name, main_py.short_path)
+        subs["%main%"] = runfiles_root_path(ctx, main_py.short_path)
 
     ctx.actions.expand_template(
         template = template,