haskell-fswatch / hfsnotify

Unified Haskell interface for basic file system notifications
BSD 3-Clause "New" or "Revised" License
136 stars 40 forks source link

0.4 release #102

Closed georgefst closed 1 year ago

georgefst commented 2 years ago

tl;dr: I'm looking forward to the next release of this library, as it looks like progress is being made towards big longstanding issues.

90% of the time I've used fsnotify it's because I want to run some action each time that the contents of a file are changed. On Linux, this corresponds to IN_CLOSE_WRITE, which on current master is generating the new CloseWrite event. Nice! It would be awesome if we could have something similar working cross-platform, but I unfortunately personally don't know much about the Windows and Mac APIs.

It seems to me that the desire for debouncing when using this library comes from the fact that multiple indistinguishable events are often generated within a very short space of time. But of course, these events are* distinguishable in the OS APIs - it's just that fsnotify removes any OS-specific info. If, in the next version of fsnotify, all platforms generate a single CloseWrite event (or even just the one Modified), rather than multiple Modified events, I'd guess that no one would miss the debouncing.

PS. I'm now the maintainer of streamly-fsnotify, so I could do with a heads-up anyway when you're close to a release, so that I can work towards being compatible quickly.

* Using NoDebounce has actually made the behaviour of this library a lot clearer to me, so I'm inclined to agree that ripping it out entirely may be the best option regardless (mentioned in #55, #95). I actually had my own buggy debounce logic implemented downstream anyway, because I didn't realise the library was already doing it.

thomasjm commented 2 years ago

See also this comment: https://github.com/haskell-fswatch/hfsnotify/issues/91#issuecomment-885387940

georgefst commented 2 years ago

Hmm, looking in to this a bit more, it seems that Windows has no equivalent of CLOSE_WRITE, and downstream debouncing workarounds are almost universally necessary.

So releasing 0.4 without any debouncing support might be seen as a serious regression.

georgefst commented 2 years ago

See also this comment: #91 (comment)

The only blocker for 0.4.0.0 is that one of the breaking changes is ripping out the flawed old debounce mechanism, and replacing it with something based on this suggestion: #55 (comment).

To my mind, #55 seems like overkill anyway. Restricting debounce so that an event is only ignored if the previous event was of the same type would be a much simpler solution to that problem.

hughjfchen commented 2 years ago

I agree getting a new release is very important because the current version of the hackage is really too old and has many glitches which has been fixed in the github version.

thomasjm commented 2 years ago

releasing 0.4 without any debouncing support might be seen as a serious regression

I agree -- that's why shipping a reasonable debounce mechanism is sort of a prerequisite to getting 0.4 out the door. Different opinions might exist about what "reasonable" means. For example, your suggestion of ignoring an event if the previous event is of the same type seems pretty flawed to me, because the OS is free to insert an unrelated event in between those two things, which would cause the debouncing to fail.

But, this is why I've been working to make debouncing an orthogonal concern to the rest of this library, so people can apply the debouncing they prefer. Basically any debouncing mechanism should simply be a function with the signature Action -> IO Action.

I can see two potential ways to get a release out quickly: 1) Finish filling out the state machine for the "fancy" debouncer described here. 2) Implement something "good enough" and clearly label its limitations.

I'll try to get to this as soon as I can but PRs are always welcome :)

georgefst commented 2 years ago

For example, your suggestion of ignoring an event if the previous event is of the same type seems pretty flawed to me

Yeah, you're probably right. There's no elegant solution here, so it's a pretty thankless task. I won't rush you in to making the wrong choice!

(To be honest, my OP here is kind of an excitable, unfocussed thought dump. The shoddiness of the Windows API has pretty much ruined my plans for what I was intending to do when I started off anyway...)

istathar commented 2 years ago

I just stopped by to see if I could remove the shelly dependency only to discover you'd already done so. :) Lovely package. Let me know if you need any help?

thomasjm commented 2 years ago

@istathar want to fill in the details of a debouncing state machine? :)

istathar commented 2 years ago

@thomasjm sure, although I got the impression above that it was kinds of an optional enhancement? If that's true, then the rest of the improvements you've made probably deserve to see the light of day and this feature could wait for 0.4.1.

thomasjm commented 2 years ago

Well, as discussed above, releasing without any debouncing mechanism built-in might be seen as a regression. I haven't polled users of this library so I don't know how many depend on it. So the best thing would be to finish the proposed solution before releasing--I'm not dead set on it, but it would be nice. I just haven't been able to find time to finish it up, but if you're offering!

domenkozar commented 2 years ago

I'm hitting multiple issues with the current version, can I help somehow with denouncing? I agree with others that it's fine to do it after the bugfix release.

EDIT: https://github.com/haskell-fswatch/hfsnotify/pull/103

domenkozar commented 2 years ago

@thomasjm We are likely to release a fork in following weeks if there is not progress on this front.

This is a temporary solution to fix a number of issues we've been facing at https://github.com/cachix/cachix, but we're open to helping out in any way to get a release out.

thomasjm commented 2 years ago

What issues are you hitting?

I'd be fine with releasing the current version with rudimentary or no debouncing. Depending on what problems you're having we may also be able to backport fixes and do a bugfix release without an actual breaking change.

domenkozar commented 2 years ago

It's fine by me to bump the major version and note in the changelog that debouncing is yet to be implemented :)

thomasjm commented 2 years ago

Okay, by popular demand we'll release 0.4.0.0 as-is. Just working on getting the tests passing on all the platforms now...

thomasjm commented 1 year ago

0.4.0.0 is on Hackage now!

domenkozar commented 1 year ago

Thanks @thomasjm :heart: