nix-community / lorri

Your project’s nix-env [maintainer=@Profpatsch,@nyarly]
Apache License 2.0
638 stars 24 forks source link

Add nix file to watcher immediately #41

Closed Profpatsch closed 3 years ago

Profpatsch commented 3 years ago

I noticed that lorri would not pick up any filesystem changes if the first nix build fails after starting to watch a project; we didn’t actually add the nix file itself to the watcher before the first build, it was picked up implicitly by the first evaluation output.

Thus if nix failed on the first run, the build loop would never be triggered again by any filesystem events, and pings from lorri direnv don’t work for this anymore, since they only trigger a rebuild if the project is not yet watched.

cc @symphorien @sternenseemann

Profpatsch commented 3 years ago

So what this doesn’t fix is e.g.

shell.nix:
import ./foo.nix
foo.nix:
<syntax error>

^ this won’t ever rebuild, not even if you fix foo.nix, unless you touch shell.nix :(

I don’t know how to best handle it, I feel like we should really best-effort add paths to the watcher, even for failing nix builds?

symphorien commented 3 years ago

WIth your reproducer, nix-instantiate -vv still yields interesting logs:

$  nix-instantiate -vv shell.nix
evaluating file '/nix/store/3xar9pka6mfq44d640znj6y0qd71x4a2-nix-2.3.10/share/nix/corepkgs/derivation.nix'
evaluating file '/tmp/foo.nix'
error: syntax error, unexpected '!', expecting $end, at /tmp/foo.nix:1:6

We still get the dependency to foo.nix.

So maybe a more general solution would be to parse the log of nix-instantiate for files to watch even when nix-instantiate fails.

In case nix-instantiate fails earlier (permission error on shell.nix maybe?) your solution may be additionally needed.

What do you think?

Profpatsch commented 3 years ago

So maybe a more general solution would be to parse the log of nix-instantiate for files to watch even when nix-instantiate fails.

I thought we were doing that already, but maybe I misremembered

Profpatsch commented 3 years ago

@symphorien I would merge this as-is, maybe we can improve it further? I don’t think it hurts to always add the nix file anyway, then we don’t depend on the nix output for that.