Tilt watcher #10218

ndeloof · 2023-01-30T10:59:29Z

What I did
imported watcher code from tilt to support new compose watch command

(not mandatory) A picture of a cute animal, if possible in relation to what you did

codecov · 2023-01-30T11:08:18Z

Codecov Report

Base: 72.79% // Head: 73.89% // Increases project coverage by +1.10% 🎉

Coverage data is based on head (a9e8351) compared to base (f24d345).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##               v2   #10218      +/-   ##
==========================================
+ Coverage   72.79%   73.89%   +1.10%     
==========================================
  Files           2        2              
  Lines         272      272              
==========================================
+ Hits          198      201       +3     
+ Misses         62       60       -2     
+ Partials       12       11       -1

Impacted Files	Coverage Δ
pkg/e2e/framework.go	`72.15% <0.00%> (+1.17%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

thaJeztah · 2023-01-30T16:04:04Z

go.mod

@@ -33,6 +33,8 @@ require (
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.8.1
 	github.com/theupdateframework/notary v0.7.0
+	github.com/tilt-dev/fsevents v0.0.0-20200515134857-2efe37af20de
+	github.com/tilt-dev/fsnotify v1.4.8-0.20220602155310-fff9c274a375


Do we know what patches are in this fork, and if they were rejected upstream? I know fsnotify has had some time where maintenance was slow, but I think it improved in that respect (and I think we have some maintainers on it that are also maintainers for moby/moby)

https://github.com/tilt-dev/fsnotify#notable-changes

That's not quite accurate anymore, since, as you noted, fsnotify/fsnotify has had more activity recently.

In fact, I believe all the important changes have been upstreamed:

Ignore file attribute change events on Windows: fsnotify#520

Configurable Windows buffer size: fsnotify#521

Recursive Windows watcher support: fsnotify#540

However, they're all on main still - no release of fsnotify/fsnotify contains them yet. Also, not a huge deal, but there's some API differences in the upstreamed implementations, most notably in the format for Windows recursive paths.

All that said, I feel good about using upstream and pinning to a Git hash for now; that'll certainly make any future improvements more straightforward.

Ah, thanks! Yes, saw the comment later on, and saw that (github indicated "10" commits in the fork);

fsnotify/fsnotify@main...tilt-dev:fsnotify:main

Pinning to a commit from upstream SGTM (short term). I'm mostly trying to avoid having 2 forks of the same dependency (as I know we have fsnotify as dependency in our tree already).

If someone has some cycles to spare to look what patches are not (yet) in upstream, we could contribute them there.

@cpuguy83 were you a maintainer on that repo? Or do I misremember that? (Otherwise I think there's some familiar people on it, that we may try to reach out to to ask for a (pre-)release).

I think I have commit access in order to get some important changes in, but I wouldn't call myself a "maintainer".

watcher_naive relies on SetRecursive which doesn't exist in upstream

pkg/watch/pattern_matcher.go

fixes tilt-dev/tilt#3556

…ker#3743) In most places in Tilt, we try to use absolute paths everywhere. So this makes things more consistent with the rest of Tilt, and lets us be a bit more flexible in how we handle subdirs and parent dirs in ignores. Fixes tilt-dev/tilt#3740

…ocker#3960)

Unused code linter isn't particularly smart about platform build tags, so since this func is only used by the "naive" (non-macOS) file watcher, it needs to live with that or it gets flagged as dead code when linting on macOS.

`WalkDir` is new in Go 1.16 and avoids calling `os.Lstat` on every visited file and directory. In most cases, we don't need that info, so this will help reduce I/O when listing files, which can be helpful for particularly big monorepos.

* lint: add make lintfix and run it Fixes all errors like: ``` File is not `goimports`-ed with -local github.com/tilt-dev/tilt (goimports) ``` * git: change to use TrimSuffix * build: remove unnecessary cast

I've been running the test suite (`./internal/engine` in particular) with `-count X` a lot recently to catch timing-related test failures. After running enough times, the tests start failing due to too many open files, so I did an audit and found a few places where files or readers weren't always being closed.

…cker#5281) `exportloopref` - detects captures of loop variable without re-assignment NOTE: There can be false negatives with this linter to avoid being overly strict and annoying! Also upgraded `golangci-lint` to latest (v1.43.0 published 2021-11-03).

Fix local-prefixes on linters imports then resolved imports to be as expected Signed-off-by: Ken Sipe <kensipe@gmail.com>

…cker#5512)

…ocker#5769) fixes tilt-dev/tilt#5764

Signed-off-by: Nick Santos <nick.santos@docker.com> Signed-off-by: Nick Santos <nick.santos@docker.com>

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

milas · 2023-02-01T20:00:57Z

pkg/compose/watch.go

+					fmt.Fprintf(s.stderr(), "change detected on %s\n", event.Path())
+
+					for src, dest := range config.Sync {
+						if strings.HasPrefix(event.Path(), src) {


For portability, I'd look at using filepath.Rel to determine this:

It'll call Clean() on both paths so separators match (e.g. if you're on Windows but defined your ignores /a/b/c)

It has some (probably imperfect but better than nothing?) case-insensitivity logic for Windows

Or perhaps IsChild from pkg/watch/paths.go which is used to filter out watches on descendants of recursively watched paths

ya +1 for using IsChild for this kind of comparison.

the things to watch out for are:

case insensitive file systems (e.g., /path/to/myrepo vs /path/to/MyRepo)

symlinks, like the /tmp -> /private/tmp symlink

so we tried to avoid string-based functions for doing this kind of check, because the path string we used as input doesn't always use the same casing as the path string that the OS attaches to the event.

Lizz and I spent some time trying to pull out a "canonical" version of paths for string comparison
https://github.com/tilt-dev/tilt/pull/5309/files
but got distracted

milas · 2023-02-01T20:03:41Z

pkg/compose/watch.go

+								Source:      event.Path(),
+								Destination: fmt.Sprintf("%s:%s/%s", service.Name, dest, event.Path()[len(src):]),


Ugh, definitely not in-scope for this PR, but we should change api.CopyOptions to use full fields for this and do the parsing in the CLI, right now we're doing that all in the backend @

compose/pkg/compose/cp.go

Lines 274 to 289 in f24d345

func splitCpArg(arg string) (container, path string) {

if system.IsAbs(arg) {

// Explicit local absolute path, e.g., `C:\foo` or `/foo`.

return "", arg

}

parts := strings.SplitN(arg, ":", 2)

if len(parts) == 1 || strings.HasPrefix(parts[0], ".") {

// Either there's no `:` in the arg

// OR it's an explicit local relative path like `./file:name.txt`.

return "", arg

}

return parts[0], parts[1]

}

this indeed is something I had in mind for a while ...

milas · 2023-02-01T20:07:24Z

pkg/watch/dockerignore.go

+func readDockerignorePatterns(repoRoot string) ([]string, error) {
+	var excludes []string
+
+	f, err := os.Open(filepath.Join(repoRoot, ".dockerignore"))


Aren't the .dockerignore loading rules more complex than this? (For example, if you have foo.dockerfile, won't it auto-use foo.dockerignore?) 😬

I don't think so. But I might be wrong. I expect users to reming me with new issues if this is the case :)

milas · 2023-02-01T20:15:31Z

pkg/watch/notify.go

+func IsWindowsShortReadError(err error) bool {
+	return runtime.GOOS == "windows" && err != nil && strings.Contains(err.Error(), "short read")
+}


FYI for once we switch off the Tilt fork...this has been superseded by ErrEventOverflow in HEAD @ fsnotify/fsnotify

milas · 2023-02-01T20:19:19Z

pkg/watch/notify.go

+	return newWatcher(paths, ignore)
+}
+
+const WindowsBufferSizeEnvVar = "TILT_WATCH_WINDOWS_BUFFER_SIZE"


This sorta ties into below re: short reads, but Tilt would detect these errors and then provide a message back to the user letting them know they should try and raise this limit.

In practice, I'm not sure how often it actually got hit (maybe @nicks knows), so it might be safe to drop configurability entirely here

Otherwise...s/TILT/COMPOSE/ 😛

people stopped filing issues about it once we put instructions on how to embiggen the buffer in the error message!! 😅

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

ndeloof force-pushed the tilt-watcher branch 11 times, most recently from 83523c1 to 68ab928 Compare January 30, 2023 15:52

thaJeztah reviewed Jan 30, 2023

View reviewed changes

ndeloof force-pushed the tilt-watcher branch from 68ab928 to fda136c Compare January 30, 2023 16:15

thaJeztah reviewed Jan 30, 2023

View reviewed changes

pkg/watch/pattern_matcher.go Outdated Show resolved Hide resolved

ndeloof force-pushed the tilt-watcher branch 3 times, most recently from cc237ff to 809d6b1 Compare January 31, 2023 09:59

ndeloof marked this pull request as ready for review January 31, 2023 10:23

ndeloof force-pushed the tilt-watcher branch 2 times, most recently from 731b70d to e0d877c Compare January 31, 2023 12:35

ndeloof requested review from a team, nicksieger, StefanScherer, ulyssessouza, glours, milas and laurazard and removed request for a team January 31, 2023 13:07

tilt: copy watch code from tesseract

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

96b4229

nicks and others added 19 commits February 1, 2023 09:19

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

01db448

change org name from windmilleng to tilt-dev (docker#3346)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

4cd9fce

watch: increase the windows watch i/o buffer (docker#3620)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

eaddffd

fixes tilt-dev/tilt#3556

watch: fix a dumb errcheck (docker#3622)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

a7d537e

watch: improve error messages when you run out of inotify instances (d…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

32934d8

…ocker#3960)

test: enable unused check, remove dead code (docker#4208)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

2f98728

test: clean up test loggers a bit (docker#4580)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

ca65c8f

lint: fix local-prefixes for goimports (docker#5453)

7a2c59d

Fix local-prefixes on linters imports then resolved imports to be as expected Signed-off-by: Ken Sipe <kensipe@gmail.com>

tests: make test fixtures responsible for tearing themselves down (do…

ca976a3

…cker#5512)

watch: optimization to help avoid inotify nodes for large file trees (d…

c0f45e7

…ocker#5769) fixes tilt-dev/tilt#5764

cleanup deprecated ioutil functions (docker#5919)

ef0cbb2

Signed-off-by: Nick Santos <nick.santos@docker.com> Signed-off-by: Nick Santos <nick.santos@docker.com>

adjust code and dependencies

3bf3051

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

use tilt watcher to track filesystem changes

4a3f38b

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

ndeloof force-pushed the tilt-watcher branch 2 times, most recently from 47015e9 to fea472e Compare February 1, 2023 09:14

milas approved these changes Feb 1, 2023

View reviewed changes

ndeloof force-pushed the tilt-watcher branch 3 times, most recently from 554ba97 to 18930bb Compare February 2, 2023 10:37

initial support for sync

a9e8351

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

ndeloof force-pushed the tilt-watcher branch from 18930bb to a9e8351 Compare February 2, 2023 10:42

ndeloof merged commit 1640f15 into docker:v2 Feb 2, 2023

ndeloof deleted the tilt-watcher branch February 2, 2023 13:59

ramnes mentioned this pull request Mar 6, 2023

Watch code and automatically rebuild when something changes #184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tilt watcher #10218

Tilt watcher #10218

ndeloof commented Jan 30, 2023 •

edited

Loading

codecov bot commented Jan 30, 2023 •

edited

Loading

thaJeztah Jan 30, 2023

milas Feb 1, 2023

thaJeztah Feb 1, 2023

cpuguy83 Feb 1, 2023

ndeloof Feb 2, 2023

milas Feb 1, 2023

milas Feb 1, 2023

nicks Feb 1, 2023 •

edited

Loading

milas Feb 1, 2023

ndeloof Feb 2, 2023

milas Feb 1, 2023

ndeloof Feb 2, 2023

milas Feb 1, 2023

milas Feb 1, 2023

nicks Feb 1, 2023

		Source: event.Path(),
		Destination: fmt.Sprintf("%s:%s/%s", service.Name, dest, event.Path()[len(src):]),

	func splitCpArg(arg string) (container, path string) {
	if system.IsAbs(arg) {
	// Explicit local absolute path, e.g., `C:\foo` or `/foo`.
	return "", arg
	}

	parts := strings.SplitN(arg, ":", 2)

	if len(parts) == 1 \|\| strings.HasPrefix(parts[0], ".") {
	// Either there's no `:` in the arg
	// OR it's an explicit local relative path like `./file:name.txt`.
	return "", arg
	}

	return parts[0], parts[1]
	}

Tilt watcher #10218

Tilt watcher #10218

Conversation

ndeloof commented Jan 30, 2023 • edited Loading

codecov bot commented Jan 30, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicks Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndeloof commented Jan 30, 2023 •

edited

Loading

codecov bot commented Jan 30, 2023 •

edited

Loading

nicks Feb 1, 2023 •

edited

Loading