From 6b9d146db488e7b7450965dd828c45fdccfdf2a9 Mon Sep 17 00:00:00 2001 From: Shayan Hoshyari Date: Sat, 10 Jan 2026 18:13:59 -0800 Subject: [PATCH 1/5] Tentative fix - Use root_symlinks instead and add the repo name of owner - Gonna make a PR to see if tests pass and in meantime will look for a good test. --- python/private/py_executable.bzl | 4 +++- python/private/venv_runfiles.bzl | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl index 1b884e9b3b..5bac6247ef 100644 --- a/python/private/py_executable.bzl +++ b/python/private/py_executable.bzl @@ -629,8 +629,10 @@ def _create_venv(ctx, output_prefix, imports, runtime_details, add_runfiles_root ), # venv files for user library dependencies (files that are specific # to the executable bootstrap and python runtime aren't here). + # `root_symlinks` should be used, otherwise, with symlinks files always go + # to `_main` prefix, and binaries from non-root module become broken. lib_runfiles = ctx.runfiles( - symlinks = venv_app_files.runfiles_symlinks, + root_symlinks = venv_app_files.runfiles_symlinks, ), ) diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index 02b18e57e8..97b3b9cfea 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -65,7 +65,8 @@ def create_venv_app_files(ctx, deps, venv_dir_map): bin_venv_path = paths.join(base, venv_path) if is_file(link_to): # use paths.join to handle ctx.label.package = "" - symlink_from = paths.join(ctx.label.package, bin_venv_path) + # ctx.label.repo_name should be prepended as we use runfiles.root_symlinks + symlink_from = paths.join(ctx.label.repo_name, ctx.label.package, bin_venv_path) runfiles_symlinks[symlink_from] = link_to else: venv_link = ctx.actions.declare_symlink(bin_venv_path) From 6914a1de79dec35106eac61d827e6d244a7f04e2 Mon Sep 17 00:00:00 2001 From: Shayan Hoshyari Date: Sat, 10 Jan 2026 18:28:48 -0800 Subject: [PATCH 2/5] Fix: gemini is correct --- python/private/venv_runfiles.bzl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index 97b3b9cfea..8d3f8298ab 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -65,8 +65,9 @@ def create_venv_app_files(ctx, deps, venv_dir_map): bin_venv_path = paths.join(base, venv_path) if is_file(link_to): # use paths.join to handle ctx.label.package = "" - # ctx.label.repo_name should be prepended as we use runfiles.root_symlinks - symlink_from = paths.join(ctx.label.repo_name, ctx.label.package, bin_venv_path) + # runfile_prefix should be prepended as we use runfiles.root_symlinks + runfile_prefix = ctx.label.repo_name or ctx.workspace_name + symlink_from = paths.join(runfile_prefix, ctx.label.package, bin_venv_path) runfiles_symlinks[symlink_from] = link_to else: venv_link = ctx.actions.declare_symlink(bin_venv_path) From b12c2018d184d7b4f693a4c045d79232e3472702 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Sat, 10 Jan 2026 20:04:50 -0800 Subject: [PATCH 3/5] add test/repro --- python/private/py_executable.bzl | 1 + python/private/venv_runfiles.bzl | 2 ++ tests/modules/other/BUILD.bazel | 16 ++++++++++++++++ tests/modules/other/venv_bin.py | 6 ++++++ 4 files changed, 25 insertions(+) create mode 100644 tests/modules/other/venv_bin.py diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl index 5bac6247ef..f6cfb2192d 100644 --- a/python/private/py_executable.bzl +++ b/python/private/py_executable.bzl @@ -633,6 +633,7 @@ def _create_venv(ctx, output_prefix, imports, runtime_details, add_runfiles_root # to `_main` prefix, and binaries from non-root module become broken. lib_runfiles = ctx.runfiles( root_symlinks = venv_app_files.runfiles_symlinks, + ##symlinks = venv_app_files.runfiles_symlinks, ), ) diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index 8d3f8298ab..2e3f002ebb 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -68,6 +68,8 @@ def create_venv_app_files(ctx, deps, venv_dir_map): # runfile_prefix should be prepended as we use runfiles.root_symlinks runfile_prefix = ctx.label.repo_name or ctx.workspace_name symlink_from = paths.join(runfile_prefix, ctx.label.package, bin_venv_path) + ##symlink_from = paths.join(ctx.label.package, bin_venv_path) + runfiles_symlinks[symlink_from] = link_to else: venv_link = ctx.actions.declare_symlink(bin_venv_path) diff --git a/tests/modules/other/BUILD.bazel b/tests/modules/other/BUILD.bazel index 46f1b96faa..3c4c903254 100644 --- a/tests/modules/other/BUILD.bazel +++ b/tests/modules/other/BUILD.bazel @@ -1,3 +1,4 @@ +load("@rules_python//python:py_binary.bzl", "py_binary") load("@rules_python//tests/support:py_reconfig.bzl", "py_reconfig_binary") package( @@ -12,3 +13,18 @@ py_reconfig_binary( bootstrap_impl = "system_python", main = "external_main.py", ) + +py_binary( + name = "venv_bin", + srcs = ["venv_bin.py"], + config_settings = { + "@rules_python//python/config_settings:venvs_site_packages": "yes", + "@rules_python//python/config_settings:bootstrap_impl": "script", + }, + deps = [ + # Add two packages that install into the same directory. This is + # to test that namespace packages install correctly and are importable. + "//nspkg_delta", + "//nspkg_gamma", + ], +) diff --git a/tests/modules/other/venv_bin.py b/tests/modules/other/venv_bin.py new file mode 100644 index 0000000000..6f419dd7f4 --- /dev/null +++ b/tests/modules/other/venv_bin.py @@ -0,0 +1,6 @@ +import nspkg +import nspkg.subnspkg +import nspkg.subnspkg.delta +import nspkg.subnspkg.gamma + +print("hi") From 9e45922d500d21cadc988e0e16cc79aeeb35a77a Mon Sep 17 00:00:00 2001 From: Shayan Hoshyari Date: Sat, 10 Jan 2026 23:32:55 -0800 Subject: [PATCH 4/5] Add an actual test --- python/private/py_executable.bzl | 1 - python/private/venv_runfiles.bzl | 1 - tests/modules/other/BUILD.bazel | 4 +- tests/modules/other/venv_bin.py | 2 +- tests/venv_site_packages_libs/BUILD.bazel | 12 +++++ .../py_binary_other_module_test.sh | 50 +++++++++++++++++++ 6 files changed, 66 insertions(+), 4 deletions(-) create mode 100755 tests/venv_site_packages_libs/py_binary_other_module_test.sh diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl index f6cfb2192d..5bac6247ef 100644 --- a/python/private/py_executable.bzl +++ b/python/private/py_executable.bzl @@ -633,7 +633,6 @@ def _create_venv(ctx, output_prefix, imports, runtime_details, add_runfiles_root # to `_main` prefix, and binaries from non-root module become broken. lib_runfiles = ctx.runfiles( root_symlinks = venv_app_files.runfiles_symlinks, - ##symlinks = venv_app_files.runfiles_symlinks, ), ) diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index 2e3f002ebb..7f6af0c957 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -68,7 +68,6 @@ def create_venv_app_files(ctx, deps, venv_dir_map): # runfile_prefix should be prepended as we use runfiles.root_symlinks runfile_prefix = ctx.label.repo_name or ctx.workspace_name symlink_from = paths.join(runfile_prefix, ctx.label.package, bin_venv_path) - ##symlink_from = paths.join(ctx.label.package, bin_venv_path) runfiles_symlinks[symlink_from] = link_to else: diff --git a/tests/modules/other/BUILD.bazel b/tests/modules/other/BUILD.bazel index 3c4c903254..30827f1929 100644 --- a/tests/modules/other/BUILD.bazel +++ b/tests/modules/other/BUILD.bazel @@ -18,8 +18,10 @@ py_binary( name = "venv_bin", srcs = ["venv_bin.py"], config_settings = { - "@rules_python//python/config_settings:venvs_site_packages": "yes", "@rules_python//python/config_settings:bootstrap_impl": "script", + # To make paths we are using in the test deterministic + "@rules_python//python/config_settings:python_version": "3.13", + "@rules_python//python/config_settings:venvs_site_packages": "yes", }, deps = [ # Add two packages that install into the same directory. This is diff --git a/tests/modules/other/venv_bin.py b/tests/modules/other/venv_bin.py index 6f419dd7f4..4963b49afe 100644 --- a/tests/modules/other/venv_bin.py +++ b/tests/modules/other/venv_bin.py @@ -3,4 +3,4 @@ import nspkg.subnspkg.delta import nspkg.subnspkg.gamma -print("hi") +print("@other//:venv_bin ran successfully.") diff --git a/tests/venv_site_packages_libs/BUILD.bazel b/tests/venv_site_packages_libs/BUILD.bazel index 2eb9678838..83ea31985f 100644 --- a/tests/venv_site_packages_libs/BUILD.bazel +++ b/tests/venv_site_packages_libs/BUILD.bazel @@ -1,3 +1,4 @@ +load("@rules_shell//shell:sh_test.bzl", "sh_test") load("//python:py_library.bzl", "py_library") load("//tests/support:py_reconfig.bzl", "py_reconfig_test") load( @@ -19,6 +20,17 @@ py_library( ], ) +sh_test( + name = "py_binary_other_module_test", + srcs = [ + "py_binary_other_module_test.sh", + ], + data = [ + "@other//:venv_bin", + ], + target_compatible_with = NOT_WINDOWS, +) + py_reconfig_test( name = "venvs_site_packages_libs_test", srcs = ["bin.py"], diff --git a/tests/venv_site_packages_libs/py_binary_other_module_test.sh b/tests/venv_site_packages_libs/py_binary_other_module_test.sh new file mode 100755 index 0000000000..347d0dbbdb --- /dev/null +++ b/tests/venv_site_packages_libs/py_binary_other_module_test.sh @@ -0,0 +1,50 @@ +# +# Test that for a py_binary from a dependency module, we place links created via runfiles(...) +# in the right place. This tests the fix made for issues/3503 +# + +set -eu + +# Helper to check links exist and where they point to +check_link() { + link=$1 + + # 1) Must exist + if [ ! -e "$link" ]; then + return 1 + fi + + # 2) Must be a symlink + if [ ! -L "$link" ]; then + return 1 + fi +} + +ensure_link() { + if ! check_link $1; then + echo "ERROR: $link does not exist" + return 1 + fi +} + +cd ${RUNFILES_DIR} + +# First do a tree of whole runfiles for easier debugging in case of failure +echo "[*] Runfile tree" +find . + +# Sanity check that invalid files don't exist. +echo "[*] Sanity check our ensure_link function" +if check_link ./__I_DO_NOT_EXIST__; then + echo "Check link function is broken" + exit 1 +fi + +# Check the links exist in the correct place. +echo "[*] Testing existence of symlinks in the right place" +ensure_link ./other+/_venv_bin.venv/lib/python3.13/site-packages/nspkg/subnspkg/delta/__init__.py +ensure_link ./other+/_venv_bin.venv/lib/python3.13/site-packages/nspkg/subnspkg/gamma/__init__.py + +# Finally, test that running the binary works, i.e imports are resolved. +echo "[*] Testing running the binary" +./other+/venv_bin From 0e509c368ce53aa5f19a36f9c120a4c4371fdc53 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Sun, 11 Jan 2026 10:40:53 -0800 Subject: [PATCH 5/5] clean up test --- tests/modules/other/BUILD.bazel | 2 - tests/modules/other/venv_bin.py | 11 ++++ tests/venv_site_packages_libs/BUILD.bazel | 3 ++ .../py_binary_other_module_test.sh | 50 ++----------------- 4 files changed, 17 insertions(+), 49 deletions(-) diff --git a/tests/modules/other/BUILD.bazel b/tests/modules/other/BUILD.bazel index 30827f1929..665049b9f5 100644 --- a/tests/modules/other/BUILD.bazel +++ b/tests/modules/other/BUILD.bazel @@ -19,8 +19,6 @@ py_binary( srcs = ["venv_bin.py"], config_settings = { "@rules_python//python/config_settings:bootstrap_impl": "script", - # To make paths we are using in the test deterministic - "@rules_python//python/config_settings:python_version": "3.13", "@rules_python//python/config_settings:venvs_site_packages": "yes", }, deps = [ diff --git a/tests/modules/other/venv_bin.py b/tests/modules/other/venv_bin.py index 4963b49afe..5455b23f40 100644 --- a/tests/modules/other/venv_bin.py +++ b/tests/modules/other/venv_bin.py @@ -1,6 +1,17 @@ import nspkg + +print(nspkg) + import nspkg.subnspkg + +print(nspkg.subnspkg) + import nspkg.subnspkg.delta + +print(nspkg.subnspkg.delta) + import nspkg.subnspkg.gamma +print(nspkg.subnspkg.gamma) + print("@other//:venv_bin ran successfully.") diff --git a/tests/venv_site_packages_libs/BUILD.bazel b/tests/venv_site_packages_libs/BUILD.bazel index 83ea31985f..e573dc6da6 100644 --- a/tests/venv_site_packages_libs/BUILD.bazel +++ b/tests/venv_site_packages_libs/BUILD.bazel @@ -28,6 +28,9 @@ sh_test( data = [ "@other//:venv_bin", ], + env = { + "VENV_BIN": "$(rootpath @other//:venv_bin)", + }, target_compatible_with = NOT_WINDOWS, ) diff --git a/tests/venv_site_packages_libs/py_binary_other_module_test.sh b/tests/venv_site_packages_libs/py_binary_other_module_test.sh index 347d0dbbdb..9fb71d5bc7 100755 --- a/tests/venv_site_packages_libs/py_binary_other_module_test.sh +++ b/tests/venv_site_packages_libs/py_binary_other_module_test.sh @@ -1,50 +1,6 @@ -# -# Test that for a py_binary from a dependency module, we place links created via runfiles(...) -# in the right place. This tests the fix made for issues/3503 -# +# Test that for a py_binary from a dependency module, we place links created via +# runfiles(...) in the right place. This tests the fix made for issues/3503 set -eu - -# Helper to check links exist and where they point to -check_link() { - link=$1 - - # 1) Must exist - if [ ! -e "$link" ]; then - return 1 - fi - - # 2) Must be a symlink - if [ ! -L "$link" ]; then - return 1 - fi -} - -ensure_link() { - if ! check_link $1; then - echo "ERROR: $link does not exist" - return 1 - fi -} - -cd ${RUNFILES_DIR} - -# First do a tree of whole runfiles for easier debugging in case of failure -echo "[*] Runfile tree" -find . - -# Sanity check that invalid files don't exist. -echo "[*] Sanity check our ensure_link function" -if check_link ./__I_DO_NOT_EXIST__; then - echo "Check link function is broken" - exit 1 -fi - -# Check the links exist in the correct place. -echo "[*] Testing existence of symlinks in the right place" -ensure_link ./other+/_venv_bin.venv/lib/python3.13/site-packages/nspkg/subnspkg/delta/__init__.py -ensure_link ./other+/_venv_bin.venv/lib/python3.13/site-packages/nspkg/subnspkg/gamma/__init__.py - -# Finally, test that running the binary works, i.e imports are resolved. echo "[*] Testing running the binary" -./other+/venv_bin +"$VENV_BIN"