Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pkgs/watcher/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
## 2.0.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also describe how symlinks are handled in the DirectoryWatcher dartdoc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


- Changes the behavior of the `DirectoryWatcher` so that it no longer follows
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make sense to also mention the bug fix aspect of the change

Bug fix: fix DirectoryWatcher bad performance when there are cyclic symlinks in the directory being watched.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

symlinks. On Linux, but not on MacOS or Windows, it used to watch directories
to which there was a symlink within the directory being watched. It no longer
does that. Code that depends on that behavior will need to be updated so that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer people let us know if that is what they wanted, since we already have most of the implementation and could add it back.

So maybe

If you were relying on that behavior, please file an issue.

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

it independently traverses the root directory, locating and resolving
symlinks, and watching any directories being linked to.

## 1.1.3

- Improve handling of
Expand Down
4 changes: 2 additions & 2 deletions pkgs/watcher/lib/src/directory_watcher/linux.dart
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class _LinuxDirectoryWatcher
});

_listen(
Directory(path).list(recursive: true),
Directory(path).list(recursive: true, followLinks: false),
(FileSystemEntity entity) {
if (entity is Directory) {
_watchSubdir(entity.path);
Expand Down Expand Up @@ -228,7 +228,7 @@ class _LinuxDirectoryWatcher

/// Emits [ChangeType.ADD] events for the recursive contents of [path].
void _addSubdir(String path) {
_listen(Directory(path).list(recursive: true), (FileSystemEntity entity) {
_listen(Directory(path).list(recursive: true, followLinks: false), (FileSystemEntity entity) {
if (entity is Directory) {
_watchSubdir(entity.path);
} else {
Expand Down
4 changes: 2 additions & 2 deletions pkgs/watcher/lib/src/directory_watcher/mac_os.dart
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ class _MacOSDirectoryWatcher

if (_files.containsDir(path)) continue;

var stream = Directory(path).list(recursive: true);
var stream = Directory(path).list(recursive: true, followLinks: false);
var subscription = stream.listen((entity) {
if (entity is Directory) return;
if (_files.contains(path)) return;
Expand Down Expand Up @@ -373,7 +373,7 @@ class _MacOSDirectoryWatcher

_files.clear();
var completer = Completer<void>();
var stream = Directory(path).list(recursive: true);
var stream = Directory(path).list(recursive: true, followLinks: false);
_initialListSubscription = stream.listen((entity) {
if (entity is! Directory) _files.add(entity.path);
}, onError: _emitError, onDone: completer.complete, cancelOnError: true);
Expand Down
2 changes: 1 addition & 1 deletion pkgs/watcher/lib/src/directory_watcher/polling.dart
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class _PollingDirectoryWatcher
_filesToProcess.add(null);
}

var stream = Directory(path).list(recursive: true);
var stream = Directory(path).list(recursive: true, followLinks: false);
_listSubscription = stream.listen((entity) {
assert(!_events.isClosed);

Expand Down
4 changes: 2 additions & 2 deletions pkgs/watcher/lib/src/directory_watcher/windows.dart
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ class _WindowsDirectoryWatcher

if (_files.containsDir(path)) continue;

var stream = Directory(path).list(recursive: true);
var stream = Directory(path).list(recursive: true, followLinks: false);
var subscription = stream.listen((entity) {
if (entity is Directory) return;
if (_files.contains(entity.path)) return;
Expand Down Expand Up @@ -435,7 +435,7 @@ class _WindowsDirectoryWatcher

_files.clear();
var completer = Completer<void>();
var stream = Directory(path).list(recursive: true);
var stream = Directory(path).list(recursive: true, followLinks: false);
void handleEntity(FileSystemEntity entity) {
if (entity is! Directory) _files.add(entity.path);
}
Expand Down
2 changes: 1 addition & 1 deletion pkgs/watcher/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: watcher
version: 1.1.3
version: 2.0.0
description: >-
A file system watcher. It monitors changes to contents of directories and
sends notifications when files have been added, removed, or modified.
Expand Down
15 changes: 15 additions & 0 deletions pkgs/watcher/test/directory_watcher/shared.dart
Original file line number Diff line number Diff line change
Expand Up @@ -341,4 +341,19 @@ void sharedTests() {
await inAnyOrder(events);
});
});
group('symlinks', () {
test('are not watched when not enabled', () async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the name? There is no longer a choice whether to enable watching symlinks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

writeFile('dir/sub/a.txt', contents: 'a');
writeFile('sibling/sub/b.txt', contents: '1');
writeSymlink('dir/linked', target: 'sibling');
await startWatcher(path: 'dir');

writeFile('sibling/sub/b.txt', contents: '2');
// Ensure that any events for the first modification arrive before the
// events for the second modification.
await pumpEventQueue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment that there is expected to be no modification event for dir/linked/b.txt? Does the test API have a way to test that explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added.

I don't know whether the test actually confirms that. It kind of looks like it's just expecting that there's a modify event for a.txt somewhere in the stream, but I can't find any way to ensure that there's only one event, or even that there's no event of a given kind.

writeFile('dir/sub/a.txt', contents: 'a');
await expectModifyEvent('dir/sub/a.txt');
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be interesting to test whether the process of creating or deleting a symlink inside a watched directory fires a notification. I would expect it to, and it might be worth pinning that down. But it's not essential for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should be tested and described with this release, since if there are any inconsistencies we might need another breaking release to fix them.

One possible behaviour is that within a watched directory, changes to symlinks to files/directories are reported exactly as changes to files would be.

Another possible behaviour is that they are always ignored.

I think either one is fine, as long as it is consistent across all three platforms + documented.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a test.

});
}
13 changes: 13 additions & 0 deletions pkgs/watcher/test/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,19 @@ void renameDir(String from, String to) {
void deleteDir(String path) {
Directory(p.join(d.sandbox, path)).deleteSync(recursive: true);
}

/// Schedules writing a symlink in the sandbox at [path] that links to [target].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "schedules" refers to tracking of modification times which is not done here.

Possibly it should be if we also test behaviour of symlinking files.

If not then "Schedules writing" can be replaced with just "Writes".

void writeSymlink(String path, {required String target}) {
var fullPath = p.join(d.sandbox, path);

// Create any needed subdirectories.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be just

createSync(target, recursive: true)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. I didn't do that because I was copying the pattern in writeFile, trying to keep a consistent style.

var dir = Directory(p.dirname(fullPath));
if (!dir.existsSync()) {
dir.createSync(recursive: true);
}

Link(fullPath).createSync(target);
}

/// Runs [callback] with every permutation of non-negative numbers for each
/// argument less than [limit].
Expand Down
Loading