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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nixos/sddm: use attrs instead of plain text #107724
Conversation
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.
Awesome! In addition to the comments I left I think it would be better to have a few commits instead of mixing together... if that's not too much a hassle.
[ "services" "xserver" "displayManager" "sddm" "autoLogin" "user" ] | ||
[ "services" "xserver" "displayManager" "autoLogin" "user" ] | ||
) | ||
(mkRenamedOptionModule |
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.
Different type so you should use mkRemovedOptionModule
and mention the new option.
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.
Good point, fixed.
type = types.lines; | ||
default = ""; | ||
settings = mkOption { | ||
type = types.attrs; |
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 have some fancy pants new types available. Take a look at this, specifically format = pkgs.formats.ini {};
.
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.
Nice, thanks, fixed.
@GrahamcOfBorg build tests.sddm |
Instead of treating the sddm config a wall of text that doesn't allow us to override anything, turn it into an attribute set. We dump `extraConfig` and instead introduce `settings` that is merged with the module defaults to provide the final configuration. There is some additional noise in here due to nixpkgs-fmt.
@GrahamcOfBorg build tests.sddm |
@GrahamcOfBorg test sddm |
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.
LGTM 👍
Motivation for this change
Instead of treating the sddm config a wall of text that doesn't allow us
to override anything, turn it into an attribute set.
We dump
extraConfig
and instead introducesettings
that is mergedwith the module defaults to provide the final configuration.
There is some additional noise in here due to nixpkgs-fmt.
We totally should do this everywhere where we are currently just doing a bunch of text but the expected format is one for which we have a generator available (ini, yaml, json, toml).
A few minor additional things:
/bin/sh
in the generated scriptsThings done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)