Clean up daemon handling in the tests

[Ericson2314]

Motivation

init.sh is tested on its own. We used to do that. I deleted it in 4720853 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.

We don't need to run init.sh twice, but I want to try to make our tests as robust as possible so that manual debugging (where tests for better or worse might be run ways that we didn't expect) is less fragile.

---

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).

Context

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

• agreed on idea
• agreed on implementation strategy
• tests, as appropriate
  • functional tests - tests/**.sh
  • unit tests - src/*/tests
  • integration tests
• documentation in the manual
• code and comments are self-explanatory
• commit message explains why the change was made
• new feature or bug fix: updated release notes

[edolstra]

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

[Ericson2314]

@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]

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

[Ericson2314]

macOS build timed it out?

[abathur]

Probably the flaky build issue in #3605

[Ericson2314]

Thanks

[Ericson2314]

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

[Ericson2314]

Yay it did!

Ping @edolstra and @regnat

[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.

[tomberek]

Test failures (repair.sh.test) are unrelated. Re-based onto master and functions as expected.

[thufschmitt]

I very much like the idea behind this.

As an alternative (maybe simpler, but also maybe stupid because I didn’t give it a lot of thoughs) scheme, how about

• Making common.sh pure again (remove the startDaemon call from it)
• Start the daemon in init.sh (and source it rather than execute it in mk/run_tests.sh so that the needed variables are properly propagated)
  ?

[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]

@thufschmitt rebased!

[dpulls]

🎉 All dependencies have been resolved !

[Ericson2314]

@thufschmitt to answer the question you asked on IRC a bit better, I didn't put the starting the daemon in init for these reasons:

1. Running tests without recalling init should still work. if one wants to examine state: the test itself starts and stops the daemon, rather than something else starting and it stopping.
2. init.sh merely does cleanup and setup of files. Besides removing stuff, it just creates new blank filesystem state. Stopping the daemon before removing the files is just for any rogue daemon, before it becomes harder to stop because the socket file was deleted.

The file is split because init.sh still needs the env vars.

[thufschmitt]

Oh yes of course, it’s nice to be able to manually run init.sh once and then the test (I actually do that all the time)

[Ericson2314]

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

[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]

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]

@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.

[thufschmitt]

This has been stalled for too long, indeed. Let's merge!

[thufschmitt]

Well, now there are some conflicts 🙃

[Ericson2314]

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