fix(bootstrap) handle when the runfiles env vars are not correct#3644
fix(bootstrap) handle when the runfiles env vars are not correct#3644adhoc-bobcat wants to merge 1 commit intobazel-contrib:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! 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 long-standing issue where Python subprocesses could inherit and incorrectly use runfiles environment variables from their parent processes, leading to import errors. The changes introduce robust validation within the Python bootstrap scripts to ensure that each Python binary correctly identifies and utilizes its own runfiles, thereby guaranteeing proper dependency resolution across nested process calls. Highlights
🧠 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. Changelog
Activity
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for an issue where Python binaries launched as subprocesses could incorrectly inherit and use runfiles environment variables from the parent process. The change correctly validates the inherited runfiles environment variables and unsets them if they are incorrect, allowing the child process to locate its own runfiles. This is implemented in both python_bootstrap_template.txt and stage2_bootstrap_template.py. The PR also includes comprehensive tests to verify this fix by checking that a nested binary can successfully import its dependencies. Additionally, there are some unrelated but correct changes to whl_library.bzl to improve reproducibility of repository rules. The changes are well-implemented and look good.
There was still an issue where the runfiles environment variables could be set for a parent Python binary, and if that spawned another Python binary (directly or indirectly), the child would try to use the parents runfiles. This was confirmable by adding a py_library dependency to the existing bin_calls_bin test, and having inner.py try to import it. A workaround sugggested in the bug for code outside of rules_python was to remove RUNFILES_DIR from the environment inherited by the child when launching the child process, though in some cases you might need to also remove RUNFILES_MANIFEST_FILE, but working around the issue is not very satisfactory. Diving into the bootstrapping process, it seemed like the proper fix was to conditionally unset them, if they were not correct. I found equivalent places in both bootstrap template files, and if the test to confirm "runfile_dir" was correct fails, the bootstrap code itself now removes those invalid values from the environment. Later bootstrap code assumes they are set correctly, if set. When they are not set, it goes through some fallback logic to locate them. By conditionally unsetting them, the fallback logic is not invoked when it isn't necessary, shortening the bootstrap process. With that change, the modified tests now pass. Fixes bazel-contrib#3518
adhoc-bobcat
force-pushed
the
inherited_runfiles_fix
branch
from
ab8d50f to
a53de8e
Compare
1 participant
This change addresses an issue that has existed since 1.7.0 where Python binaries launched as subprocesses could incorrectly inherit and use runfiles environment variables (
RUNFILES_DIR,RUNFILES_MANIFEST_FILE) from the parent process.Why this change is needed:
When a Python binary spawns another Python binary, the child process inherits the environment variables. If the parent had runfiles-related environment variables set, the child would attempt to use the parent's runfiles tree, which is incorrect and leads to import errors if the child has different dependencies or a different runfiles layout.
Behavior Before:
A child Python process would trust and use any existing
RUNFILES_DIRandRUNFILES_MANIFEST_FILEenvironment variables. If these were set by a parent Python process, they would point to the parent's runfiles, causing the child to fail when trying to load its own resources or dependencies.Behavior After:
The Python bootstrap scripts now include a check to validate the inherited runfiles environment variables. If the runfiles variables exist but do not point to the correct location for the current binary, the bootstrap script will unset both
RUNFILES_DIRandRUNFILES_MANIFEST_FILEfrom the environment. This allows the subsequent fallback logic in the bootstrap process to correctly locate the runfiles for the current process, ensuring dependencies are resolved properly. This modification has been applied to both bootstrap template files.This change ensures that even nested Python binary calls correctly find their respective runfiles.
Fixes #3518