-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow watch of symbolic links to folders on Unix #52679
Conversation
Tagging subscribers to this area: @carlossanlop |
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Directory.Changed.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Directory.Changed.cs
Outdated
Show resolved
Hide resolved
@@ -28,11 +31,11 @@ public void FileSystemWatcher_Directory_Changed_WatchedFolder() | |||
{ | |||
using (var testDirectory = new TempDirectory(GetTestFilePath())) | |||
using (var dir = new TempDirectory(Path.Combine(testDirectory.Path, "dir"))) | |||
using (var watcher = new FileSystemWatcher(dir.Path, "*")) | |||
using (var watcher = new FileSystemWatcher(GetDirectoryOrSymLink(dir.Path), "*")) | |||
{ | |||
Action action = () => Directory.SetLastWriteTime(dir.Path, DateTime.Now + TimeSpan.FromSeconds(10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to your change, but I'm having trouble understanding why this test is not expecting any events after changing the last write time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FSW in this case is watching the same folder whose LastWriteTime
is being changed and this test demonstrates that changes to the watched directory are not being caught.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a second thought, I think these tests that use ExpectEvent(watcher, 0, ...)
are wrong and instead should use ExpectNoEvent
.
@jozkee, in the I suggest you clone the code from that program, remove the workaround code, consume your compiled bits containing the changes in this PR, and see if the bug is fixed. |
This won't fix that bug. This fix is only allowing the watched directory to be a symlink. It's not following symlinks in child directories, nor following symlink'ed files. There's even a more complicated scenario with #36091 where a chain of symlinks is changed in the middle. I think the best fix for #36091 would be to make it use a polling file watcher that follows symlinks to poll the target. |
This fix it's actually allowing the follow of symlinks within the watched directory since it removes But I am wondering if doing so could be a problem, specially for cyclic symlinks but it appears to me that cycles are not a problem since the code uses a set to track the child directories that are being wached: runtime/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs Lines 383 to 386 in d77854a
@ericstj do you think that could be a problem? |
That wasn't my understanding on the docs on
Test it out and see 😄 I guess it depends on this: WRT the differences between platforms, we could make this a capability that's just different between the platforms, or we could introduce an API and have that API throw on platforms where we can't honor it. Do you imagine there are folks who would want to disallow this behavior: that would speak to having an API? I remember @GrabYourPitchforks having an opinion about things that follow symlinks by default. |
Ah yes, the symlinks within the watched directory were being followed with my previous change because we are manually calling
Agree, follow symlinks should not be the default, or at least should not be addressed by this PR, it would be a discussion of it's own.
We already have a very similar discussion in the issue for symbolic link APIs: It seems there is people in both sides of "having follow semantics as the default" or "make follow links opt-in" so I think it would not be great to allow that here. |
Test Filed #53010 and disabled the test in those versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left some small, optional suggestions and a couple of questions.
src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.SymbolicLink.cs
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.SymbolicLink.cs
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.SymbolicLink.cs
Show resolved
Hide resolved
using var watcher = new FileSystemWatcher(linkPath); | ||
|
||
// Act - Assert | ||
Assert.Throws<FileNotFoundException>(() => watcher.EnableRaisingEvents = true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: What's the exception message? I find it strange that we throw FileNotFoundException
instead of DirectoryNotFoundException
. The symbolic link was created with isDirectory: true
, and the file exists, and we already know it doesn't make sense to watch a file, so I would've expected an error telling me a directory could not be found.
Same question applies for line 55, where the link targets itself, but it's created with isDirectory: true
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the exception message?
Error reading the {directory name} directory.
Same question applies for line 55, where the link targets itself, but it's created with isDirectory: true too.
Same error is thrown.
src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.SymbolicLink.cs
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.SymbolicLink.cs
Outdated
Show resolved
Hide resolved
So then this PR is just making a change to follow the symlink if it is the root directory watched, is that correct? Is that the same as Windows? I didn't want to imply any sort of right/wrong judgement with my previous comment around following symlinks of child directories. I actually think that could be seen as an interesting feature that is easier for us to do on Linux due to the way the watching APIs work (we already need to traverse and add and their is a flag for following). The feature could also be added to the Windows implementation if we shared some of the code that traverses, just rather than add directories it finds (not necessary since ReadDirectoryChanges can be recursive), it could add symlinks it finds. Interestingly enough if we did that, it would actually be similar to what would be needed to make a complete symlink following solution for Linux as well, since it could cover the symlink-ed file case.
Agreed. I think it'd be possible to have a complete symlink following file-watcher, thought it should probably be opt-in. IMHO such a feature would be a nice complement to the other symlink API that's being worked on and would probably be more impactful than this change. You might consider doing it instead of this change if this change was causing our behaviors to diverge on platforms. If not then this seems fine. |
After my last changes, that's correct.
It is no longer diverging, so I think we can merge this and consider adding the "follow symlinks" feature flag on top. |
Such a feature would help solve #36091, when not using polling, and is probably in line with a user's expectations when they don't know anything about the symlinks in the file-system they run on. |
@jaredpar, infra question: many OSX builds are failing to unzip test assets, do you know if this is a known issue? I already tried re-running a couple times and the error persists. |
cc @dotnet/runtime-infrastructure. |
I believe what happened is that the zip produced by libraries build has a problem: Since that build was successful your reruns aren't fixing it. The legs which are failing are all trying to consume the artifact produced by this build leg. It's not clear to me why that artifact is bad, I don't see a failure in zipping. Maybe we can examine the artifact in AzDo and see if something is wrong with it. |
I can reproduce locally consuming that artifact:
|
There's a failure in the iOS simulator leg that may be significant Actually it's in iOS, tvOS, and MacCatalyst. A crash in System.IO.FileSystem.Watcher.Tests |
It is failing without even running tests, right? [link]:
Apple TV (tvOS 13.4) - created by XHarness.log what does this mean?:
The source changes I made should only affect Linux. I'm kinda lost now, any guidance on how to repro these errors locally? |
The tests run, but crash and unfortunately the logs don't tell much. I'll try and pull your branch to see if I can tell. |
FWIW I think @steveisok and I am looking at two different things. @steveisok is looking at test failures from the staging build that don't actually fail the PR, though we'd want to understand them if this PR were introducing them. I was commenting on the failures in build leg that launches tests on OSX, those appear to be stemming from a corrupt test tar-ball, which is corrupt in the build artifacts. If you wanted to see if that corrupt tar-ball was a fluke you could try to retrigger the entire build. My guess is there was an unreported corruption in upload of the tar-ball, which would be an AzDo task issue. cc @MattGal in case he's seen that before. |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
Correct, I was curious if there were any staging pipeline failures with this change and wanted to help validate. @jozkee, it looks like what you're doing in the symlinks tests isn't supported on iOS. I wish this was printed in the logs and that's something I can take away from this and try to improve upon.
|
@steveisok that's weird, I was supposedly using Is the attribute not working properly? |
Apparently not. My symlinks PR had the same issue: I was using |
I think the attribute is working, it's just that runtime/src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs Line 49 in 35a0c5b
|
I've avoided hitting |
@@ -461,6 +461,11 @@ protected static bool CanCreateSymbolicLinks | |||
|
|||
public static bool CreateSymLink(string targetPath, string linkPath, bool isDirectory) | |||
{ | |||
if (OperatingSystem.IsIOS() || OperatingSystem.IsTvOS() || OperatingSystem.IsMacCatalyst()) // OSes that don't support Process.Start() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android and Browser can't start processes either:
if (OperatingSystem.IsIOS() || OperatingSystem.IsTvOS() || OperatingSystem.IsMacCatalyst()) // OSes that don't support Process.Start() | |
if (OperatingSystem.IsIOS() || OperatingSystem.IsTvOS() || OperatingSystem.IsMacCatalyst() || OperatingSystem.IsAndroid() || OperatingSystem.IsBrowser()) // OSes that don't support Process.Start() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't those platforms be specified with the attribute as the other three?
runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
Lines 1206 to 1209 in 4bdaa8e
[UnsupportedOSPlatform("ios")] | |
[UnsupportedOSPlatform("maccatalyst")] | |
[UnsupportedOSPlatform("tvos")] | |
public bool Start() |
cc @jeffhandley
Co-authored-by: Carlos Sanchez <[email protected]>
This reverts commit 5b44eb0195c2e5824ac375862765ea5a2cdeb0ad.
Co-authored-by: Carlos Sanchez <[email protected]>
Fixes #25078