Use wait4path with command and script launchd options #1052
Conversation
|
I wonder if you still want to |
|
I also had a similar problem as @will-lol described in the #1043 issue but with nextdns: The service didn't start after reboot and the service state was stuck at I had to refactor the nextdns service to use |
|
Hi @niklasravnsborg. I believe so. I've refactored some modules that were using wait4path hack to use 'command' or 'script' instead. I am now working on getting tests to pass. I could include refactors of other modules that are manually using serviceConfig.ProgramArguments also. It may also be best for module maintainers to exercise their own judgement. |
|
@will-lol If you want you can also pull in my nextdns fix here :) I'm looking forward to not maintaining my own fork anymore :D |
|
We could implement our own |
|
All of the tests broken by this change are now fixed. I wonder if we'd prefer to deal with fixing problems with other modules that aren't yet using /bin/wait4path in some fashion in a separate issue or PR? |
modules/launchd/default.nix
Outdated
| serviceConfig.ProgramArguments = mkIf (cmd != "") [ | ||
| "/bin/sh" | ||
| "-c" | ||
| "/bin/wait4path /nix/store && exec ${cmd}" |
There was a problem hiding this comment.
Is there any special reason, you used exec here? I think we could just drop it and it would make extending the cmd string easier, because one could still use basic shell syntax.
There was a problem hiding this comment.
Using 'exec' was the previous behaviour. I also received a comment (#1052 (comment)) suggesting that I add it back.
I'm now of the view that chaining commands together with any special shell syntax belongs in a script. The shell that is used to execute the program arguments is also not obvious to the user of the abstraction. By using a script one would expect (by nix convention) that you are getting bash, which also has some extended syntax not supported by /bin/sh.
Totally open to anything else tho
|
Manual testing of the refactors I made that I've done on my machine:
|
|
@will-lol Do you know why some of the tests are broken? |
|
Hah not totally sure.. I can't really see a reason why it failed that time. The error suggests hello.nix wasn't copied or checked into git? All checks are passing now. |
|
I wonder if the real fix is to get all users to migrate to an unencrypted Nix Store as it should still be encrypted automatically by FileVault see: DeterminateSystems/nix-installer#753 (comment) It'll probably require users to uninstall nix-darwin then Nix and then reinstall both of them |
|
I personally use an encrypted store. An unencrypted store is not protected in the same way that FileVault volumes are, even though the storage is encrypted at rest; you can access such volumes in macOS Recovery without authentication. My understanding is that full encryption is also a hard requirement for many enterprise users. I hope we can move towards a more structured interface per #1052 (comment), but this seems like an improvement on the status quo. |
addresses LnL7#1043 fix: use exec in launchd daemon config fix: dont use a script thats in the nix store fix: remove manual wait4path in linux-builder fix: remove manual wait4path in karabiner elements fix: remove manual wait4path in nix-daemon fix: remove manual wait4path in nix-optimise fix: remove manual wait4path in tailscaled fix: autossh test Revert "fix: remove manual wait4path in nix-daemon" This reverts commit 6aec084. fix: remove bad exec Reapply "fix: remove manual wait4path in nix-daemon" This reverts commit c8f136e. fix: update autossh test to reflect changes in f86e613 fix: services-activate-system-changed-label-prefix test fix: services-buildkite-agent test fix: services-activate-system test fix: escape ampersand fix: services-lorri test fix: services-nix-optimise test fix: services-nix-gc test refactor: use script rather than command in daemon fix: use config.command for clarity style: fix indentation fix: use lib.getExe rather than directly pointing to file revert: a87fc7b - mistaken refactor meant that service waited for nix store and not the relevant path
@Enzime I think even an unencrypted store might suffer the same problem when using in combination with launchd services, since the OS might try to start those services before the store is mounted (while the mounting itself is done in a launchd service). |
|
@will-lol Do I see correctly there some other services missing regarding this update to use the I personally just noticed it regarding |
| # For unknown reasons this daemon will fail if VirtualHIDDeviceClient is not exec'd. | ||
| "/bin/wait4path /nix/store && exec \"${pkgs.karabiner-elements.driver}/Library/Application Support/org.pqrs/Karabiner-DriverKit-VirtualHIDDevice/Applications/Karabiner-DriverKit-VirtualHIDDeviceClient.app/Contents/MacOS/Karabiner-DriverKit-VirtualHIDDeviceClient\"" | ||
| ]; | ||
| command = "${pkgs.karabiner-elements.driver}/Library/Application Support/org.pqrs/Karabiner-DriverKit-VirtualHIDDevice/Applications/Karabiner-DriverKit-VirtualHIDDeviceClient.app/Contents/MacOS/Karabiner-DriverKit-VirtualHIDDeviceClient"; |
There was a problem hiding this comment.
Is it intentional that just this one uses command and the other ones use script?
|
Tested this locally and looks like it's working |
|
For those of us who pinned nixpkgs to an earlier version to use Karabiner 14.3, this breaks it. |
|
Came here to investigate the same issue. My |
|
See #1092 |
Addresses my recent issue, #1043.
I've attempted to make this compatible with both the 'command' and 'script' options of 'launchd.daemons.'. This works fine on my machine, but some tests that check the launchd plist files fail because this will change the format of them slightly.
I thought I would put this first commit here in case anyone had any objections before I went ahead and rewrote all of the tests and modules to not use wait4path anymore.