use FSEventStreamSetDispatchQueue instead of deprecated FSEventStreamScheduleWithRunLoop

[emanuel-skrenkovic]

Hello!

FSEventStreamScheduleWithRunLoop is deprecated and triggers compile warnings so I basically made this change for myself to get rid of the warning.

Wanted to open the create the PR in case this is actually a useful change to have. Feel free to close the PR if the change is not needed.

What does this pull request do?

Replaces the deprecated FSEventStreamScheduleWithRunLoop with the suggested FSEventStreamSetDispatchQueue.
Also removes the run loop part, as FSEventStreamSetDispatchQueue handles it itself using a separate, hidden thread.
The behavior remains the same - calling Start is non-blocking.

Helpful discussion I found on the web:
https://www.spinics.net/lists/git/msg452085.html

For FSEventStreamScheduleWithRunLoop the docs say:
Introduced in macOS 10.5 and deprecated in macOS 13.0

FSEventStreamSetDispatchQueue the docs say:
Available on macOS 10.6 and later

Seems like macOS 10.5 gets shafted by this change but only that version.

Where should the reviewer start?

wrap.go

The test TestBasicExample tests that the code actually works, but all the tests here are very basic, and frankly, I don't have enough understanding of this to prove this doesn't break something.

https://github.com/docker/compose has a direct dependency on fsevents, and I tried running all their tests using my fsevents code and nothing seemed to break. Not sure that the right code paths were executed though.

How should this be manually tested?

I do not know, to be honest. I just modified the TestBasicExample to create a bunch of files and events seemed to be emitted properly.

Have a nice day!

[arp242]

The test TestBasicExample tests that the code actually works, but all the tests here are very basic, and frankly, I don't have enough understanding of this to prove this doesn't break something.

Yes, this was also my concern with #61, and why I haven't merged that.

Personally what I'd do is copy the tests from fsnotify; the testdata directory has test scripts that can probably be ported fairly easily. They're run with TestScript() and parsed with helpers_test.go. Just need to update things like NewWatcher() with whatever fsevents has for this and then, hopefully, it should work reasonably well without too much effort.

I fixed the CI in #63, so at least that should run again.

[arp242]

Seems like the CI fails because of a race condition in the test; you need to either protect events with a mutex, or return in that goroutine that collects the events before you loop over the events.

[pbnjay]

This looks good to me, one minor quibble is the addition of DispatchQueueRelease which adds a layer of indirection. Since this PR uses dispatch_queue_create directly I think we can just use dispatch_release directly as well and simplify the code paths. (The other C wrappers do C pointer manipulation so they make sense)

[emanuel-skrenkovic]

This looks good to me, one minor quibble is the addition of DispatchQueueRelease which adds a layer of indirection. Since this PR uses dispatch_queue_create directly I think we can just use dispatch_release directly as well and simplify the code paths. (The other C wrappers do C pointer manipulation so they make sense)

Thanks for looking at the code!

The reason I did the indirection is because of types - Golang won't allow me to treat dispatch_queue_t (which is returned from dispatch_queue_create) as dispatch_object_t (which dispatch_release takes in).

Passing the argument in C code allows for implicit type conversion.

I don't have too much experience with cgo so I could be missing something. I would've preferred to avoid indirection if possible, so if there's a way, I'll change it. :)

Tried conversion and ends up in this :(
cannot convert a (variable of type _Ctype_dispatch_queue_t) to type _Ctype_dispatch_object_t

And passing the dispatch_queue_t into dispatch_release ends up with this:
cannot use a (variable of type _Ctype_dispatch_queue_t) as _Ctype_dispatch_object_t value in argument to (_Cfunc_dispatch_release)

[pbnjay]

Aha that makes sense, I missed that. Could wrap the create call for more symmetry but I don’t feel as strongly there.

LGTM!

[pbnjay]

LGTM

[arp242]

Thanks! I merged it and tagged v0.2.0

[emanuel-skrenkovic]

Thanks for all the help!