nix-darwin icon indicating copy to clipboard operation
nix-darwin copied to clipboard

fix(launchd): `StartCalendarInterval`, `nix.gc.interval`

Open tmillr opened this issue 1 year ago • 5 comments

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

tmillr avatar Jun 29 '23 09:06 tmillr

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?

steveej avatar Sep 13 '23 10:09 steveej

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

tmillr avatar Sep 13 '23 20:09 tmillr

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.

tmillr avatar Sep 22 '23 07:09 tmillr

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.

tmillr avatar Sep 22 '23 12:09 tmillr

Thanks for all your work on this! I will try to take a look in the next couple days.

emilazy avatar Sep 22 '23 19:09 emilazy

@emilazy if i'm not mistaken this is waiting for your review.

steveej avatar May 16 '24 08:05 steveej

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

tmillr avatar Jun 09 '24 18:06 tmillr

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.

emilazy avatar Jun 13 '24 09:06 emilazy