diff --git a/pkg/watch/notify_test.go b/pkg/watch/notify_test.go index 1d3a7c05..6aca38ec 100644 --- a/pkg/watch/notify_test.go +++ b/pkg/watch/notify_test.go @@ -1,5 +1,3 @@ -// +build !windows - package watch import ( @@ -14,9 +12,8 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/windmilleng/tilt/internal/dockerignore" "github.com/windmilleng/tilt/internal/testutils/tempdir" @@ -41,6 +38,11 @@ func TestNoWatches(t *testing.T) { } func TestEventOrdering(t *testing.T) { + if runtime.GOOS == "windows" { + // https://qualapps.blogspot.com/2010/05/understanding-readdirectorychangesw_19.html + t.Skip("Windows doesn't make great guarantees about duplicate/out-of-order events") + return + } f := newNotifyFixture(t) defer f.tearDown() @@ -143,10 +145,7 @@ func TestWatchesAreRecursive(t *testing.T) { f.events = nil // change sub directory changeFilePath := filepath.Join(subPath, "change") - _, err := os.OpenFile(changeFilePath, os.O_RDONLY|os.O_CREATE, 0666) - if err != nil { - t.Fatal(err) - } + f.WriteFile(changeFilePath, "change") f.assertEvents(changeFilePath) } @@ -278,6 +277,9 @@ func TestSingleFile(t *testing.T) { } func TestWriteBrokenLink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no user-space symlinks on windows") + } f := newNotifyFixture(t) defer f.tearDown() @@ -292,6 +294,9 @@ func TestWriteBrokenLink(t *testing.T) { } func TestWriteGoodLink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no user-space symlinks on windows") + } f := newNotifyFixture(t) defer f.tearDown() @@ -311,6 +316,9 @@ func TestWriteGoodLink(t *testing.T) { } func TestWatchBrokenLink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no user-space symlinks on windows") + } f := newNotifyFixture(t) defer f.tearDown() @@ -399,6 +407,9 @@ func TestWatchNonexistentDirectory(t *testing.T) { f := newNotifyFixture(t) defer f.tearDown() + ignore, _ := dockerignore.NewDockerPatternMatcher(f.paths[0], []string{"./"}) + f.setIgnore(ignore) + root := f.JoinPath("root") err := os.Mkdir(root, 0777) if err != nil { @@ -422,20 +433,19 @@ func TestWatchNonexistentDirectory(t *testing.T) { } else { f.assertEvents(parent) } + f.events = nil f.WriteFile(file, "hello") - if runtime.GOOS == "darwin" { - // mac doesn't return the dir change as part of file creation - f.assertEvents(file) - } else { - f.assertEvents(parent, file) - } + f.assertEvents(file) } func TestWatchNonexistentFileInNonexistentDirectory(t *testing.T) { f := newNotifyFixture(t) defer f.tearDown() + ignore, _ := dockerignore.NewDockerPatternMatcher(f.paths[0], []string{"./"}) + f.setIgnore(ignore) + root := f.JoinPath("root") err := os.Mkdir(root, 0777) if err != nil { @@ -469,7 +479,7 @@ func TestWatchCountInnerFile(t *testing.T) { f.assertEvents(a, b, file) expectedWatches := 3 - if runtime.GOOS == "darwin" { + if isRecursiveWatcher() { expectedWatches = 1 } assert.Equal(t, expectedWatches, int(numberOfWatches.Value())) @@ -493,7 +503,7 @@ func TestWatchCountInnerFileWithIgnore(t *testing.T) { f.assertEvents(b, file) expectedWatches := 3 - if runtime.GOOS == "darwin" { + if isRecursiveWatcher() { expectedWatches = 1 } assert.Equal(t, expectedWatches, int(numberOfWatches.Value())) @@ -514,7 +524,7 @@ func TestIgnoreCreatedDir(t *testing.T) { f.assertEvents(a) expectedWatches := 2 - if runtime.GOOS == "darwin" { + if isRecursiveWatcher() { expectedWatches = 1 } assert.Equal(t, expectedWatches, int(numberOfWatches.Value())) @@ -540,7 +550,7 @@ func TestIgnoreCreatedDirWithExclusions(t *testing.T) { f.assertEvents(a) expectedWatches := 2 - if runtime.GOOS == "darwin" { + if isRecursiveWatcher() { expectedWatches = 1 } assert.Equal(t, expectedWatches, int(numberOfWatches.Value())) @@ -563,14 +573,20 @@ func TestIgnoreInitialDir(t *testing.T) { f.assertEvents() expectedWatches := 3 - if runtime.GOOS == "darwin" { + if isRecursiveWatcher() { expectedWatches = 2 } assert.Equal(t, expectedWatches, int(numberOfWatches.Value())) } +func isRecursiveWatcher() bool { + return runtime.GOOS == "darwin" || runtime.GOOS == "windows" +} + type notifyFixture struct { - out *bytes.Buffer + ctx context.Context + cancel func() + out *bytes.Buffer *tempdir.TempDirFixture notify Notify ignore PathMatcher @@ -580,7 +596,10 @@ type notifyFixture struct { func newNotifyFixture(t *testing.T) *notifyFixture { out := bytes.NewBuffer(nil) + ctx, cancel := context.WithCancel(context.Background()) nf := ¬ifyFixture{ + ctx: ctx, + cancel: cancel, TempDirFixture: tempdir.NewTempDirFixture(t), paths: []string{}, ignore: EmptyMatcher{}, @@ -621,6 +640,11 @@ func (f *notifyFixture) rebuildWatcher() { func (f *notifyFixture) assertEvents(expected ...string) { f.fsync() + if runtime.GOOS == "windows" { + // NOTE(nick): It's unclear to me why an extra fsync() helps + // here, but it makes the I/O way more predictable. + f.fsync() + } if len(f.events) != len(expected) { f.T().Fatalf("Got %d events (expected %d): %v %v", len(f.events), len(expected), f.events, expected) @@ -639,6 +663,9 @@ func (f *notifyFixture) consumeEventsInBackground(ctx context.Context) chan erro go func() { for { select { + case <-f.ctx.Done(): + close(done) + return case <-ctx.Done(): close(done) return @@ -672,6 +699,8 @@ func (f *notifyFixture) fsyncWithRetryCount(retryCount int) { F: for { select { + case <-f.ctx.Done(): + return case err := <-f.notify.Errors(): f.T().Fatal(err) @@ -714,6 +743,7 @@ func (f *notifyFixture) closeWatcher() { for range notify.Events() { } }() + go func() { for range notify.Errors() { } @@ -721,6 +751,7 @@ func (f *notifyFixture) closeWatcher() { } func (f *notifyFixture) tearDown() { + f.cancel() f.closeWatcher() f.TempDirFixture.TearDown() numberOfWatches.Set(0) diff --git a/pkg/watch/paths.go b/pkg/watch/paths.go new file mode 100644 index 00000000..b1056e81 --- /dev/null +++ b/pkg/watch/paths.go @@ -0,0 +1,80 @@ +package watch + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/pkg/errors" + + "github.com/windmilleng/tilt/internal/ospath" +) + +func greatestExistingAncestors(paths []string) ([]string, error) { + result := []string{} + for _, p := range paths { + newP, err := greatestExistingAncestor(p) + if err != nil { + return nil, fmt.Errorf("Finding ancestor of %s: %v", p, err) + } + result = append(result, newP) + } + return result, nil +} +func greatestExistingAncestor(path string) (string, error) { + if path == string(filepath.Separator) || + path == fmt.Sprintf("%s%s", filepath.VolumeName(path), string(filepath.Separator)) { + return "", fmt.Errorf("cannot watch root directory") + } + + _, err := os.Stat(path) + if err != nil && !os.IsNotExist(err) { + return "", errors.Wrapf(err, "os.Stat(%q)", path) + } + + if os.IsNotExist(err) { + return greatestExistingAncestor(filepath.Dir(path)) + } + + return path, nil +} + +// If we're recursively watching a path, it doesn't +// make sense to watch any of its descendants. +func dedupePathsForRecursiveWatcher(paths []string) []string { + result := []string{} + for _, current := range paths { + isCovered := false + hasRemovals := false + + for i, existing := range result { + if ospath.IsChild(existing, current) { + // The path is already covered, so there's no need to include it + isCovered = true + break + } + + if ospath.IsChild(current, existing) { + // Mark the element empty fo removal. + result[i] = "" + hasRemovals = true + } + } + + if !isCovered { + result = append(result, current) + } + + if hasRemovals { + // Remove all the empties + newResult := []string{} + for _, r := range result { + if r != "" { + newResult = append(newResult, r) + } + } + result = newResult + } + } + return result +} diff --git a/pkg/watch/paths_test.go b/pkg/watch/paths_test.go new file mode 100644 index 00000000..155a9545 --- /dev/null +++ b/pkg/watch/paths_test.go @@ -0,0 +1,30 @@ +package watch + +import ( + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/windmilleng/tilt/internal/testutils/tempdir" +) + +func TestGreatestExistingAncestor(t *testing.T) { + f := tempdir.NewTempDirFixture(t) + defer f.TearDown() + + p, err := greatestExistingAncestor(f.Path()) + assert.NoError(t, err) + assert.Equal(t, f.Path(), p) + + p, err = greatestExistingAncestor(f.JoinPath("missing")) + assert.NoError(t, err) + assert.Equal(t, f.Path(), p) + + missingTopLevel := "/missingDir/a/b/c" + if runtime.GOOS == "windows" { + missingTopLevel = "C:\\missingDir\\a\\b\\c" + } + _, err = greatestExistingAncestor(missingTopLevel) + assert.Contains(t, err.Error(), "cannot watch root directory") +} diff --git a/pkg/watch/watcher_darwin.go b/pkg/watch/watcher_darwin.go index b92473d2..14b4999e 100644 --- a/pkg/watch/watcher_darwin.go +++ b/pkg/watch/watcher_darwin.go @@ -6,7 +6,6 @@ import ( "github.com/pkg/errors" - "github.com/windmilleng/tilt/internal/ospath" "github.com/windmilleng/tilt/pkg/logger" "github.com/windmilleng/fsevents" @@ -72,14 +71,6 @@ func (d *darwinNotify) loop() { // Add a path to be watched. Should only be called during initialization. func (d *darwinNotify) initAdd(name string) { - // Check if this is a subdirectory of any of the paths - // we're already watching. - for _, parent := range d.stream.Paths { - if ospath.IsChild(parent, name) { - return - } - } - d.stream.Paths = append(d.stream.Paths, name) if d.pathsWereWatching == nil { @@ -136,6 +127,7 @@ func newWatcher(paths []string, ignore PathMatcher, l logger.Logger) (*darwinNot stop: make(chan struct{}), } + paths = dedupePathsForRecursiveWatcher(paths) for _, path := range paths { path, err := filepath.Abs(path) if err != nil { diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index 2c61b721..c015e869 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -27,11 +27,12 @@ type naiveNotify struct { ignore PathMatcher log logger.Logger - watcher *fsnotify.Watcher - events chan fsnotify.Event - wrappedEvents chan FileEvent - errors chan error - numWatches int64 + isWatcherRecursive bool + watcher *fsnotify.Watcher + events chan fsnotify.Event + wrappedEvents chan FileEvent + errors chan error + numWatches int64 } func (d *naiveNotify) Start() error { @@ -39,18 +40,29 @@ func (d *naiveNotify) Start() error { return nil } - for name := range d.notifyList { + pathsToWatch := []string{} + for path := range d.notifyList { + pathsToWatch = append(pathsToWatch, path) + } + + pathsToWatch, err := greatestExistingAncestors(pathsToWatch) + if err != nil { + return err + } + if d.isWatcherRecursive { + pathsToWatch = dedupePathsForRecursiveWatcher(pathsToWatch) + } + + for _, name := range pathsToWatch { fi, err := os.Stat(name) if err != nil && !os.IsNotExist(err) { return errors.Wrapf(err, "notify.Add(%q)", name) } - // if it's a file that doesn't exist, watch its parent + // if it's a file that doesn't exist, + // we should have caught that above, let's just skip it. if os.IsNotExist(err) { - err = d.watchAncestorOfMissingPath(name) - if err != nil { - return errors.Wrapf(err, "watchAncestorOfMissingPath(%q)", name) - } + continue } else if fi.IsDir() { err = d.watchRecursively(name) if err != nil { @@ -70,6 +82,14 @@ func (d *naiveNotify) Start() error { } func (d *naiveNotify) watchRecursively(dir string) error { + if d.isWatcherRecursive { + err := d.add(dir) + if err == nil || os.IsNotExist(err) { + return nil + } + return errors.Wrapf(err, "watcher.Add(%q)", dir) + } + return filepath.Walk(dir, func(path string, mode os.FileInfo, err error) error { if err != nil { return err @@ -99,24 +119,6 @@ func (d *naiveNotify) watchRecursively(dir string) error { }) } -func (d *naiveNotify) watchAncestorOfMissingPath(path string) error { - if path == string(filepath.Separator) { - return fmt.Errorf("cannot watch root directory") - } - - _, err := os.Stat(path) - if err != nil && !os.IsNotExist(err) { - return errors.Wrapf(err, "os.Stat(%q)", path) - } - - if os.IsNotExist(err) { - parent := filepath.Dir(path) - return d.watchAncestorOfMissingPath(parent) - } - - return d.add(path) -} - func (d *naiveNotify) Close() error { numberOfWatches.Add(-d.numWatches) d.numWatches = 0 @@ -141,6 +143,17 @@ func (d *naiveNotify) loop() { continue } + if d.isWatcherRecursive { + if d.shouldNotify(e.Name) { + d.wrappedEvents <- FileEvent{e.Name} + } + continue + } + + // If the watcher is not recursive, we have to walk the tree + // and add watches manually. We fire the event while we're walking the tree. + // because it's a bit more elegant that way. + // // TODO(dbentley): if there's a delete should we call d.watcher.Remove to prevent leaking? err := filepath.Walk(e.Name, func(path string, mode os.FileInfo, err error) error { if err != nil { @@ -239,8 +252,14 @@ func newWatcher(paths []string, ignore PathMatcher, l logger.Logger) (*naiveNoti return nil, err } + err = fsw.SetRecursive() + isWatcherRecursive := err == nil + wrappedEvents := make(chan FileEvent) notifyList := make(map[string]bool, len(paths)) + if isWatcherRecursive { + paths = dedupePathsForRecursiveWatcher(paths) + } for _, path := range paths { path, err := filepath.Abs(path) if err != nil { @@ -250,13 +269,14 @@ func newWatcher(paths []string, ignore PathMatcher, l logger.Logger) (*naiveNoti } wmw := &naiveNotify{ - notifyList: notifyList, - ignore: ignore, - log: l, - watcher: fsw, - events: fsw.Events, - wrappedEvents: wrappedEvents, - errors: fsw.Errors, + notifyList: notifyList, + ignore: ignore, + log: l, + watcher: fsw, + events: fsw.Events, + wrappedEvents: wrappedEvents, + errors: fsw.Errors, + isWatcherRecursive: isWatcherRecursive, } return wmw, nil diff --git a/pkg/watch/watcher_naive_test.go b/pkg/watch/watcher_naive_test.go index 35ec894b..5dab889c 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -1,4 +1,4 @@ -// +build !darwin,!windows +// +build !darwin package watch @@ -6,12 +6,17 @@ import ( "fmt" "os" "os/exec" + "runtime" "strconv" "strings" "testing" ) func TestDontWatchEachFile(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skip("This test uses linux-specific inotify checks") + } + // fsnotify is not recursive, so we need to watch each directory // you can watch individual files with fsnotify, but that is more prone to exhaust resources // this test uses a Linux way to get the number of watches to make sure we're watching