fix(system_python): write import paths to generated file instead of using PYTHONPATH#3242
Merged
rickeylev merged 10 commits intobazel-contrib:mainfrom Sep 6, 2025
Merged
Conversation
aignas
reviewed
Sep 6, 2025
Collaborator
aignas
left a comment
aignas
left a comment
There was a problem hiding this comment.
Gave a quick look at this and I think the change is really exciting. I'll do a thorough review once it is ready for review.
Collaborator
Author
|
fyi @mering -- non-shell based bootstrap (still needs a system python, so not our coveted native launcher, but doesn't require you to have a shell installed) @jacky8hyf -- should make bootstrap=script work with site disabled ( |
Collaborator
Author
|
Ok, ready for review |
aignas
approved these changes
Sep 6, 2025
Collaborator
aignas
left a comment
aignas
left a comment
There was a problem hiding this comment.
This is really cool, thank you!
This was referenced Dec 15, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 25, 2026
…y_binary (#3495) ### Problem In #3242, it looks like `rules_python` introduced new template placeholders in the bootstrap scripts: `%stage2_bootstrap%` and `%interpreter_args%`. And from what I can tell, also made their successful substitution a requirement. This works totally fine when the caller is calling `rules_python` directly. However, when external repositories (like gRPC's cython) define py_binary using the native rule, these placeholders don't seem to be substituted? The result is that the literal placeholder text ends up in the generated bootstrap scripts, causing `SyntaxError`s or file-not-found errors at runtime ### Fix Detect if the `%stage2_bootstrap%` variable isn't expanded and fallback to `%main%` which IS substituted even for native `py_binary`. For `%interpreter_args%`, wrap it in triple-quotes so it's hopefully always valid Python syntax, then detect the sentinel and default to an empty list. This is a bit hacky, but is fairly non-invasive. --------- Co-authored-by: Richard Levasseur <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changes the system_python bootstrap to use a 2-stage process like the script
bootstrap does. Among other things, this means the import paths are written to
a generated file (
bazel_site_init.py, same as boostrap=script) and sys.pathsetup is performed by the Python code in stage 2.
Since the PYTHONPATH environment variable isn't used, this fixes the problem on
Windows where the value is too long.
This also better unifies the system_python and script based bootstraps because the
same stage 2 code and bazel_site_init code is used.
Along the way, several other improvements:
(stdlib, binary paths, runtime site packages).
-S).interpreter_argsattribute andRULES_PYTHON_ADDITIONAL_INTERPRETER_ARGSenv var work with system_python.main_modulework with system_python.this because their environment doesn't install any shells as a security
precaution).
Fixes #2652