Ensure that rctx.symlink invalidates correctly if it is a copy#28682
Ensure that rctx.symlink invalidates correctly if it is a copy#28682fmeum wants to merge 3 commits intobazelbuild:masterfrom
rctx.symlink invalidates correctly if it is a copy#28682Conversation
fmeum
force-pushed
the
28575-symlink-content
branch
from
74cd481 to
4d9c878
Compare
fmeum
force-pushed
the
28575-symlink-content
branch
from
4d9c878 to
2c1b25e
Compare
fmeum
changed the title
rctx.symlink invalidation on Windowsrctx.symlink invalidates correctly if it is a copy
github-actions
Bot
added
team-ExternalDeps
![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 correctly ensures that ctx.symlink invalidates correctly when emulated as a copy, for example on Windows. The change adds a watch on the symlink target when native symlinks are not supported. The implementation is sound and is accompanied by good tests. I've added one comment about an opportunity to improve test coverage for file permissions.
|
|
||
| self.RunBazel(args=['build', '@other_repo//:file'], cwd=work_dir) | ||
|
|
||
| def doTestSymlinkToFileInOtherRepoNoticesFileChange(self): |
There was a problem hiding this comment.
This is a great test for file content changes. To make it more robust, consider also testing the preservation of file permissions, especially the executable bit. When ctx.symlink is emulated as a copy on Windows, the file permissions are not preserved by the current implementation in WindowsFileSystem, which could lead to issues if the linked file is an executable script.
You could extend this test or add a new one where data.txt is an executable script, and the genrule attempts to execute it. This would highlight the issue and ensure the symlink emulation is correct with respect to permissions as well.
|
@bazel-io fork 9.1.0 |
meteorcloudy
added
awaiting-PR-merge
|
@meteorcloudy I realized that this has an issue, will send a fix later. |
fmeum
force-pushed
the
28575-symlink-content
branch
from
b8006d1 to
c9bb725
Compare
|
@meteorcloudy PTAL, I decided to add the symlink bit to the fingerprint since I could see cache entries without the additional watch polluting (local or remote) repo contents caches when the startup flag is flipped. I haven't managed to reproduce this in a test. |
github-actions
Bot
removed
the
awaiting-PR-merge
…elbuild#28682) ### Description When `rctx.symlink` is implemented as a copy (on Windows with default settings), the target is now watched so that modifications to it are still seen through the fake "symlink". Motivation ### Build API Changes No ### Checklist - [x] I have added tests for the new use cases (if any). - [ ] I have updated the documentation (if applicable). ### Release Notes RELNOTES: `rctx.symlink` now implicitly watches the target if it falls back to a copy. Closes bazelbuild#28682. PiperOrigin-RevId: 873775628 Change-Id: I6bd9d38e19b20e3342802ab4a11acd319b7ac52f
…opy (#28682) (#28741) ### Description When `rctx.symlink` is implemented as a copy (on Windows with default settings), the target is now watched so that modifications to it are still seen through the fake "symlink". Motivation ### Build API Changes No ### Checklist - [x] I have added tests for the new use cases (if any). - [ ] I have updated the documentation (if applicable). ### Release Notes RELNOTES: `rctx.symlink` now implicitly watches the target if it falls back to a copy. Closes #28682. PiperOrigin-RevId: 873775628 Change-Id: I6bd9d38e19b20e3342802ab4a11acd319b7ac52f Commit 6dd6396 Co-authored-by: Fabian Meumertzheim <[email protected]>
2 participants
Description
When
rctx.symlinkis implemented as a copy (on Windows with default settings), the target is now watched so that modifications to it are still seen through the fake "symlink".Motivation
Build API Changes
No
Checklist
Release Notes
RELNOTES:
rctx.symlinknow implicitly watches the target if it falls back to a copy.