Use ocaml inotify

[aalekseyev]

This PR makes dune use inotify library directly instead of relying on an external process (inotifywait).
The reasons for this are:

• This removes one opaque indirection in reasoning about inotify semantics, putting it under our control. (inotify is already complicated, inotifywait adds its own quirks to the mix).
• It lets us express interest in a part of the workspace that we're working with. With inotifywait you have to subscribe to the entire workspace, which uses more inotify watches and requires a full workspace scan at startup to establish those watches.

[ghost]

I'm trying to understand how async_inotify communicates with the scheduler.

IIUC, when async_inotify receive some data from inotify, it sends a (unit -> unit) job to the scheduler via File_watcher_task. This job:

• processes the data received from inotify and produce a list of Async_inotify.Event.t
• sends these events one by one to the scheduler via the emit_event callback
• this callback, provided by dune_file_watcher, packs the event into a list of a single element and adds this list to the event queue

So zooming out, when we receive an inotify event:

• async_inotify sends a (unit -> unit) callback to the scheduler
• the scheduler picks it up, execute it and this callback enqueues more things to the event queue
• the scheduler picks up the newly added events from the queue and process them

It feels like we could simplify this by making async_inotify send a unit -> Event.t list callback to the scheduler instead. WDYT?

[aalekseyev]

That is correct.
I agree we should be able to emit a batch of events instead of doing it one by one.
Looking at that...

[aalekseyev]

Pushed that change. Looks better. I also split pump_events into two parts (based on which thread they run in) and added an .mli (I forgot to copy that in from jane).

[ghost]

That looks better indeed. Could we go one step further and send a (unit -> Event.t list) to the scheduler rather that send a (unit -> unit) that enqueues more events? That would remove one hoop. We might also be able to move the process_inotify_event to Dune_file_watcher, so that the scheduler never sees the low-level inotify events.

[aalekseyev]

Sounds good, trying that...

[aalekseyev]

@jeremiedimino, I pushed a commit (ba509cb) that makes this change and moves event processing code (including ignored_files) from the scheduler to file watcher. I like the direction this is going. In the current state there's still two separate functions in event queue for:

    val send_file_watcher_task : t -> (unit -> Dune_file_watcher.Event.t list) -> unit
    val send_file_watcher_events : t -> Dune_file_watcher.Event.t list -> unit

but only the former is exposed to the file watcher and we could probably get rid of it, but I thought I'd stop somewhere.

[ghost]

Sounds good

[ghost]

Scheduler.Run.Detect_external should be renamed, now that the watcher might be internal. What about Detect or Auto? The command line option takes automatic and manual FWIW, so we could also use Automatic and Manual.

[ghost]

Looks good overall, left a few suggestions.

@snowleopard you should probably read the changes to Fs_memo as well.

[aalekseyev]

Scheduler.Run.Detect_external should be renamed, now that the watcher might be internal. What about Detect or Auto? > The command line option takes automatic and manual FWIW, so we could also use Automatic and Manual.

I like "Automatic". Renaming to that. I've never liked this "manual" name, given that it's impossible to send manual events. I think I'll keep that constructor unrenamed for now because its name at least describes the true behavior.

[snowleopard]

@snowleopard you should probably read the changes to Fs_memo as well.

@jeremiedimino Thanks for the ping! Looking.

[snowleopard]

The fs_memo changes look good to me but I haven't looked much further, as I guess that got covered by @jeremiedimino's review.

[ghost]

Looks good!