search

target / lorri

Your project's nix-env
Apache License 2.0
992 stars 69 forks source link

Improve readDir watches #152

Open roberth opened 5 years ago

roberth commented 5 years ago

Lorri seems to watch directories recursively when it detects builtins.readDir. This seems unnecessary because readDir only returns the direct children. Any use of those children should be detectable with existing means.

Profpatsch commented 5 years ago

If you had written how you noticed this we might have figured this and that a bit sooner :)

roberth commented 5 years ago

Catching a few files too much didn't seem that bad, but I agree in general more info is good.

Profpatsch commented 5 years ago

Okay, so what needs to be done here:

This needs to be split into two constructors, one for files and one for dirs (and logged-evaluation.nix changed to print different things):

https://github.com/target/lorri/blob/0b06b7ebe381c00ee69bf54830c3b85f45576727/src/builder.rs#L330-L333

Then we need some more logic here:

https://github.com/target/lorri/blob/0b06b7ebe381c00ee69bf54830c3b85f45576727/src/builder.rs#L156-L158

cc @curiousleo

curiousleo commented 5 years ago

This needs to be split into two constructors, one for files and one for dirs (and logged-evaluation.nix changed to print different things)

I think there may be a "smaller diff" solution to the problem at hand: just don't watch directories recursively. Note that when watching a directory, notify will still tell us when any of its children are updated:

If recursive_mode is RecursiveMode::Recursive events will be delivered for all files in that tree. Otherwise only the directory and its immediate children will be watched.

https://docs.rs/notify/5.0.0-pre.1/notify/trait.Watcher.html#tymethod.watch

"only the directory and its immediate children" seems to be exactly what is needed here.

My Nix-foo isn't good enough to be 100% sure that this is the correct behaviour, but according to what @roberth said about readDir, it seems fine ...? Draft PR here: https://github.com/target/lorri/pull/232.