Include unresolved symlink target into the source directory hash #10
Conversation
This makes it so that changes to the targets of unresolved symlinks invalidate actions that depend on the source directory containing them.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis update introduces a new integration test class, Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant BazelBuildSystem
participant SourceDirectory
participant ActionExecutor
TestRunner->>BazelBuildSystem: Trigger build with genrule consuming SourceDirectory
BazelBuildSystem->>SourceDirectory: Scan directory and contents
SourceDirectory-->>BazelBuildSystem: Return files, dirs, symlinks info
BazelBuildSystem->>ActionExecutor: Decide if action needs invalidation
ActionExecutor-->>BazelBuildSystem: Execute genrule if invalidated
BazelBuildSystem-->>TestRunner: Report build result and action execution events
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java (1)
181-188: Clarify rationale for ignoring the test.The test is marked with
@Ignore("TODO(#25834)"). Are you planning to address this soon, or should it remain disabled?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java(1 hunks)src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java(2 hunks)src/test/java/com/google/devtools/build/lib/skyframe/BUILD(1 hunks)src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java(2 hunks)src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java(1 hunks)
🔇 Additional comments (8)
src/test/java/com/google/devtools/build/lib/skyframe/BUILD (1)
1924-1943: Appropriate integration test addition for source directory symlink handlingThis new test target will verify proper handling of source directories including symlinks, which aligns well with the PR's objective of ensuring unresolved symlink targets are included in source directory hashing.
src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java (2)
833-833: Updated method call to match the new signatureThe signature of
danglingSymlinkhas been updated to no longer take a metadata parameter since it now computes the digest internally.
847-847: Updated method call to match the new signatureConsistent update to remove the metadata parameter from
danglingSymlinkcall, reflecting the factory method's new signature.src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java (1)
189-191: Updated method call to match the new signatureThe code has been correctly updated to use the new signature of
danglingSymlink, which now computes the digest internally from the symlink target rather than requiring an explicit metadata parameter.src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java (2)
579-588: Document the collision-avoidance approach.XORing the first byte of the SHA-256 digest to avoid collisions with regular file digests is clever but not immediately obvious. Consider adding a short comment explaining the rationale behind
digest[0] ^= 1;.
17-24:❓ Verification inconclusive
Consider verifying ISO_8859_1 usage for non-Latin filenames.
The imports for
ISO_8859_1andDigestHashFunctionlook fine, but if there's a possibility of non-Latin filename characters, consider verifying that converting them to ISO-8859-1 will not produce unexpected results.Would you like to run a quick check across the codebase for potential non-ASCII path usage?
🏁 Script executed:
#!/bin/bash # Description: Find lines that reference `PathFragment.create` with potential non-ASCII usage rg -A 3 "PathFragment\.create\(.*[^\x00-\x7F].*\)"Length of output: 52
Manual Verification Recommended: Confirm ISO_8859_1 Handling for Non-Latin Filenames
The initial grep check for non-ASCII usage in calls to
PathFragment.createreturned no results. However, the absence of evidence from this low-yield search means we cannot conclusively rule out potential issues with non-Latin filenames. Please perform a manual review of any file path encoding logic that may rely on ISO-8859-1 to ensure that filenames with non-Latin characters are handled correctly.
- Verify that conversions or operations involving ISO_8859_1 (as imported in this file) are safe for non-Latin filename characters.
- Re-assess any related logic in file path manipulations across the codebase to avoid encoding issues.
src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java (2)
45-86: Kudos on the comprehensive setup method.The
setUpGenrule()method meticulously sets up a varied directory structure with nested files, symlinks, and dangling symlinks to test invalidation semantics thoroughly. Nice job covering a wide range of conditions!
214-233: Great coverage of edge-cases and failure scenarios.Your tests for crossing package boundaries and infinite symlink expansions are well-structured. This ensures you catch potential breakages in complex real-world scenarios.
|
/refacto-test |
🤖 Refacto.AI: PR Summary & Reviewer GuideWarning: Requirements context (e.g., linked ticket) was not provided. Analysis is based solely on PR metadata and code diff, which may be incomplete. 🎯 1. The Goal (Why was this PR raised?)This PR aims to include unresolved symlink targets into the source directory hash, ensuring that changes to dangling symlink targets are properly tracked and invalidate dependent actions when necessary. 🗺️ 2. The Changes (What was implemented?)
🤔 3. Key Areas for Human ReviewArea of Concern: Hash Generation for Dangling Symlinks
Area of Concern: Integration Test Completeness
Area of Concern: Potential Performance Impact
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
1 similar comment
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
🤖 Refacto.AI: PR Summary & Reviewer GuideWarning: Requirements context (e.g., linked ticket) was not provided. Analysis is based solely on PR metadata and code diff, which may be incomplete. 🎯 1. The Goal (Why was this PR raised?)This PR aims to include unresolved symlink targets into the source directory hash, ensuring that changes to dangling symlinks properly invalidate dependent actions. The implementation modifies how dangling symlinks are handled in the filesystem traversal system. 🗺️ 2. The Changes (What was implemented?)
🤔 3. Key Areas for Human ReviewArea of Concern: Symlink Digest Computation
Area of Concern: Integration Test Coverage
Area of Concern: Potential Bug in Symlink Handling
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
This makes it so that changes to the targets of unresolved symlinks invalidate actions that depend on the source directory containing them.
Work towards bazelbuild#25834
Summary by CodeRabbit
New Features
Bug Fixes
Tests