fix: Updated code using deprecated FSEventStreamScheduleWithRunLoop

[dploeger]

fixes #59

What does this pull request do?

It removes the call to FSEventStreamScheduleWithRunLoop which is deprecated and produces compiler warnings.

Where should the reviewer start?

Check if the changed code makes sense.

How should this be manually tested?

Good question. I just tested it using the integrated test suite and I don't really know what this module does, but a dependency of mine is using it and I wanted to get rid of the warning. 😄

[kostyay]

Any way to merge this?
Major docker modules still rely on fsevents and without this fix there is an annoying compilation error on Max OS X

[arp242]

"I don't really know what this module does, but a dependency of mine is using it and I wanted to get rid of the warning" doesn't exactly inspire great confidence in its correctness.

No one is really maintaining this, and while I don't mind merging things that have some signals that they're correct, tested, etc. this is not really that.

[pbnjay]

Yeah it's been a bit since I did FSEvents work, but it looks to me that the CFRunLoop reference in the struct should also be dropped throughout and replaced with the dispatch queue ref. The stop() call needs to also unschedule from the dispatch queue by setting it to NULL.

It would probably also be better to use a NULL label for the dispatch queue since the allocation/reference doesn't add any useful information (and may likely make debugging multi-watcher code more difficult).

I can tackle it next week when I've got a bit of free time unless someone else jumps in.

[arp242]

Sure, I'll merge anything as long as there's a signal that it's probably okay and not going to break things. There are very few tests here and no tests for edge cases. I never even looked (or ran) the code so I don't really know just from the patch – I just "accidentally" maintain this as part of fsnotify and experience with that shows that there are tons of edge cases and ways to break people's usage.

For something like this, which in spite of being a short patch is at the core of the functionality, this would be something like "I ran the code on my machine for an application I use and confirmed it still works fine over the period of a few days", "I'm experienced with macOS dev/FSEvents and this is the right way to solve this", "here's another project in $lang which did exactly the same and that worked for them", or something else. Certainly something more than "I don't know what this does but this makes the compile warning go away".

And a compile warning is perhaps a bit annoying, but really not a big deal. Certainly less of a deal than "your change broke my application".

[philc]

One small data point to add: I've been using this PR in a personal project -- an always-on background process for an app that's used every day by two people -- for the last 60 days. I've had no issues with fsnotify.

[arp242]

Also: #62

[arp242]

@pbnjay do you have any opinion on whether this patch or #62 is better? I go the CI back up running and @emanuel-skrenkovic is working on actually adding some tests, so we can merge something, but it's really not obvious to me which patch is better.

[pbnjay]

Since this PR still includes the RunLoop code and the Dispatch queues I think it will be difficult to maintain even if it doesn't exhibit issues yet. #62 matches what I would have done, I left a minor comment there.

Thank you for putting things in motion so we could get this updated @dploeger!

[arp242]

Fixed via #62.