Add tests for hard and soft links #12590
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MichaReiser
force-pushed
the
watcher-link-tests
branch
5 times, most recently
from
af1f272
to
08adb19
Compare
|
MichaReiser
force-pushed
the
watcher-link-tests
branch
2 times, most recently
from
21696c5
to
a8ec90d
Compare
MichaReiser
force-pushed
the
watcher-link-tests
branch
4 times, most recently
from
43c98c9
to
084c2a3
Compare
MichaReiser
commented
Jul 31, 2024
| @@ -1153,7 +1151,9 @@ mod tests { | |||
| #[test] | |||
| #[cfg(target_family = "unix")] | |||
| fn symlink() -> anyhow::Result<()> { | |||
| use crate::db::tests::TestDb; | |||
There was a problem hiding this comment.
This fixes a few unused warnings on windows
MichaReiser
commented
Jul 31, 2024
| .expect("Expected watch changes but observed none.") | ||
| } | ||
|
|
||
| fn try_stop_watch(&mut self, timeout: Duration) -> Option<Vec<watch::ChangeEvent>> { |
There was a problem hiding this comment.
I hope this will help to improve the stability of the tests. We can now use a high timeout for functions that expect at least a single event and a smaller timeout for functions that don't.
MichaReiser
commented
Jul 31, 2024
| fn next_io_tick() { | ||
| let temp = tempfile::tempfile().unwrap(); | ||
| /// Updates the content of a file and ensures that the last modified file time is updated. | ||
| fn update_file(path: impl AsRef<SystemPath>, content: &str) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Let's hope this fixes the other instability
AlexWaygood
reviewed
Jul 31, 2024
carljm
approved these changes
Jul 31, 2024
MichaReiser
force-pushed
the
watcher-link-tests
branch
from
f4d9ec9
to
3570bb7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The goal I set out for was to add support for symlinks and hard links when watching files. I no longer think we should pursue this goal, at least not now. This PR has mainly become a documentation PR and it adds a few tests demonstrating the behavior on different systems/platforms.
Why not support watching symlinks and hardlinks? Primarily, because I don't think there's sufficient use to justify the complexity. I tested both VS Code and Pycharm and both show stale results after making changes to a hard-linked file or updating a symlinked file. This is as good as Red Knot is today. Specifically, the following changes are not supported:
The other reason is that the platform specific file watcher APIs don't generate the necessary events. I compared the events with
@parcel/watcher(used by VS Code, and parcel), andfs.watch(nodejs) and the generated events match. The only system that I found that supported hard links well is Pyright which is based onchokidar. I'm not a 100% sure how chokidar makes it work but I belive it does a directory traversal to find changes rather than relying on the events only. We could do that as well, but it seems rather expensive to support a not so common workflow.There are ways how we could support symlinks and hardlinks without traversing the directory.
HandletoVec<File>to lookup all files pointing to the same hard link. There's some complexity how we would support this with ourSystemabstractionreal_pathtoVec<File>for all symlinked files.I think any approach (or traversing the directory to find changes) is worth exploring if this is something that users run into often.
It is probably also worth exploring watchman as a possible
notifybackend to support very large projects.Side note: Ruff doesn't follow symlinks today. Pyright has support for it.
Test Plan
This PR is only tests 😆