Skip to content

fix: manual runfiles path construction when using submodules #3636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
faximan wants to merge 3 commits into
base: main
Choose a base branch
from
+46 −14
Open

fix: manual runfiles path construction when using submodules #3636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5238e75
Fix manual runfiles path construction
faximan Feb 24, 2026
16f3e4b
Add entry to CHANGELOG
faximan Feb 24, 2026
dc940e9
Add test
faximan Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ END_UNRELEASED_TEMPLATE
* (zipapp) Resolve issue passing through compression settings in
`py_zippapp_binary` targets
([#3646](https://github.com/bazel-contrib/rules_python/issues/3646)).
* (bootstrap) Fixed incorrect runfiles path construction in bootstrap scripts when binary is defined in another bazel module

{#v0-0-0-added}
### Added
Expand Down
10 changes: 10 additions & 0 deletions examples/bzlmod/other_module/other_module/pkg/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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",
Expand All @@ -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"])
8 changes: 8 additions & 0 deletions examples/bzlmod/tests/other_module/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,18 @@
# 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)"},
)
22 changes: 22 additions & 0 deletions examples/bzlmod/tests/other_module/other_module_import_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
19 changes: 5 additions & 14 deletions python/private/py_executable.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Collaborator

@aignas aignas Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add a test somewhere? For example examples/bzlmod has submodules in it which could ensure that we can still import things.

@rickeylev Any other ideas how we could do this?

Copy link
Author

@faximan faximan Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and failed to write a test here tests the specific fix in this PR. As I'm very unfamiliar with this code base in general, I am afraid I'd need support to land that.

Copy link
Author

@faximan faximan Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aignas any way we can move this forward?

Copy link
Collaborator

@aignas aignas Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have a test before we can merge this. Even a happy path test case that previously did not work and now works would be better than nothing. Because we have examples/bzlmod which has submodule other_module, it should be doable IMHO. Having a py_binary defined in the other_module and the test in the examples/bzlmod would be the way to go.

Copy link
Author

@faximan faximan Mar 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, thanks for the suggestions. I have added the most minimal test I could come up with that fails on main but passes with the fix. Please take a look!

Copy link
Author

@faximan faximan Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test fails on Windows for unrelated reasons to the PR. I'm not familiar with Windows usage, so please let me know if you see any obvious way to fix.

Otherwise I could disable it on Windows like this I think?

    target_compatible_with = select({
        "@platforms//os:windows": ["@platforms//:incompatible"],
        "//conditions:default": [],
    }),

ctx.workspace_name,
stage2_bootstrap.short_path,
),
"%stage2_bootstrap%": runfiles_root_path(ctx, stage2_bootstrap.short_path),
"%workspace_name%": ctx.workspace_name,
},
)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 ""

Expand Down Expand Up @@ -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:
Expand All @@ -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,
Expand Down