Rebase2 #5
Draft
Rebase2 #5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ows (fsnotify#289)" This reverts commit e2e9517.
* Update issue templates * remove old issue template
* Test on Go 1.18 and two most recent versions * on push * ci
This would have caught fsnotify#389 and should catch build failures for supported targets.
In newEmptyPoller when creating the poller using newEmptyPoller(fd), the fd field of the poller instance is already set. There is no need to set it again.
It would be good to re-enable tests on PRs.
…#422) * Change 1ms sleeps to 50ms * Use const variable names to indicate why we are sleeping
In cases where fsnotify is a transitive dependency of a package, it might need to be built on platforms not supported (yet), e.g. aix. Provide an empty implemention for these cases, so depending packages don't need to add workarounds. Replaces fsnotify#314
…MAX_PATH (fsnotify#361) * Fix crash on windows if raw.FileNameLength exceeds syscall.MAX_PATH * Add comment * Update windows.go
…tored (fsnotify#374) * Add a feature to return the directories and files that are being monitored * add WatchList() method for bsd and windows platforms * preallocate space for the array to be returned
fsnotify#370) Fixes fsnotify#259 Fixes fsnotify#243 Co-authored-by: Martin Tournoij <martin@arp242.net>
* [bugfix] close handle when remWatch open in getIno * Use new tests Note: the tests pass even without the syscall.CloseHandle() Co-authored-by: jie <jie@tt.com> Co-authored-by: Martin Tournoij <martin@arp242.net>
Doesn't really test anything yet, but should do soon.
It's annoying as I need to keep deleting the template, and it doesn't provide that much value IMO.
Not really needed anymore as far as I can tell: fsnotify#193 (comment)
…ify#484) We need to update x/sys to support GOARCH=loong64, which is new in Go 1.19. Also kick off fewer builds; they regularly get hung in the queue as we have quite a few test jobs now.
People are running in to trouble because the 4K buffer can overflow; see: fsnotify#72. This is not a "real" fix, but I think a 64K buffer is acceptable even on memory-limited machines; no one is running the Windows on an Arduino, and even with something like 128M of memory, the extra 124K is basically negligible. There is also no real performance difference between allocating a large array vs. a small array: they're both comparable. It should probably be enough for most applications. Need to look in the future to either dynamically grow the size, or allow setting it similar to what tilt does as mentioned in fsnotify#72.
Specifically to address fsnotify#364; syscall.GetQueuedCompletionStatus() contains a bug that can't be fixed in a backwards-compatible way, so they didn't. Once you use sys/windows for one thing, you gotta use it for everything. I ran the tests with -count=100 locally, and it all seems fine. Fixes fsnotify#364
This was added in 00f2dfd (2011): "BSD Test: Need to use outside program to trigger some events". All tests pass fine with just os.Rename(), so just use that. Might add another real-world test in the future for common tools like mv, cp, vi, etc. because they don't always behave the same as library calls, but calling mv in *only* the mv() helper is rather odd. This also renames a few things in the kqueue helper for clarity, and uses map[string]struct{} if we just use them for lookups to an allocation (I originally tried to fix fsnotify#488, but gave up on that for now, hence the kqueue changes).
…fy#490) This Skip() was added in 2013 (c825c6d) with "Windows cannot overwrite file on rename", but it seems to work fine. os.Rename() is also documented as "If newpath already exists and is not a directory, Rename replaces it", so if it *doesn't* work it's a bug in os.Rename(). Maybe os.Rename() behaved different nine years ago.
Symlinks are supported since Windows Vista; there's no reason to not test it.
The timeout for unix.Kevent() is causing issues; every 100ms it will do a new unix.Kevent() syscall, which isn't too efficient: even if you have just one change an hour, you will still keep calling kevent() ten times per second, resulting in a needlessly high CPU usage. Without a timeout, kevent() will block indefinitely until there are some events, which is much more efficient. We can't just remove the timout however, since we can't interrupt the kevent() call on FreeBSD and NetBSD, and it will hang forever. This issue is described in more detail here: fsnotify#262 (comment) To solve this we register a new kevent() with the file descriptor set to the closepipe; when this pipe is closed an event is sent and kevent() will stop blocking. This is a rebased version of fsnotify#124. Fixes fsnotify#89 Fixes fsnotify#237 Fixes fsnotify#333 Supersedes and closes fsnotify#124 Supersedes and closes fsnotify#262 Supersedes and closes fsnotify#334 Co-authored-by: Felix Lange <fjl@twurst.com>
Watching an entire directory would fail if it contains a file we can't read due to permission errors. That's obviously not ideal, so just skip the file. Also add a test for trying to watch a directory where the permissions are denied, and test the error is what's expected; this prevents regressions like in fsnotify#393. Fixes fsnotify#439
Add a helper function instead of doing a select on w.done and w.Errors or w.Events every time. Also move most functions to methods on the Watcher.
The sys* constants were used in the old golang.org/x/exp/winfsnotify package on which this is based, but haven't been in use for a long time. Remove the "one shot", "dir only", and "ignore" options as they were never set. Also don't send "sysFSQOVERFLOW" as an event if the buffer is too long; we already send an error and it's just a weird magical number you get. This could be simplified greatly more by removing the whole sysFS… → windows.FILE_NOTIFY… → sysFS… conversions, but that's for another day.
Also remove the copyright headers on the files; the license text doesn't require them, forbids removing them, and they add nothing IMHO, and it's a few more lines to scroll past every time you open a file.
This will be useful once we start splitting out "systems" and "backends" a bit more, e.g. when polling support is added.
…eaned up after Remove (fsnotify#494) This moves a number of tests from backend_inotify_test.go to fsnotify_test.go, as they're not really inotify-specific. In particular, it moves the "stress test", which creates a bunch of events/files. It also expands this test to (potentially) create many more than just 1,000 files, depending on how many the system will allow. Unfortunately these tests seem pretty flaky on kqueue platforms, where they're allowed to fail for now (failing the test won't fail the test run). This seems to expose some existing limits/problems that need to fixed in a future PR. Also test that the internal state is cleaned up with TestRemoveState(). The Windows backend doesn't have a test for it (or rather, it doesn't run) as it *doesn't* clean the state properly, but I found it too confusing to fix 🤷 Need to spend some time on that in the future. Reorder/rename some GitHub Actions test runs to show nicer in the UI. Fixes fsnotify#42 Fixes fsnotify#268
Update some documentation and add various examples. I added the examples as subcommands of ./cmd/fsnotify, so they're directly runnable, and it's at least guaranteed they compile. This adds some simpler test cases so it's easier to verify it actually works as documented on all platforms, and it adds internal.Debug() for all platforms, which is useful in dev. Fixes 49 Fixes 74 Fixes 94 Fixes 122 Fixes 238 Fixes 372 Fixes 401
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: Please do not open a pull request that adds or changes the public API without giving consideration to all supported operating systems.
What does this pull request do?
Where should the reviewer start?
How should this be manually tested?