fix(bootstrap): manual runfiles path construction when using submodules (#3636)
This fixes https://github.com/bazel-contrib/rules_python/issues/3563.
Verified by running the repro in
https://github.com/mering/reproduction_rules_python_1_7.
The identified regression in
https://github.com/bazel-contrib/rules_python/commit/b8e32c454a1158cd78ce4ecaef809b99bef4e5da
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>
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e574870..031b066 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -80,6 +80,9 @@
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
diff --git a/examples/bzlmod/other_module/other_module/pkg/BUILD.bazel b/examples/bzlmod/other_module/other_module/pkg/BUILD.bazel
index 53344c7..318822d 100644
--- a/examples/bzlmod/other_module/other_module/pkg/BUILD.bazel
+++ b/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 @@
],
)
+# 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"])
diff --git a/examples/bzlmod/tests/other_module/BUILD.bazel b/examples/bzlmod/tests/other_module/BUILD.bazel
index 1bd8a90..24231e6 100644
--- a/examples/bzlmod/tests/other_module/BUILD.bazel
+++ b/examples/bzlmod/tests/other_module/BUILD.bazel
@@ -5,6 +5,7 @@
# 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",
@@ -12,3 +13,16 @@
"@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": [],
+ }),
+)
diff --git a/examples/bzlmod/tests/other_module/other_module_import_test.py b/examples/bzlmod/tests/other_module/other_module_import_test.py
new file mode 100644
index 0000000..6b92a85
--- /dev/null
+++ b/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()
diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl
index 284aea6..495c7dd 100644
--- a/python/private/py_executable.bzl
+++ b/python/private/py_executable.bzl
@@ -510,10 +510,7 @@
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 @@
"%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 @@
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 @@
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 @@
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,
