nix icon indicating copy to clipboard operation
nix copied to clipboard

Clean up daemon handling

Open Ericson2314 opened this issue 3 years ago • 14 comments

Split common.sh into the vars and functions definitions vs starting the daemon (and possibly other initialization logic). This way, init.sh can just source the former. Trying to start the daemon before nix.conf is written will fail because nix daemon requires --experimental-features 'nix-command'.

killDaemon is idempotent, so it's safe to call when no daemon is running.

startDaemon and killDaemon use the PID (which is now exported to subshells) to decide whether there is work to be done, rather than NIX_REMOTE, which might conceivably be set differently even if a daemon is running.

startDaemon and killDaemon can save/restore the old NIX_REMOTE as NIX_REMOTE_OLD.

init.sh kills daemon before deleting everything (including the daemon socket).

init.sh is tested on its own. We used to do that. I deleted it in 4720853129b6866775edd9f90ad6f10701f98a3c but I am not sure why. Better to just restore it; at one point working on this every other test passed, so seems good to check whether init.sh can be run twice.

~~depends on #6179~~

Ericson2314 avatar Dec 09 '21 15:12 Ericson2314

What was the issue here? I re-run init.sh all the time without any problems.

edolstra avatar Dec 09 '21 19:12 edolstra

@edolstra At one point everything was passing but init.sh. This has now grown a lot into fixing various issues. (I am about to push again.)

Ericson2314 avatar Dec 09 '21 19:12 Ericson2314

OK updated with lots more stuff. Curious if CI will agree with me, but seems good locally.

Ericson2314 avatar Dec 09 '21 19:12 Ericson2314

macOS build timed it out?

Ericson2314 avatar Dec 09 '21 21:12 Ericson2314

Probably the flaky build issue in #3605

abathur avatar Dec 11 '21 16:12 abathur

Thanks

Ericson2314 avatar Dec 12 '21 00:12 Ericson2314

Rebased, hoping won't get that spurious error this time.

Ericson2314 avatar Dec 14 '21 22:12 Ericson2314

Yay it did!

Ping @edolstra and @regnat

Ericson2314 avatar Dec 14 '21 22:12 Ericson2314

@edolstra Does this look good to you? I would like to start pushing on this and other testing improvements again prior to getting back to fun feature work like RFC 92, so we have a firmer foundation before any more (potentially destabilizing) new feature.

Ericson2314 avatar Jan 23 '22 22:01 Ericson2314

@thufschmitt I had thought about that, but I decided you (probably) had an intent of init.sh just preparing files and not running processes, and I decided that was a good thing :). I forget the exact reasons why.

Ericson2314 avatar Feb 28 '22 16:02 Ericson2314

@thufschmitt rebased!

Ericson2314 avatar Feb 28 '22 17:02 Ericson2314

:tada: All dependencies have been resolved !

dpulls[bot] avatar Feb 28 '22 18:02 dpulls[bot]

@edolstra @thufschmitt approved. Is this OK with you?

Ericson2314 avatar Mar 09 '22 03:03 Ericson2314

@edolstra pinging again. It really would be good to merge this. It doesn't commit us to doing any more testing, it just makes the existing testing more robust.

Ericson2314 avatar Mar 25 '22 02:03 Ericson2314

I was inspired to return to this because #7600 has a devilishly hard to debug issue because it only cropped up in the daemon tests, and I was reminded how flaky that stuff is today.

In the process I found that the history had become quite messy, and so I am cleaning it up. ~~This is not just the first commit in what was a few cleanups. So the previous reply history may talk about changes that are no longer in here.~~ Ah I see that the original change probably didn't work on its own.

Ericson2314 avatar Jan 16 '23 19:01 Ericson2314

@thufschmitt Small reminder on reviewing this :). @roberth and I last night discussed some good ideas to clean stuff up further, and avoid the profusion of support files (to which this PR sadly adds one more). But I think it is good to land this in the meantime; having the daemon start and stop logic be more robust will make further cleanups easier.

Ericson2314 avatar Feb 08 '23 13:02 Ericson2314

Well, now there are some conflicts :upside_down_face:

thufschmitt avatar Feb 23 '23 06:02 thufschmitt

Thanks @thufschmitt. For some reason when I rebased --- no conflicts!

Ericson2314 avatar Feb 23 '23 16:02 Ericson2314