Support %bazel_workspace% interpolation in --repo_env. Attempt to sup… #3
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis update removes the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bazel
participant CommandEnvironment
participant ResourceManager
User->>Bazel: Runs command with --repo_env containing %bazel_workspace%
Bazel->>CommandEnvironment: Initializes with workspace and repo env
CommandEnvironment->>CommandEnvironment: Replace %bazel_workspace% with workspace path in repo env
CommandEnvironment->>ResourceManager: getLocalResourceManager() via singleton
Bazel->>User: Executes command with interpolated environment
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/shell/bazel/starlark_repository_test.sh (1)
883-924: Good test implementation for the workspace path interpolation feature.This test effectively validates the
%bazel_workspace%interpolation in repository environment variables by:
- Creating a tool in the workspace
- Setting up a repo rule that uses the tool via a PATH environment variable with
%bazel_workspace%- Correctly handling platform-specific path separators for both Windows and non-Windows
- Verifying the tool executes properly with the interpolated path
Consider quoting
$PATHin line 923 to prevent word splitting if it contains spaces:-common --repo_env=PATH="$repo_env_path" +common --repo_env=PATH="${repo_env_path}"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 923-923: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java(3 hunks)src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java(1 hunks)src/test/shell/bazel/starlark_repository_test.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
src/test/shell/bazel/starlark_repository_test.sh
[warning] 923-923: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (4)
src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java (1)
545-547: Good documentation update for the%bazel_workspace%interpolation feature.The help text addition clearly explains the functionality of replacing
%bazel_workspace%with the absolute workspace path. This documentation is important for users to understand how to leverage this feature.src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java (3)
236-236: Removal of resourceManager parameter from constructor.The change simplifies the dependency structure by removing the resourceManager parameter, which will be obtained through the singleton instance instead.
358-360: Good implementation of the%bazel_workspace%interpolation feature.The code properly checks for a non-null workspace before performing the replacement, preventing potential null pointer exceptions. The string replacement is straightforward and effectively implements the desired functionality.
718-718: Migrated to singleton ResourceManager pattern.The method now uses the singleton ResourceManager.instance() instead of an instance field. This simplifies the API and removes the need for passing the ResourceManager through the constructor.
|
/gemini review |
![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 support for %bazel_workspace% interpolation in the --repo_env flag, allowing repository rules to access the workspace path. It also attempts to support Windows. The changes include modifications to CommandEnvironment.java, CommonCommandOptions.java, and starlark_repository_test.sh. Overall, the changes seem well-structured and include a new test case to verify the interpolation functionality. However, there are a few areas that could benefit from closer examination and potential improvement.
Summary of Findings
- Resource Manager Removal: The
ResourceManagerfield and its associated logic have been removed fromCommandEnvironment.java. It's important to verify that this removal doesn't introduce any regressions or performance issues, especially concerning resource management during command execution. While the code now usesResourceManager.instance(), it's crucial to ensure this singleton access pattern is appropriate and doesn't lead to contention or other unforeseen consequences. - Windows Support: The pull request description mentions an attempt to support Windows. The test case
test_repo_env_workspace_interpolationincludes a conditional check for$is_windows. It's important to ensure that the Windows-specific path handling is correct and that the test case adequately covers Windows scenarios. Further investigation may be needed to confirm that the changes are fully compatible with Windows. - Error Handling in Starlark Repository Test: In the
test_repo_env_workspace_interpolationtest, the error message in thefailfunction could be improved by including the actual PATH variable. This would provide more context when debugging test failures related to environment variable interpolation.
Merge Readiness
The pull request introduces a valuable feature with the %bazel_workspace% interpolation. However, the removal of the ResourceManager and the attempt to support Windows require careful consideration. I recommend addressing the identified issues before merging. I am unable to directly approve this pull request, and other reviewers should examine the code before merging.
| result = ctx.execute(["my_tool"]) | ||
| if result.return_code != 0: | ||
| fail("my_tool failed ({}, PATH = {}): {}".format(result.return_code, ctx.os.environ["PATH"], result.stderr)) |
There was a problem hiding this comment.
Consider including the actual PATH variable in the error message for better debugging:
fail("my_tool failed ({}, PATH = {}): {}".format(result.return_code, ctx.os.environ["PATH"], result.stderr))
| result = ctx.execute(["my_tool"]) | |
| if result.return_code != 0: | |
| fail("my_tool failed ({}, PATH = {}): {}".format(result.return_code, ctx.os.environ["PATH"], result.stderr)) | |
| fail "my_tool failed ($result.return_code, PATH = ${ctx.os.environ["PATH"]}): $result.stderr" |
…port Windows
Summary by CodeRabbit
New Features
%bazel_workspace%in environment variable values with the workspace path when configuring repository environments.Documentation
%bazel_workspace%substitution.Tests
%bazel_workspace%in repository environment variables.