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" |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Repository Environment Interpolation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| if (workspace.getWorkspace() != null) { | ||
| value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString()); | ||
| } |
There was a problem hiding this comment.
Path Injection Risk
Direct string replacement of %bazel_workspace% with workspace path without validation creates path injection vulnerability. Malicious repository rules could exploit this to manipulate paths and potentially access files outside intended boundaries. This could lead to unauthorized file access or path traversal attacks.
if (workspace.getWorkspace() != null) {
Path workspacePath = workspace.getWorkspace();
// Normalize path to prevent path traversal attacks
String normalizedPath = workspacePath.normalize().toAbsolutePath().toString();
value = value.replace("%bazel_workspace%", normalizedPath);
} else {
logger.warning("Cannot interpolate %bazel_workspace% as workspace is null");
}
Commitable Suggestion
| if (workspace.getWorkspace() != null) { | |
| value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString()); | |
| } | |
| if (workspace.getWorkspace() != null) { | |
| Path workspacePath = workspace.getWorkspace(); | |
| // Normalize path to prevent path traversal attacks | |
| String normalizedPath = workspacePath.normalize().toAbsolutePath().toString(); | |
| value = value.replace("%bazel_workspace%", normalizedPath); | |
| } else { | |
| logger.warning("Cannot interpolate %bazel_workspace% as workspace is null"); | |
| } |
Standards
- CWE-22
- CWE-73
- OWASP-A01
| if (workspace.getWorkspace() != null) { | ||
| value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString()); | ||
| } |
There was a problem hiding this comment.
Null Workspace Check
The workspace null check protects against NPE, but there's no handling for the case when workspace is null. Without fallback behavior, repository rules using %bazel_workspace% will receive uninterpolated values, causing potential runtime failures.
if (workspace.getWorkspace() != null) {
value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString());
} else {
logger.warning("Cannot interpolate %bazel_workspace% as workspace is null");
// Leave the placeholder unchanged to avoid silent failures
}
Commitable Suggestion
| if (workspace.getWorkspace() != null) { | |
| value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString()); | |
| } | |
| if (workspace.getWorkspace() != null) { | |
| value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString()); | |
| } else { | |
| logger.warning("Cannot interpolate %bazel_workspace% as workspace is null"); | |
| // Leave the placeholder unchanged to avoid silent failures | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Handling
| public ResourceManager getLocalResourceManager() { | ||
| return resourceManager; | ||
| return ResourceManager.instance(); |
There was a problem hiding this comment.
ResourceManager Reference Change
Method implementation changed from returning stored resourceManager field to ResourceManager.instance(). This changes from instance-specific to global singleton access pattern, potentially affecting resource isolation and reliability in multi-environment scenarios.
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Resource-Mgmt
|
|
||
| public ResourceManager getLocalResourceManager() { | ||
| return resourceManager; | ||
| return ResourceManager.instance(); |
There was a problem hiding this comment.
Resource Manager Reference
The constructor parameter resourceManager was removed but the getLocalResourceManager method now returns a global singleton instead of the instance-specific reference. This changes the logical behavior from using a command-specific resource manager to a global one, potentially affecting resource management isolation.
Standards
- Business-Rule-State-Consistency
- Logic-Verification-Data-Flow
| private final RemoteAnalysisCachingEventListener remoteAnalysisCachingEventListener; | ||
| private final ImmutableList.Builder<IdleTask> idleTasks = ImmutableList.builder(); | ||
| private final ResourceManager resourceManager; | ||
|
|
There was a problem hiding this comment.
Unnecessary Field Removal
The resourceManager field was removed but its usage pattern was changed to a singleton access. This creates an inconsistent pattern where some dependencies are injected while others use global state, reducing maintainability.
Standards
- Clean-Code-Consistency
- Maintainability-Quality-Dependency-Management
- SOLID-DIP
| @Nullable String buildRequestIdOverride, | ||
| ConfigFlagDefinitions configFlagDefinitions, | ||
| ResourceManager resourceManager) { | ||
| ConfigFlagDefinitions configFlagDefinitions) { |
There was a problem hiding this comment.
Constructor Parameter Removal
The resourceManager parameter was removed from the constructor but its usage was changed to a singleton access pattern. This creates inconsistent dependency management where some components are injected while others use global state.
Standards
- SOLID-DIP
- Clean-Code-Dependency-Management
- Design-Pattern-Dependency-Injection
| if (workspace.getWorkspace() != null) { | ||
| value = value.replace("%bazel_workspace%", workspace.getWorkspace().getPathString()); | ||
| } |
There was a problem hiding this comment.
String Replacement Logic
String replacement is performed unconditionally without checking if the placeholder exists in the value. This could lead to unnecessary string operations for every environment variable, impacting performance with large environment sets.
Standards
- Clean-Code-Performance
- Maintainability-Quality-Optimization
1 participant
…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.