Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kqueue emit Create Events and watch all files in a directory #372

Merged
merged 2 commits into from
Dec 20, 2021
Merged

Kqueue emit Create Events and watch all files in a directory #372

merged 2 commits into from
Dec 20, 2021

Conversation

xanderio
Copy link
Contributor

@xanderio xanderio commented Dec 7, 2021

This PR fixes some shortcoming in the kqueue backend.

  • The kqueue backend started as a copy of the inotify backend.
    One major difference between kqueue and inotify was overlook by me,
    inotify reports events for changes to files contained in a folder if the
    folder is watched. Kqueue dosen't do this, each file has to be watch
    individually.

  • As kqueue dosen't provide this information directly we have to do some
    trickery, that is keep a list of known file in a directory and compare
    it to the directory after a Write event was emitted for that directory.

This fixes #365

The kqueue backend started as a copy of the inotify backend.
One major difference between kqueue and inotify was overlook by me,
inotify reports events for changes to files contained in a folder if the
folder is watched. Kqueue dosen't do this, each file has to be watch
individually.

This change fixes this, events are now reported for all files in a
recursive watch.
Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for starting this. At best we'd add some kind of watcher restart feature, such that an invalid watcher state can be recovered from instead of rebuilding the whole watcher.

src/kqueue.rs Outdated Show resolved Hide resolved
src/kqueue.rs Outdated Show resolved Hide resolved
src/kqueue.rs Outdated Show resolved Hide resolved
Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks for investigating!

@0xpr03
Copy link
Member

0xpr03 commented Dec 13, 2021

Sadly the CI isn't happy https://github.com/notify-rs/notify/runs/4487903445?check_suite_focus=true
The error types don't seem to align.

As kqueue dosen't provide this information directly we have to do some
trickery, that is keep a list of known file in a directory and compare
it to the directory after a Write event was emitted for that directory.

This change fixes #365.
@xanderio
Copy link
Contributor Author

That issue should be fixed now, totally missed it. A simple cargo check did reveal it, luckily there the CI :D

@xanderio
Copy link
Contributor Author

@0xpr03 is there anything preventing this from getting merged?

@0xpr03
Copy link
Member

0xpr03 commented Dec 20, 2021

Sorry it's just on me to find the time

@0xpr03 0xpr03 merged commit adbbd33 into notify-rs:main Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Event for kqueue missing
2 participants