LnL7 / nix-darwin

nix modules for darwin
MIT License
2.82k stars 431 forks source link

Use wait4path with command and script launchd options #1052

Closed will-lol closed 1 week ago

will-lol commented 1 month ago

Addresses my recent issue, https://github.com/LnL7/nix-darwin/issues/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.

Samasaur1 commented 1 month ago

I wonder if you still want to exec the actual command, which appears to be the previous behavior but looks like you removed it

niklasravnsborg commented 3 weeks ago

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 spawn scheduled (sudo launchctl print system/org.nixos.nextdns). I now applied this refactor to my nix-darwin fork and now everything works like a charm.

I had to refactor the nextdns service to use command instead of serviceConfig.ProgramArguments in https://github.com/niklasravnsborg/nix-darwin/commit/697115e51d4c6eccd6bb65738a32b1b14258f4d9. Do I see it correctly that all services using launchd.daemons have to be refactored in a similar fashion?

will-lol commented 3 weeks ago

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.

niklasravnsborg commented 3 weeks ago

@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

emilazy commented 3 weeks ago

We could implement our own programArguments that uses lib.escapeShellArgs to construct an equivalent shell command, since the list interface is nicer and it would mean fewer changes to services.

will-lol commented 3 weeks ago

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? Also: many of the modules I migrated over from ProgramArguments and /bin/wait4path I do not use myself, and have not yet confirmed to be working on bare metal. If required I'll work on that next weekend and hopefully we can get this merged. Thanks all!

will-lol commented 1 week ago

Manual testing of the refactors I made that I've done on my machine:

niklasravnsborg commented 1 week ago

@will-lol Do you know why some of the tests are broken?

will-lol commented 1 week ago

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.

Enzime commented 1 week ago

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: https://github.com/DeterminateSystems/nix-installer/issues/753#issuecomment-1995192341

It'll probably require users to uninstall nix-darwin then Nix and then reinstall both of them

emilazy commented 1 week ago

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 https://github.com/LnL7/nix-darwin/pull/1052#issuecomment-2336654030, but this seems like an improvement on the status quo.

niklasravnsborg commented 1 week ago

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)

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

niklasravnsborg commented 1 week ago

@will-lol Do I see correctly there some other services missing regarding this update to use the script, right? How should we proceed on those? Merge this first and wait for module maintainers to move their logic over as well?

I personally just noticed it regarding nextdns and would be available to update and test it there.

Enzime commented 1 week ago

Tested this locally and looks like it's working

utkarshgupta137 commented 1 week ago

For those of us who pinned nixpkgs to an earlier version to use Karabiner 14.3, this breaks it.

ofalvai commented 6 days ago

Came here to investigate the same issue. My nix-darwin follows nixpkgs-stable to keep using Karabiner 14.3 and updating nix-darwin to the latest version breaks Karabiner. Rolling back to 6374cd7e50aa057a688142eed2345083047ad884 makes it work again.

Enzime commented 6 days ago

See #1092