services/turnstiled: init#22
Conversation
|
awesome! please hold off merging for a few days, i would like to share some related |
aanderse
left a comment
There was a problem hiding this comment.
very cool! this will work so well with the dinit.services and dinit.user.services options i have written!
| ]; | ||
|
|
||
| patches = [ | ||
| ./patch.diff |
There was a problem hiding this comment.
/run/current-system/sw/bin/dinit-monitor is less than ideal, especially if we're already linking against dinit - how about:
postPatch = ''
substituteInPlace backend/dinit \
--replace-fail '/usr/bin/dinit-monitor' '${lib.getExe' dinit "dinit-monitor"}'
'';and if we're doing that we could inline the patch to meson.build to simplify (ie. drop patch.diff):
substituteInPlace meson.build \
--replace-fail "get_option('prefix'), get_option('sysconfdir'), 'turnstile'" "'/etc', 'turnstile'"There was a problem hiding this comment.
Agreed, didn't think of this while I was writing it. Changed in latest commit.
| "-Ddefault_backend=dinit" | ||
| "-Ddinit=enabled" | ||
| "-Dstatedir=/var/lib/turnstiled" | ||
| "-Dpam_moddir=./pam" |
| mesonFlags = [ | ||
| "-Ddefault_backend=dinit" | ||
| "-Ddinit=enabled" | ||
| "-Dstatedir=/var/lib/turnstiled" |
There was a problem hiding this comment.
isn't that already the value? if not, sounds like we need to modify localstatedir instead?
|
|
||
| stdenv.mkDerivation { | ||
| pname = "turnstile"; | ||
| version = "0-unstable-2025-12-15"; |
There was a problem hiding this comment.
worth pinning to version 0.1.11, only one irrelevant commit off this... makes it easier to upstream into nixpkgs, which should definitely be a goal of ours
There was a problem hiding this comment.
Good point, pinned it in latest commit.
| enableGraphicalMonitor = mkOption { | ||
| type = bool; | ||
| default = false; | ||
| description = "Whether or not to monitor environment changes to DISPLAY and WAYLAND_DISPLAY variables. Currently requires manually adding `dinitctl setenv VAR=$VAR`to any startup scripts for your graphical environment to function."; | ||
| }; |
There was a problem hiding this comment.
i'm not sure we should actually have this as an option at all... and if we should, then it should be configured via services.turnstiled.package = pkgs.turnstiled.override { enableGraphicalMonitor = false; }; or something along those lines
There was a problem hiding this comment.
I'm relatively neutral on the idea, the main purpose of the option was to allow an immediately obvious way to disable them for people looking at the options from a non-source code position. Like I said though, no strong opinion, happy to remove if you think its best to make it transparent.
Decision might also depend on how we resolve this comment so I figured I'd leave it unchanged for now.
| service_dir = mkOption { | ||
| type = str; | ||
| default = "$HOME/.config/dinit.d"; | ||
| description = "Users service dir for `turnstile`'s `dinit` backend. This should include a way to differentiate per user unless all users have identical services."; | ||
| }; | ||
|
|
||
| boot_dir = mkOption { | ||
| type = str; | ||
| default = "${cfg.dinit.service_dir}/boot.d"; | ||
| description = "Users service boot dir for `turnstile`'s `dinit` backend. This should include a way to differentiate per user unless all users have identical services."; | ||
| }; | ||
|
|
||
| system_boot_dir = mkOption { | ||
| type = path; | ||
| default = "/usr/lib/dinit.d/user/boot.d"; | ||
| description = "Systems service boot dir for `turnstile`'s `dinit` backend."; | ||
| }; |
There was a problem hiding this comment.
could we please put these under a settings key (key, not option)?
There was a problem hiding this comment.
For sure, makes a lot of sense.
| "turnstile/backend/dinit.conf".text = '' | ||
| boot_dir="${cfg.dinit.boot_dir}" | ||
| system_boot_dir="${cfg.dinit.system_boot_dir}" | ||
| services_dir1="${cfg.dinit.service_dir}" | ||
| services_dir2="/etc/dinit.d/user" | ||
| services_dir3="/usr/local/lib/dinit.d/user" | ||
| services_dir4="/usr/lib/dinit.d/user" | ||
| ''; |
There was a problem hiding this comment.
| "turnstile/backend/dinit.conf".text = '' | |
| boot_dir="${cfg.dinit.boot_dir}" | |
| system_boot_dir="${cfg.dinit.system_boot_dir}" | |
| services_dir1="${cfg.dinit.service_dir}" | |
| services_dir2="/etc/dinit.d/user" | |
| services_dir3="/usr/local/lib/dinit.d/user" | |
| services_dir4="/usr/lib/dinit.d/user" | |
| ''; | |
| "turnstile/backend/dinit.conf".text = (pkgs.formats.keyValue { }).generate "dinit.conf" cfg.dinit.settings; |
see comments above about adding services.turnstiled.dinit.settings option
There was a problem hiding this comment.
Leaving unresolved because I think I want to change the services.turnstiled.dinit.settings.service_dir option to allow a list so users can change services_dir 2-4 and beyond. Makes the conversion a bit more complicated but I will definitely look into this
There was a problem hiding this comment.
as much as i don't like opinionated modules... yes, that is so so so much better 😅
please 🙇♂️
| settings = | ||
| with lib; | ||
| mkOption { | ||
| type = | ||
| with types; | ||
| submodule { | ||
| options = { |
There was a problem hiding this comment.
| settings = | |
| with lib; | |
| mkOption { | |
| type = | |
| with types; | |
| submodule { | |
| options = { | |
| settings = { |
not everything needs to be an option - in this case there is no actual advantage, and an extra submodule evaluation has to occur, so i would drop the mkOption and go with a raw attribute set for settings in this case
There was a problem hiding this comment.
unoptioned settings
|
|
||
| configFile = lib.mkOption { | ||
| type = lib.types.path; | ||
| default = "/etc/turnstiled.conf"; | ||
| description = "Configuration file location for tunrstiled"; | ||
| }; |
There was a problem hiding this comment.
| configFile = lib.mkOption { | |
| type = lib.types.path; | |
| default = "/etc/turnstiled.conf"; | |
| description = "Configuration file location for tunrstiled"; | |
| }; |
unused, drop it 👍
initial commit for the turnstiled service. currently very barebones