-
Notifications
You must be signed in to change notification settings - Fork 14
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
fix: renamed displayManager setting #142
Conversation
@@ -407,7 +407,7 @@ | |||
} | |||
(mkIf cfg.enable { | |||
environment.systemPackages = [cfg.package]; | |||
services.xserver.displayManager.sessionPackages = [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.
Should we add a backward compatible option?
like
services = if options.services.xserver.displayManager ? sessionPackages
then xserver.displayManager.sessionPackages = [cfg.package];
else displayManager.sessionPackages = [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.
Yes, thank you, that should be done; i overlooked that, will change that before it could be merged
I vaguely wish to support the latest stable NixOS. See: #32 Simply changing the option to the new variant is not really useful in this regard; it will break compatibility with NixOS 23.11. Is there a nice way to change it on just 24.05? Otherwise, it might be better to leave it and let nixpkgs handle compatibility. The PR is already merged, just hasn't made it into nixos-unstable yet: https://nixpk.gs/pr-tracker.html?pr=303186 |
For now, other than waiting for nixpkgs, a user encountering this issue can add the following module as a workaround: {lib, ...}: {
imports = [
(lib.mkRenamedOptionModule ["services" "xserver" "displayManager" "sessionPackages"] ["services" "displayManager" "sessionPackages"])
];
} It causes an ugly warning, though. |
lmao in actually testing 23.11 i realize there's a handful of other incompatibilities anyways (mergeAttrsList is only in unstable; home-manager module relies on it in a couple places). You said you wanted to make it backwards compatible? At the current state of my repo, that's not gonna accomplish much. So, i think it looks good to merge already, to be honest. I think i'll implement the backwards compatibility in a nice way in the future, along with the rest of 23.11 fixes. |
i played around with the suggestion from @oluceps for backwards compatibility; it did't "just work" so i didn't commit anything for that yet, but the idea seems sound and i will look into it more (if not useful for the project just to get more familiar with nix) and hopefully submit a new merge request when i'm done. |
I'll merge this, then, since as it turns out the modules already aren't compatible with 23.11 (but the packages are) |
wait what no wtf am i thinking. this is intentional. overriding the nixpkgs input is not the way to do it. this breaks because i've been testing 23.11 compatibility by overriding the input with the nix cli, but in a real system it's more granular than this. |
In NixOS/nixpkgs#291913 the option
services.xserver.displayManager.sessionPackages
has been renamed toservices.displayManager.sessionPackages
to prepare for wayland without xserver enabled. This flake uses this option and breaks on the latest nixpkgs.This change will likely not be breaking for much longer as backwards compatibility should be re-addded once NixOS/nixpkgs#303186 is merged. But since niri is a wayland compositor, I believe it would make sense to make this change anyways.