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

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)
+	f.WriteFile(changeFilePath, "change")
-	if err != nil {
-		t.Fatal(err)
-	}
 
 	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" {
+	f.assertEvents(file)
-		// mac doesn't return the dir change as part of file creation
-		f.assertEvents(file)
-	} else {
-		f.assertEvents(parent, 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,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)

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 {

pkg/watch/watcher_naive.go

@@ -27,11 +27,12 @@ type naiveNotify struct {
 	ignore PathMatcher
 	log    logger.Logger
 
-	watcher       *fsnotify.Watcher
+	isWatcherRecursive bool
-	events        chan fsnotify.Event
+	watcher            *fsnotify.Watcher
-	wrappedEvents chan FileEvent
+	events             chan fsnotify.Event
-	errors        chan error
+	wrappedEvents      chan FileEvent
-	numWatches    int64
+	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)
+			continue
-			if err != nil {
-				return errors.Wrapf(err, "watchAncestorOfMissingPath(%q)", name)
-			}
 		} 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,
+		notifyList:         notifyList,
-		ignore:        ignore,
+		ignore:             ignore,
-		log:           l,
+		log:                l,
-		watcher:       fsw,
+		watcher:            fsw,
-		events:        fsw.Events,
+		events:             fsw.Events,
-		wrappedEvents: wrappedEvents,
+		wrappedEvents:      wrappedEvents,
-		errors:        fsw.Errors,
+		errors:             fsw.Errors,
+		isWatcherRecursive: isWatcherRecursive,
 	}
 
 	return wmw, nil

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