watch: use the recursive watcher on windows (docker#3306)

Author: nicks

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 := &notifyFixture{
+		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,13 +743,15 @@ func (f *notifyFixture) closeWatcher() {
 		for range notify.Events() {
 		}
 	}()
+
 	go func() {
 		for range notify.Errors() {
 		}
 	}()
 }
 
 func (f *notifyFixture) tearDown() {
+	f.cancel()
 	f.closeWatcher()
 	f.TempDirFixture.TearDown()
 	numberOfWatches.Set(0)

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
+}

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")
+}

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 {