fix(inotify): watch arbitrary depth

[rgrinberg]

This PR fixes the inotify backend to allow watching x/y/z even if y (or x) do not exist. The fix works by watching the topmost directory that exists and then adding more watches as descendants are created. After adding a watch, we rescan the directory (as per the manual) because some events might have already occurred in that directory.

[rgrinberg]

friendly ping @snowleopard

[snowleopard]

@rgrinberg There is some logic in fs_memo.ml that (partially) addresses this issue:

dune/src/dune_engine/fs_memo.ml

Lines 59 to 89 in 8e155da

	(* CR-someday aalekseyev: For [watch_path] to work correctly we need to ensure
	that the parent directory of [path] exists. That's certainly not guaranteed
	by the [Fs_memo] API, so we should do something to make it more robust, but
	I believe that is masked by the fact that we usually (always?) look at the
	source directory before looking for files in that directory.
	It might seem that the [`Does_not_exist] "fall back to the containing dir"
	trick used below can be extended to fall back all the way to the root, but
	it can't be because watching the root is not sufficient to receive events
	for creation of "root/a/b/c" -- for that we need to watch "root/a/b". *)
	let watch_path watcher path =
	match Dune_file_watcher.add_watch watcher path with
	| Ok () -> ()
	| Error `Does_not_exist -> (
	(* If we're at the root of the workspace (or the Unix root) then we can't
	get [`Does_not_exist] because Dune can't start without a workspace and
	the Unix root always exists. Hence, the [_exn] below can't raise,
	except if the user deletes the workspace directory under our feet, in
	which case all bets are off. *)
	let containing_dir = Path.parent_exn path in
	(* If the [path] is absent, we need to wait for it to be created by
	watching the parent. We still try to add a watch for the [path] itself
	after that succeeds, in case the [path] was created already before we
	started watching its parent. *)
	(match Dune_file_watcher.add_watch watcher containing_dir with
	| Ok () -> ()
	| Error `Does_not_exist ->
	Log.info
	[ Pp.textf "Attempted to add watch to non-existent directory %s."
	(Path.to_string containing_dir)
	]);

Is this not sufficient for you, i.e. are you hitting the Attempted to add watch to non-existent directory path?

[rgrinberg]

Thanks for the pointer, we should get rid of that workaround now I suppose.

Is this not sufficient for you, i.e. are you hitting the Attempted to add watch to non-existent directory path?

Yes, this is easy to hit. But also, note that the macos does not have this limitation at all. So this PR just addresses the inconsistency between the two backends.

[snowleopard]

@rgrinberg Right, makes sense. Let's drop this workaround then.

[rgrinberg]

The workaround is gone.

[rgrinberg]

ping @snowleopard

[snowleopard]

ping @snowleopard

Sorry, I'm too out of breath for reviewing subtle stuff like this. Need to find some calm time -- will try to review by end of week!

[rgrinberg]

@snowleopard this week perhaps?

[snowleopard]

to_scan might as well be a list to avoid this to_list.

[snowleopard]

The assumption here seems to be that the path got created. Maybe worth adding an assert about this?

[snowleopard]

Actually, it doesn't seem great that we're doing all this for every event, not just for events that correspond to path creations. I think we shoud refactor this logic so that inotify.awaiting_creation and the associated mutex is only touched when necessary. (There should probably be a separate function that processes path creation events and returns a list of paths that need to be scanned.)

[snowleopard]

The order of arguments in @ seems weird, almost guaranteeing quadratic complexity if we assume that acc grows from a large number of small processed lists. Should this be swapped?

[snowleopard]

(Same for events.)

[snowleopard]

Add a brief comment why this Fdecl is needed?

[snowleopard]

Same architectural concern here: messing around with mutex for every watch, even though most watches don't care about the awaiting_creation logic, seems wrong.

[snowleopard]

I think we should print something more informative here. My understanding is that we can only end up here if p is the workspace root and it doesn't exist? I guess this can only mean that the user deleted the directory while Dune was running. If you agree that that's the only situation where we can reach this code path, let's just say this.

[snowleopard]

Could you also add a test where we move a whole tree (say, with 2 levels and 2 subdirectories in each node) into a watched place? I'd like to make sure we can correctly deal with moves and trees (in addition to creating sequences of nested paths as tested above).

[snowleopard]

Thanks, it's a good improvement but I'm a bit unhappy that we are polluting happy/typical code paths with unhappy logic -- can we structure the code in a way that makes it obvious that the mutex and friends are only needed for watching paths that don't exist?