-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
displayManager, windowManager, desktopManager: use extensible option types #20271
Conversation
@ericsagnes, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @abbradar and @ocharles to be potential reviewers. |
although we might have more display managers available but not Edit: Nevermind, I mixed up display managers and window managers here. |
maybe |
nixos/tests/gnome3.nix
Outdated
@@ -11,7 +11,7 @@ import ./make-test.nix ({ pkgs, ...} : { | |||
|
|||
services.xserver.enable = true; | |||
|
|||
services.xserver.displayManager.auto.enable = true; | |||
services.xserver.displayManager.enable = "auto" |
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.
Missed ;
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 catch, thank you!
de270e8
to
07e5988
Compare
Update: fixed the conflicts. |
@ericsagnes I guess the next step is to rewrite this code using extensible option declaration, with the |
@nbp I am not sure to understand, this PR is a rewrite of |
Ok, sorry I did a quick walkthrough and only noticed https://github.com/NixOS/nixpkgs/pull/20271/files#diff-05435704a2787433f3120047d8dc2b35R319 , which made me think that these were still the old changes. I will try to review it over the week-end. |
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.
These changes looks good and succeeded where I failed to improve the module system in the past. This solve the uniqueness issue of the display manager, while providing a simple and nice interface for it.
On the other hand, the change to the auto
display manager highlights one of the pitfall introduced by the uniqueness, which is that we would either have to repeat the config, or find another way to carry the result than the option which got used for the input. (see comments below)
@@ -126,7 +122,12 @@ in | |||
|
|||
###### implementation | |||
|
|||
config = mkIf cfg.enable { | |||
config = mkIf (elem dmcfg.enable [ "slim" "auto" ]) { |
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.
It sounds like we do want to have a services.xserver.displayManager.slim.enabled
(with an extra d
) internal option, in order to make these abstraction modular.
In this case we want the auto
display manager to be an alias for enabling the slim
display manager as a back-end implementation while being added in the same unique list as slim
. Having enabled
option checks would allow us to work-around the uniqueness of the enum
type.
|
||
services.xserver.displayManager.slim = { | ||
autoLogin = mkIf (dmcfg.enable == "auto") true; | ||
defaultUser = mkIf (dmcfg.enable == "auto") dmcfg.auto.user; |
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.
These definitions belong to the auto.nix
file, which declares these options.
@@ -171,6 +171,15 @@ in | |||
|
|||
services.xserver.displayManager = { | |||
|
|||
enable = mkOption { | |||
type = with types; nullOr (enum [ ]); | |||
example = "slim"; |
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.
you can move the example to the slim.nix
file, if you prefer.
@@ -171,6 +171,15 @@ in | |||
|
|||
services.xserver.displayManager = { | |||
|
|||
enable = mkOption { |
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.
I think select
makes more sense than any other name. enable
is too much tied to a boolean value, while select
clearly suppose a finite set of options. I would avoid the passive form here, as this is an input, and not a result.
36466a6
to
991fd69
Compare
Thanks for the detailed review and the precise advices, you are always very helpful! Next step is to use extended option types for It will also make it easier to document the whole changes and check that all the tests are passing. I will request another review when everything ready. |
f2bcc97
to
5d197e9
Compare
Added commits for desktop managers and window managers. @nbp The PR is ready to review, could you please review it again when you have some time? Passing the following tests, unchecked means failure:
|
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.
I admit I am not fond of the windowManagers.select
and desktopManagers.select
changes, as opposed to the displayManager.select
option which expect a single result.
services.xserver.windowManager.select = [ "i3" ]; | ||
</programlisting> | ||
Note: This option is a list because it is possible to enable multiple window managers at the same time. | ||
If multiple window managers are set, the first in the list will become the default one. |
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.
Unfortunately, there no ordering of elements guaranteed in NixOS, as the result of a merge can be whatever.
If the following 2 modules are merged, you have no idea how that are going to be merged.
{ services.xserver.desktopManager.select = [ "plasma5" ]; }
{ services.xserver.desktopManager.select = [ "gnome3" ]; }
Previously we had mkOrder
properties with mkBefore
and mkAfter
, but these got replaced by a TODO in modules.nix.
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.
Which TODO are you refering to? I have tested with a simple example and it seem that mkOrder
is working as expected, but I might be missing something.
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.
Ok, only a sub-part of mkOrder
is not implemented, which is a debatable part, about pushing down the ordering property. Honestly, as opposed to mkIf
and mkOverride
it might be debatable to support mkOrder
on an attribute set which is not an option definition, so I will add an assertion and see if people can come with a use-case which where this might matter.
@@ -205,6 +229,21 @@ in | |||
|
|||
services.xserver.displayManager = { | |||
|
|||
select = mkOption { | |||
type = with types; nullOr (enum [ ]); | |||
default = defaultDisplayManager; |
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.
Do not used computed values as default values.
If you want to do so, use the config section with mkDefault
property.
The reason we should avoid this is because the documentation capture the default value, and adding this here would cause the documentation to not only depend on the option declarations, but also to depends on the option definitions.
Doing so used to recompile the documentation locally instead of downloading it. Today we use a work-around to avoid the recompilation, but this implies that we have to re-evaluate all NixOS modules a second time, which is far from ideal.
description = '' | ||
Whether to enable SLiM as the display manager. | ||
Option to abstract slim usage. |
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.
nit: SLiM, not slim. ( https://wiki.archlinux.org/index.php/SLiM )
I do agree that it does not feel like the perfect solution, but still it is an improvement over the current I am open to change this approach to any better solution. |
I hope that most NixOS user would never have to learn about Thus, I am highly worried about the order of the window / desktop managers, as this is an option which is quite visible to new desktop users of NixOS. While I do not expect new users to split their modules them-self, I would still expect to have new users import someone else configuration. Before, importing someone configuration would raise an error message about the fact that the option is defined twice. In this new case this would pick one at random. I think it would be better to have each window / desktop manager define what is their default display manager with What do you think? |
I see it's been a while since this was last commented on but the final suggestion where each configuration comes with a sensible default that I can override makes sense to me. Have we propositioned the community via IRC and discourse to gain more thoughts around this interface? Alternatively, has this work been abandoned and this pull request should be closed? |
Thank you for your contributions.
|
This has changed in 20.03 and is incompatible with this PR. |
Motivation for this change
Use extensible option types for xserver displayManager, windowManager and desktopManager options.
This should allow easier xserver configuration and modules.
Note: This PR content allow to setup Xserver by only setting required options, related options will automatically get adapted defaults. For example, to set
plasma5
it was required to set the following:Now the following will have the same result:
Setting plasma5 as the desktop manager will automatically select
sddm
as display manager and enable the X server.Note: The desktop managers and window managers are a list because it is possible to enable multiple at the same time. In that case the first entry in the list is considered as the default.
cc: @edolstra @nbp