fix (venv_site_packages): Fix wrong runfiles.symlinks when py_binary is not in root module

[shayanhoshyari]

When: 1) venv_site_packages is on 2) we have a py_binary in a non-root module (e.g. a tool used in rules), and it uses python deps that result in usage of runfiles.symlinks, the ctx.runfile based symlinks end up going in the wrong folder (_main), while the ctx.actions.symlink(...) files go in the right place. This results in an invalid venv and import errors.

The reason is that actions.symlinks always go to the _main module, as Bazel docs explain. To send symlinks to other modules, one needs to use root_symlinks and prefix them with the right module name.

Fixes: #3503

[gemini-code-assist]

Summary of Changes

Hello @shayanhoshyari, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical issue affecting py_binary targets located outside the root module when venv_site_packages is active. Previously, virtual environment symlinks were misdirected, causing runtime failures and import errors. The changes ensure that symlinks are generated with the correct module context by utilizing root_symlinks and prepending the repository name to paths, thereby stabilizing virtual environment creation and resolving import errors for such configurations.

Highlights

• Virtual Environment Symlink Correction: Addresses an issue where ctx.runfile based symlinks for py_binary targets in non-root modules were incorrectly placed in the _main folder when venv_site_packages was enabled, leading to broken virtual environments and import errors.
• Adopted root_symlinks: The ctx.runfiles call in _create_venv now correctly uses root_symlinks instead of symlinks to ensure proper handling of module-specific paths, as required by Bazel's runfiles mechanism for non-main modules.
• Repository Name Prefixing for Symlinks: Symlink paths generated in create_venv_app_files are now prefixed with ctx.label.repo_name. This change ensures that symlinks are correctly directed to their respective modules when root_symlinks are used, resolving the previous misdirection.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request aims to fix an issue with venv_site_packages for py_binary targets in non-root modules by switching from symlinks to root_symlinks. The change in py_executable.bzl is correct. However, the corresponding change in venv_runfiles.bzl to construct the symlink path is incorrect. It will fail for targets in the main repository by omitting the workspace name prefix, and for external repositories by not stripping the '@' prefix from the repository name. I've left a critical comment with a suggested fix for this issue.

[rickeylev]

Thanks for the fix. Overall it LGTM. I added the basics of a test to verify it, but have run out of time to fully incorporate the test. I was manually testing by running bazel run @other//:venv_bin.

I think all that's left is to change the venv_bin to a py_test, then put a test_suite() somewhere that refers to it (tests/venvs_site_packages_libs would be a good a place)

[shayanhoshyari]

Thanks for the quick response.

@rickeylev I pushed a test in 1b6fe66

I used a sh_test so I can also actually test running the binary. Let me know if you have any concern, and I can switch to a analysis_test (test_suite).

Nevermind, needs a analysis_test due to #3505 (comment). I will try to do it tomorrow.

[shayanhoshyari]

Seems bazel 7 calls it other/ rather than other+/
https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/019bac0c-4189-4c07-8d12-8904cd464fa0/tests/venv_site_packages_libs/py_binary_other_module_test/test_attempts/attempt_1.log

Ok really need an analysis test here for this part. Will do.

[rickeylev]

You can create an analysis test if you really want (I think there's several in the venv_site_packages_libs directory somewhere you can base upon; you can just pass the venv_bin binary as the input and inspect its runfiles).

That said, I would just check that the modules are importable instead of their exact paths. The exact paths are somewhat just an implementation detail. The key attribute we care about is if things are importable. If you still want to check the paths then just do a looser match, e.g. other*/*/site-packages/nspkg/subnspkg/gamma/__init__.py

[shayanhoshyari]

I would just check that the modules are importable instead of their exact paths

Oh even easier. Ok I will do this. Since the execution of the binary checks this already, I think I just need to delete these lines that check the files. And add some better error message if imports fail in the binary.

[rickeylev]

I've cleaned up and simplified the test.

[shayanhoshyari]

I've cleaned up and simplified the test.

music to my ears. thanks.