nix-darwin
nix-darwin copied to clipboard
fix(launchd): `StartCalendarInterval`, `nix.gc.interval`
Stricter launchd -> StartCalendarInterval type which now verifies that the integers passed to Minute
, Hour
, etc. are within range. When provided, the value for this type must be a non-empty list and must not contain duplicates. Examples and defaults now type-check successfully.
Use this new type for nix.gc.interval
. Its example and default now type-check successfully.
Create modules/launchd/types.nix
file for easier use of launchd types needed in multiple files.
Update option documentation:
Update and improve wording/documentation of launchd's StartCalendarInterval
.
Improve wording/documentation of nix.gc.interval
("time interval" can be misleading as it's actually a "calendar" interval, e.g. { Hour = 3; Minute = 15;}
actually runs daily, not every 3.25 hours).
hey, since #611 was dropped in favor of this i'm interested in getting this PR merged.
@tmillr thanks for the work on this. do you have bandwidth to see this through or would you prefer someone else continues here?
I'm not sure when I'll be able to finish this. I'm fine with someone else working on this pr or finishing it. Thanks
I've had more time lately and so I've decided I'd try to get this finished once and for all (hopefully within the next day or 2). Sorry for the delay.
Updates
Changes
- Calendar interval must be a list (when provided). I did this because allowing an attrset as well ruined the documentation.
- Create
launchd/types.nix
file - Update option documentation
- Add type-check for duplicate interval specs (i.e. attrsets) within the provided list
- Saner default for gc interval (i.e. the default is now to run weekly instead of daily)
Questions/Concerns/Etc.
- (FIXED) ~An error message produced by the new type-checking might not be very detailed/helpful? (e.g. an error due to finding a duplicate interval spec within the list, or, an error due to the list being empty). Unfortunately, it doesn't seem that nix allows returning a custom message from the type-check function.~
- A warning should probably be emitted if an empty attrset is found within the list? (because I am pretty sure that this will result in a job which runs every minute). I don't think Nix recommends emitting warnings within the type-check itself though, so I imagine that this would have to be done anywhere/everywhere
StartCalendarInterval
is used if implemented? - Similarly, there's no check for if the list contains an empty attrset in it along with others which aren't empty (wacky edge-case, probably not a big deal? considering the above is implemented at some point). More than one empty attrset will cause an error however, due to there being duplicates within the list.
Overall though I think I'm happy with it as it is now and maybe these points can just be revisited/referenced in the future if needed. Let me know if there's anything else.
Thanks for all your work on this! I will try to take a look in the next couple days.
@emilazy if i'm not mistaken this is waiting for your review.
Hi @emilazy. I think it's good to go now if you'd like to review it again. I've addressed the merge conflicts (which mainly had to do with the recent removal of use of lib.mdDoc
in def1e23). All checks are passing, and I've also done some manual testing locally. Let me know if anything else needs to be changed. Thanks
Thank you for all the work you’ve put into this, and I’m sorry for my prolonged absence! Catching up now and will try to review this in the next few days. Please do feel free to ping me if it slips through the cracks.