windows: expose support for recursive directory watches #339
Conversation
nicks
force-pushed
the
nicks/recursive-patch
branch
2 times, most recently
from
d4b92eb
to
020ef21
Compare
|
LGTM. @houndci-bot , could merge this PR? @nicks Another question is, after configuring the The problem I currently encounter is that due to the limitations of business scenarios, there will be a lot of monitoring subdirectories, and the recursive |
|
@nicksantos if this gets merged, you may need to include this #361 when your recursive watch hits a filepath that exceeds windows' syscall.MAX_PATH. |
nicks
force-pushed
the
nicks/recursive-patch
branch
from
68d5111
to
ecfdba6
Compare
nicks
force-pushed
the
nicks/recursive-patch
branch
from
ecfdba6
to
31297bf
Compare
|
thanks! addressed all comments. most of the comments were on to changed things that were also issues in other tests, so i took the liberty of addressing those too. also rebased on main. |
nicks
force-pushed
the
nicks/recursive-patch
branch
from
31297bf
to
785816f
Compare
| // SetRecursive makes future Add() calls watch directories recursively | ||
| // if this file system supports that. | ||
| // Must be called before Add/Remove. | ||
| func (w *Watcher) SetRecursive() error { |
There was a problem hiding this comment.
I'm not sure about this API -- what do others think?
Adding a separate API is good in that there aren't any breaking changes for existing users.
There is no way to disable the recursive flag. Is that important?
There was a problem hiding this comment.
ya, i could go either way on this.
my thinking was that you want the API to be as restrictive as possible - no switching back and forth between recursive and non-recursive, no switching modes once the watcher has started listening on events.
But certainly SetRecursive(bool) would be a lot more idiomatic.
There was a problem hiding this comment.
There are a lot of different options, and it's important to get the API right, because the same API would need to support a user-space recursive watcher (#18) and FSEvents on macOS.
@pbnjay @rogpeppe Having look at the Mac side, do you have any opinions on how this API should work?
If I recall, macOS is always recursive when making a watch?
This option seems pretty good and is backwards compatible -- a method on the watcher.
Other options would be some other form of NewWatcher with configuration. If we want the watcher to be 100% recursive or 100% not, this may be better.
Or having a separate AddRecursive function, which would allow intermixing of recursive and not-recursive. But we may wind up with too many variants -- like with and without Symlinks, Recursive, Ops filtering, etc.
There was a problem hiding this comment.
I can confirm that fsevents on macOS is recursive by default.
I am in support of Other options would be some other form of NewWatcher with configuration. If we want the watcher to be 100% recursive or 100% not, this may be better.
I don't see a concrete use case in which we want to keep switching back and forth. Most of the time, people want a recursive watch. If there are paths or sub-trees that they are not interested in, they can reasonably add additional filtering-out step at their application level to ignore such events.
Please let us know what else to do for this to be acceptable. thanks
There was a problem hiding this comment.
As a side note, we forked this (https://github.com/PreVeil/fsnotify/commits/preveil) and enabled a recursive watch like this in our production software and we don't observe issues #259 and #243.
There was a problem hiding this comment.
One possibility would be to use Add("dir/**") or Add("dir/...").
Especially since recursive operations may be expensive on some platforms, there may be use cases where you might want things like "recursively watch ~/.cache/go-build, and non-recursively watch ~/.cache/go-mod", or something like that.
The advantage of using a string like that is that #352 can be added without too much friction too as e.g. AddMask("path", fsnotify.CREATE | fsnotify.DELETE) or some such.
There was a problem hiding this comment.
Would it make sense to have a syntax like Glob?
https://pkg.go.dev/path/filepath@go1.18.4#Glob
https://pkg.go.dev/io/fs@go1.18.4#Glob
There was a problem hiding this comment.
One challenge with this approach is what happens on BSD (kqueue) or Linux (inotify), since only Windows and FSEvents on macOS support recursive watching.
Is it an error on some platforms? At least until such time as a user-space recursive watcher (#18) covers those platforms?
There was a problem hiding this comment.
One challenge with this approach is what happens on BSD (kqueue) or Linux (inotify), since only Windows and FSEvents on macOS support recursive watching.
Yeah, .Add() can return an error in those cases, similar to how SetRecursive() does now. Something like:
func recursive(path string) (string, bool) {
if strings.HasSuffix(path, "/...") || strings.HasSuffix(path, `\...`) {
return path[:len(path)-4], true
}
return path, false
}
path, recurse := recursive(path)
if recurse {
return errors.New("recursion not supported on this system")
}
Or for Windows and other systems where it works:
recurse := false
path, recurse = recursive(path)
Would it make sense to have a syntax like Glob?
I'd be in favour of that; there's an old issue for that (#10).
Using the /... syntax is probably better, since e.g. something like **/*.zip won't work and I don't think we need to support that (it'll be tricky to get that to work well for all cases).
There was a problem hiding this comment.
Yeah, that makes sense, and there is precedence for that format (e.g. go test ./...). Sounds good.
This adds a recursive watcher for inotify; per my suggestion in [1] it
uses the "/..." syntax in the path to indicate recursive watches,
similar to how Go tools work:
w, _ := fsnotify.NewWatcher()
w.Add("some/dir/...")
This will watch "some/dir" as well as any subdirectories of "some/dir".
The upshot of using a syntax like this is that we can:
1. With AddRecursive(path string) adding new Add* methods would become
hard; for example AddMask("path", fsnotify.Create) to only get CREATE
events; we'd then also have to add AddMaskRecursive(). Plus we'd
have to add a RemoveRecursive() as well.
2. With Watcher.SetRecursive() like in #339 it's not possible to add
some paths recursively and others non-recursively, which may be
useful in some cases. Also, it makes it a bit easier to accept user
input; in the CLI or config you can set "dir/..." and just pass that
as-is to fsnotify, without applications having to write special code.
For other watchers it will return ErrRecursionUnsupported for now;
Windows support is already mostly finished in #339, and kqueue can be
added in much the same way as inotify in a future PR.
I also moved all test helpers to helper_test.go, and added a bunch of
"shell-like" functions so you're not forever typing error checks and
filepath.Join(). The new "eventCollector" is also useful in tests to
conveniently collect a slice of events.
TODO:
- Also support recursion in Remove(), and deal with paths added with
"...". I just updated the documentation but didn't actually implement
anything.
- A few test cases don't seem quite right; want to get #470 merged first
as it really confuses things.
- Maybe think of a few more test cases.
[1]: #339 (comment)
This adds a recursive watcher for inotify; per my suggestion in [1] it
uses the "/..." syntax in the path to indicate recursive watches,
similar to how Go tools work:
w, _ := fsnotify.NewWatcher()
w.Add("some/dir/...")
This will watch "some/dir" as well as any subdirectories of "some/dir".
The upshot of using a syntax like this is that we can:
1. With AddRecursive(path string) adding new Add* methods would become
hard; for example AddMask("path", fsnotify.Create) to only get CREATE
events; we'd then also have to add AddMaskRecursive(). Plus we'd
have to add a RemoveRecursive() as well.
2. With Watcher.SetRecursive() like in #339 it's not possible to add
some paths recursively and others non-recursively, which may be
useful in some cases. Also, it makes it a bit easier to accept user
input; in the CLI or config you can set "dir/..." and just pass that
as-is to fsnotify, without applications having to write special code.
For other watchers it will return ErrRecursionUnsupported for now;
Windows support is already mostly finished in #339, and kqueue can be
added in much the same way as inotify in a future PR.
I also moved all test helpers to helper_test.go, and added a bunch of
"shell-like" functions so you're not forever typing error checks and
filepath.Join(). The new "eventCollector" is also useful in tests to
conveniently collect a slice of events.
TODO:
- Also support recursion in Remove(), and deal with paths added with
"...". I just updated the documentation but didn't actually implement
anything.
- A few test cases don't seem quite right; want to get #470 merged first
as it really confuses things.
- Maybe think of a few more test cases.
[1]: #339 (comment)
This adds a recursive watcher for inotify; per my suggestion in [1] it
uses the "/..." syntax in the path to indicate recursive watches,
similar to how Go tools work:
w, _ := fsnotify.NewWatcher()
w.Add("some/dir/...")
This will watch "some/dir" as well as any subdirectories of "some/dir".
The upshot of using a syntax like this is that we can:
1. With AddRecursive(path string) adding new Add* methods would become
hard; for example AddMask("path", fsnotify.Create) to only get CREATE
events; we'd then also have to add AddMaskRecursive(). Plus we'd
have to add a RemoveRecursive() as well.
2. With Watcher.SetRecursive() like in #339 it's not possible to add
some paths recursively and others non-recursively, which may be
useful in some cases. Also, it makes it a bit easier to accept user
input; in the CLI or config you can set "dir/..." and just pass that
as-is to fsnotify, without applications having to write special code.
For other watchers it will return ErrRecursionUnsupported for now;
Windows support is already mostly finished in #339, and kqueue can be
added in much the same way as inotify in a future PR.
I also moved all test helpers to helper_test.go, and added a bunch of
"shell-like" functions so you're not forever typing error checks and
filepath.Join(). The new "eventCollector" is also useful in tests to
conveniently collect a slice of events.
TODO:
- Also support recursion in Remove(), and deal with paths added with
"...". I just updated the documentation but didn't actually implement
anything.
- A few test cases don't seem quite right; want to get #470 merged first
as it really confuses things.
- Maybe think of a few more test cases.
[1]: #339 (comment)
This adds a recursive watcher for inotify; per my suggestion in [1] it
uses the "/..." syntax in the path to indicate recursive watches,
similar to how Go tools work:
w, _ := fsnotify.NewWatcher()
w.Add("some/dir/...")
This will watch "some/dir" as well as any subdirectories of "some/dir".
The upshot of using a syntax like this is that we can:
1. With AddRecursive(path string) adding new Add* methods would become
hard; for example AddMask("path", fsnotify.Create) to only get CREATE
events; we'd then also have to add AddMaskRecursive(). Plus we'd
have to add a RemoveRecursive() as well.
2. With Watcher.SetRecursive() like in #339 it's not possible to add
some paths recursively and others non-recursively, which may be
useful in some cases. Also, it makes it a bit easier to accept user
input; in the CLI or config you can set "dir/..." and just pass that
as-is to fsnotify, without applications having to write special code.
For other watchers it will return ErrRecursionUnsupported for now;
Windows support is already mostly finished in #339, and kqueue can be
added in much the same way as inotify in a future PR.
I also moved all test helpers to helper_test.go, and added a bunch of
"shell-like" functions so you're not forever typing error checks and
filepath.Join(). The new "eventCollector" is also useful in tests to
conveniently collect a slice of events.
TODO:
- Also support recursion in Remove(), and deal with paths added with
"...". I just updated the documentation but didn't actually implement
anything.
- A few test cases don't seem quite right; want to get #470 merged first
as it really confuses things.
- Maybe think of a few more test cases.
[1]: #339 (comment)
|
There's still quite a bit of work that needs to be done in terms of API; not 100% sure what it will look like yet, but certainly different from this patch. For example #521 adds a very different way of passing options, and I probably want to use The core is pretty trivial (just pass a boolean around), just need to figure out an API that works well. Either way, much of this will have to be re-done, and I don't think there's much value in keeping this PR open or basing it this PR, as most things will be different. Can subscribe to #21 for notification when it's added. |
Recursive watches can be added by using a "/..." parameter, similar to
the Go command:
w.Add("dir") // Behaves as before.
w.Add("dir/...") // Watch dir and all paths underneath it.
w.Remove("dir") // Remove the watch for dir and, if
// recursive, all paths underneath it too
w.Remove("dir/...") // Behaves like just "dir" if the path was
// recursive, error otherwise (probably
// want to add recursive remove too at some
// point).
The advantage of using "/..." vs. an option is that it can be easily
specified in configuration files and the like; for example from a TOML
file:
[watches]
dirs = ["/tmp/one", "/tmp/two/..."]
Options for this were previously discussed at:
#339 (comment)
This should be expanded to other backends too; I started with Windows
because the implementation is the both the easiest and has the least
amount of control (just setting a boolean parameter), and we can focus
mostly on writing tests and documentation and the for it, and we can
then match the inotify and kqueue behaviour to the Windows one.
Updates #18
Recursive watches can be added by using a "/..." parameter, similar to
the Go command:
w.Add("dir") // Behaves as before.
w.Add("dir/...") // Watch dir and all paths underneath it.
w.Remove("dir") // Remove the watch for dir and, if
// recursive, all paths underneath it too
w.Remove("dir/...") // Behaves like just "dir" if the path was
// recursive, error otherwise (probably
// want to add recursive remove too at some
// point).
The advantage of using "/..." vs. an option is that it can be easily
specified in configuration files and the like; for example from a TOML
file:
[watches]
dirs = ["/tmp/one", "/tmp/two/..."]
Options for this were previously discussed at:
#339 (comment)
This should be expanded to other backends too; I started with Windows
because the implementation is the both the easiest and has the least
amount of control (just setting a boolean parameter), and we can focus
mostly on writing tests and documentation and the for it, and we can
then match the inotify and kqueue behaviour to the Windows one.
Fixes #21
Recursive watches can be added by using a "/..." parameter, similar to
the Go command:
w.Add("dir") // Behaves as before.
w.Add("dir/...") // Watch dir and all paths underneath it.
w.Remove("dir") // Remove the watch for dir and, if
// recursive, all paths underneath it too
w.Remove("dir/...") // Behaves like just "dir" if the path was
// recursive, error otherwise (probably
// want to add recursive remove too at some
// point).
The advantage of using "/..." vs. an option is that it can be easily
specified in configuration files and the like; for example from a TOML
file:
[watches]
dirs = ["/tmp/one", "/tmp/two/..."]
Options for this were previously discussed at:
#339 (comment)
This should be expanded to other backends too; I started with Windows
because the implementation is the both the easiest and has the least
amount of control (just setting a boolean parameter), and we can focus
mostly on writing tests and documentation and the for it, and we can
then match the inotify and kqueue behaviour to the Windows one.
Fixes #21
Recursive watches can be added by using a "/..." parameter, similar to
the Go command:
w.Add("dir") // Behaves as before.
w.Add("dir/...") // Watch dir and all paths underneath it.
w.Remove("dir") // Remove the watch for dir and, if
// recursive, all paths underneath it too
w.Remove("dir/...") // Behaves like just "dir" if the path was
// recursive, error otherwise (probably
// want to add recursive remove too at some
// point).
The advantage of using "/..." vs. an option is that it can be easily
specified in configuration files and the like; for example from a TOML
file:
[watches]
dirs = ["/tmp/one", "/tmp/two/..."]
Options for this were previously discussed at:
#339 (comment)
This should be expanded to other backends too; I started with Windows
because the implementation is the both the easiest and has the least
amount of control (just setting a boolean parameter), and we can focus
mostly on writing tests and documentation and the for it, and we can
then match the inotify and kqueue behaviour to the Windows one.
Fixes #21
Recursive watches can be added by using a "/..." parameter, similar to
the Go command:
w.Add("dir") // Behaves as before.
w.Add("dir/...") // Watch dir and all paths underneath it.
w.Remove("dir") // Remove the watch for dir and, if
// recursive, all paths underneath it too
w.Remove("dir/...") // Behaves like just "dir" if the path was
// recursive, error otherwise (probably
// want to add recursive remove too at some
// point).
The advantage of using "/..." vs. an option is that it can be easily
specified in configuration files and the like; for example from a TOML
file:
[watches]
dirs = ["/tmp/one", "/tmp/two/..."]
Options for this were previously discussed at:
#339 (comment)
This should be expanded to other backends too; I started with Windows
because the implementation is the both the easiest and has the least
amount of control (just setting a boolean parameter), and we can focus
mostly on writing tests and documentation and the for it, and we can
then match the inotify and kqueue behaviour to the Windows one.
Fixes #21
Recursive watches can be added by using a "/..." parameter, similar to
the Go command:
w.Add("dir") // Behaves as before.
w.Add("dir/...") // Watch dir and all paths underneath it.
w.Remove("dir") // Remove the watch for dir and, if
// recursive, all paths underneath it too
w.Remove("dir/...") // Behaves like just "dir" if the path was
// recursive, error otherwise (probably
// want to add recursive remove too at some
// point).
The advantage of using "/..." vs. an option is that it can be easily
specified in configuration files and the like; for example from a TOML
file:
[watches]
dirs = ["/tmp/one", "/tmp/two/..."]
Options for this were previously discussed at:
#339 (comment)
This should be expanded to other backends too; I started with Windows
because the implementation is the both the easiest and has the least
amount of control (just setting a boolean parameter), and we can focus
mostly on writing tests and documentation and the for it, and we can
then match the inotify and kqueue behaviour to the Windows one.
Fixes #21
Co-authored-by: Milas Bowman <milasb@gmail.com>
What does this pull request do?
On Windows, ReadDirectoryChanges allows recursive watches, so I added a function call to enable it
I saw some discussion on this in these issues:
#18
#21
but given that there's not much activity in this repo, it didn't seem worth blocking on a user-space solutoin to expose this
How should this be manually tested?
I've run the unittest on Windows and confirms it watches things successfully. There's also a new test in integration_test, adapted from the existing subdirectory test.