Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork567
services: addsleepwatcher service#1617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:nix-darwin-25.05
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This service allows specification of user or system scriptsto run at various system event moments, such as "system wokefrom sleep" or "AC power plugged in", etc.RequiresNixOS/nixpkgs#455914 to bemerged first.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this should targetmaster instead ofnix-darwin-25.05; it can be backported if desired.
Also I didn't leave this as an inline comment because there wasn't one place to put it, but did you consider making the scripts betypes.lines instead oftypes.nullOr types.lines and checking for"" instead ofnull? I think it would be functionally the same but seems to match our style better.
| }; | ||
| in | ||
| { | ||
| environment.systemPackages = [ cfg.package ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Doessleepwatcher need to be in the system path? It doesn't seem like this is useful given that the daemon is already running.
| { | ||
| environment.systemPackages = [ cfg.package ]; | ||
| launchd.user.agents.sleepwatcher = mkIf (!cfg.run_as_system) launchdConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
User agents in nix-darwin itself need to have amanagedBy attribute so that we can properly display error messages whensystem.primaryUser is unset.
| run_as_system = mkOption { | ||
| type = types.bool; | ||
| default = false; | ||
| description = '' | ||
| Whether to run sleepwatcher as a system daemon (true) or user agent (false). | ||
| System daemon runs as root and can execute system-level commands. | ||
| User agent runs as the user and is preferred for user-specific actions. | ||
| ''; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there any case where the user might want to run both a systemwide and a per-user instance?
| ProgramArguments = [ | ||
| "${cfg.package}/bin/sleepwatcher" | ||
| ] ++ optionals cfg.verbose [ "--verbose" ] | ||
| ++ optionalHook "sleep" cfg.on_sleep | ||
| ++ optionalHook "wakeup" cfg.on_wakeup | ||
| ++ optionalHook "displaysleep" cfg.on_display_sleep | ||
| ++ optionalHook "displaywakeup" cfg.on_display_wakeup | ||
| ++ optionalHook "displaydim" cfg.on_display_dim | ||
| ++ optionalHook "displayundim" cfg.on_display_undim | ||
| ++ optionals (cfg.on_idle != null && cfg.idle_time != null) | ||
| ([ "--timeout" "${toString cfg.idle_time}" ] ++ | ||
| (optionalHook "idle" cfg.on_idle)) | ||
| ++ optionalHook "idleresume" cfg.on_idle_resume | ||
| ++ optionalHook "plug" cfg.on_plug | ||
| ++ optionalHook "unplug" cfg.on_unplug; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We prefer that you setcommand instead ofserviceConfig.ProgramArguments so that a) the service picks up/bin/wait4path so that it doesn't fail repeatedly and be put in the penalty box when the nix store is not yet mounted; b) future changes to launchd services apply to them as well
| ++ optionals (cfg.on_idle != null && cfg.idle_time != null) | ||
| ([ "--timeout" "${toString cfg.idle_time}" ] ++ | ||
| (optionalHook "idle" cfg.on_idle)) | ||
| ++ optionalHook "idleresume" cfg.on_idle_resume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This check checks whethercfg.on_idle is null twice, which seems redundant.
Also, I imaginecfg.on_idle_resume should also be ignored ifcfg.idle_time is unset?
| verbose = mkOption { | ||
| type = types.bool; | ||
| default = true; | ||
| description = "Whether to verbosely log actions performed by sleepwatcher."; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This option is essentially useless without settingserviceConfig.StandardOutPath and/orserviceConfig.StandardErrorPath on the service itself, as launchd just swallows the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd recommend adding options for these attributes rather than hardcoding them, btw; see some other services that do that.
| in optionals (hookValue != null) [ "--${longArg}" "${hookScript}" ]; | ||
| launchdConfig = { | ||
| path = [ config.environment.systemPath ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's the reasoning behind this as opposed to having people set the PATH individually on each script that needs it set?
Also note that the homepage for this software says
which should be reflected somewhere in this service, probably in the description for those options. |
Uh oh!
There was an error while loading.Please reload this page.
This service allows specification of user or system scripts to run at various system event moments, such as "system woke from sleep" or "AC power plugged in", etc.
RequiresNixOS/nixpkgs#455927 to be merged (and backported) first.