refactor: create full venv for bootstrap=system_python #3473
Conversation
Summary of ChangesHello @rickeylev, 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 refactors the Python bootstrapping mechanism to ensure that the 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. 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 is a good refactoring that unifies the system_python and script bootstrap implementations by always creating a full virtual environment. The changes simplify the codebase by removing conditional logic, which improves maintainability.
I've added one suggestion to further refactor a section of the code to reduce duplication. Additionally, as a result of these changes, some conditional checks in other parts of the file (like if venv.interpreter:) have become redundant, as venv.interpreter will no longer be None. These could be cleaned up in a subsequent change to remove the now-unreachable else branches.
| if not venvs_use_declare_symlink_enabled or not runtime.supports_build_time_venv: | ||
| recreate_venv_at_runtime = True | ||
|
|
||
| # When the venv symlinks are disabled, the $venv/bin/python3 file isn't | ||
| # needed or used at runtime. However, the zip code uses the interpreter | ||
| # File object to figure out some paths. | ||
| interpreter = ctx.actions.declare_file("{}/{}".format(bin_dir, py_exe_basename)) | ||
| ctx.actions.write(interpreter, "actual:{}".format(interpreter_actual_path)) | ||
|
|
||
| elif runtime.interpreter: | ||
| # Even though ctx.actions.symlink() is used, using | ||
| # declare_symlink() is required to ensure that the resulting file | ||
| # in runfiles is always a symlink. An RBE implementation, for example, | ||
| # may choose to write what symlink() points to instead. | ||
| interpreter = ctx.actions.declare_symlink("{}/{}".format(bin_dir, py_exe_basename)) | ||
|
|
||
| rel_path = relative_path( | ||
| # dirname is necessary because a relative symlink is relative to | ||
| # the directory the symlink resides within. | ||
| from_ = paths.dirname(runfiles_root_path(ctx, interpreter.short_path)), | ||
| to = interpreter_actual_path, | ||
| ) | ||
|
|
||
| ctx.actions.symlink(output = interpreter, target_path = rel_path) | ||
| else: | ||
| interpreter = ctx.actions.declare_symlink("{}/{}".format(bin_dir, py_exe_basename)) | ||
| ctx.actions.symlink(output = interpreter, target_path = runtime.interpreter_path) | ||
| ctx.actions.symlink(output = interpreter, target_path = rel_path) | ||
| else: | ||
| interpreter = None | ||
| interpreter = ctx.actions.declare_symlink("{}/{}".format(bin_dir, py_exe_basename)) | ||
| ctx.actions.symlink(output = interpreter, target_path = runtime.interpreter_path) |
There was a problem hiding this comment.
The elif and else blocks here have some repeated logic for creating the interpreter symlink. You can refactor this to reduce duplication by declaring the symlink first and then conditionally determining the target path. This would make the code a bit cleaner and easier to maintain.
if not venvs_use_declare_symlink_enabled or not runtime.supports_build_time_venv:
recreate_venv_at_runtime = True
# When the venv symlinks are disabled, the $venv/bin/python3 file isn't
# needed or used at runtime. However, the zip code uses the interpreter
# File object to figure out some paths.
interpreter = ctx.actions.declare_file("{}/{}".format(bin_dir, py_exe_basename))
ctx.actions.write(interpreter, "actual:{}".format(interpreter_actual_path))
else:
# Even though ctx.actions.symlink() is used, using
# declare_symlink() is required to ensure that the resulting file
# in runfiles is always a symlink. An RBE implementation, for example,
# may choose to write what symlink() points to instead.
interpreter = ctx.actions.declare_symlink("{}/{}".format(bin_dir, py_exe_basename))
if runtime.interpreter:
target_path = relative_path(
# dirname is necessary because a relative symlink is relative to
# the directory the symlink resides within.
from_ = paths.dirname(runfiles_root_path(ctx, interpreter.short_path)),
to = interpreter_actual_path,
)
else:
target_path = runtime.interpreter_path
ctx.actions.symlink(output = interpreter, target_path = target_path)
|
This is almost done, but tow issues remain:
I'm not sure if both of these can be fixed. I guess if we drop bazel 7? We'll do that in a month or so anyways. Maybe don't test bazel 7 + packaging? |
|
I personally think that it could be best to fail if we detect bazel 7 and we cannot support it. +1 for excluding the tests via |
|
The issue with bazel 7 isn't with our rules, but with rules_pkg, in two respects:
Our venv's work fine on bazel 7; it's just rules_pkg that has an issue. A py_binary doesn't know if its being consumed by pkg_tar, so it can't automatically disable the symlink-based venv creation. edit -- if we disable build-time venv creation for bazel 7, that could work. Its somewhat a regression in functionality, though. One only necessary if rules_pkg is used. |
|
Ah -- how about, for bazel 7, we don't create a full venv for system_python? That would match existing behavior. Bazel 8 + system_python would do full venv. Then we can drop Bazel 7 support In Jan/Feb timeline |
2 participants
The system_python bootstrap is basically just an alternative stage1 bootstrap now.
Finish unifying it with bootstrap=script by having it create a full venv.
Notes:
the pyvenv.cfg trick only works in 3.11+. The runtime_env toolchain has a conditional
to mark the toolchain as supporting build time venv or not based on python version.
WIP -- DO NOT MERGE