displayManager, windowManager, desktopManager: use extensible option types by ericsagnes · Pull Request #20271 · NixOS/nixpkgs

ericsagnes · 2016-11-09T02:54:42Z

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:

{
  services.xserver.enable = true;
  services.xserver.displayManager.sddm.enable = true;
  services.xserver.desktopManager.plasma5.enable = true;
}

Now the following will have the same result:

{
  services.xserver.desktopManager.select = [ "plasma5" ];
}

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

mention-bot · 2016-11-09T02:54:44Z

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

FRidh · 2016-11-09T07:28:47Z

I am not sure of the naming of the new option, displayManager.enable, as enable options are usually used with booleans.
There might be more appropriate option name, and suggestions are welcome!

available?

although we might have more display managers available but not enabled.

Edit: Nevermind, I mixed up display managers and window managers here.

danbst · 2016-11-09T10:08:26Z

maybe services.xserver.displayManager.selected = "kdm"; ?

rasendubi · 2016-11-09T15:42:45Z

nixos/tests/gnome3.nix

Good catch, thank you!

ericsagnes · 2016-12-03T07:20:13Z

ping @edolstra @nbp

Any thoughts on this, so far the list of propositions for this kind of option name are:

enable
enabled
select
selected
manager
managers
available

ericsagnes · 2017-03-25T12:18:27Z

Update: fixed the conflicts.

nbp · 2017-05-26T13:58:38Z

@ericsagnes I guess the next step is to rewrite this code using extensible option declaration, with the enum type (#18380), correct?

ericsagnes · 2017-05-26T22:55:43Z

@nbp I am not sure to understand, this PR is a rewrite of displayManager.*.enable options to use extensible option types (displayManager.enable with a nullOr enum).
For example: Main declaration, extended declaratations.

nbp · 2017-05-30T20:21:40Z

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.

nbp

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)

nbp · 2017-05-31T21:08:21Z

nixos/modules/services/x11/display-managers/slim.nix

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.

nbp · 2017-05-31T21:09:15Z

nixos/modules/services/x11/display-managers/slim.nix

These definitions belong to the auto.nix file, which declares these options.

nbp · 2017-05-31T21:10:50Z

nixos/modules/services/x11/display-managers/default.nix

you can move the example to the slim.nix file, if you prefer.

nbp · 2017-05-31T21:12:45Z

nixos/modules/services/x11/display-managers/default.nix

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.

ericsagnes · 2017-06-01T13:01:50Z

Thanks for the detailed review and the precise advices, you are always very helpful!
I added the requested changes.

Next step is to use extended option types for desktopManagers and windowManagers to have a consistent xserver configuration.
As making a dedicated PR for each would result in some noise for unstable users, I propose to extend the scope of this PR so it contains desktopManager and windowManager too.

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.

ericsagnes · 2017-06-02T13:09:05Z

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:

gnome3-gdm was also failing on the latest nixos-unstable (http://hydra.nixos.org/build/53595717), so I suppose the failure reason is not related to this PR contents.

nbp

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.

nbp · 2017-06-03T11:28:01Z

nixos/doc/manual/release-notes/rl-1709.xml

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


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.

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.

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.

nbp · 2017-06-03T11:39:12Z

nixos/modules/services/x11/display-managers/default.nix


+      select = mkOption {
+        type = with types; nullOr (enum [ ]);
+        default = defaultDisplayManager;


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.

nbp · 2017-06-03T14:06:51Z

nixos/modules/services/x11/display-managers/slim.nix

+        default  = dmcfg.select == "slim";
        description = ''
-          Whether to enable SLiM as the display manager.
+          Option to abstract slim usage.


nit: SLiM, not slim. ( https://wiki.archlinux.org/index.php/SLiM )

ericsagnes · 2017-06-04T07:30:50Z

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.

I do agree that it does not feel like the perfect solution, but still it is an improvement over the current desktopManager.*.enable and desktopManager.default combination that was requiring extra checks to verify the default desktop / window manager was enabled (and also checks in other modules).

I am open to change this approach to any better solution.

nbp · 2017-06-10T17:06:07Z

I hope that most NixOS user would never have to learn about mkIf, mkOverride, or mkOrder.

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 mkDefault, and raise an error if we do not have a uniquely preferred display manager as the result of the merge. Thus, a user having selected both KDE and Gnome would be prompted to select a display manager as there is no guessable default.

What do you think?

alunduil · 2019-07-28T19:58:22Z

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?

stale · 2020-06-01T19:51:42Z

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the
related code and @ mention them in a comment.
Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
irc.freenode.net.

domenkozar · 2020-07-20T07:40:50Z

This has changed in 20.03 and is incompatible with this PR.

Multiple security issues are mentionned in the changelog: ``` Fixed issue NixOS#20271: [security] Reflected XSS in CSRF token handling Fixed issue [security]: Export survey/label set resources with limited permissions Fixed issue NixOS#20008: XSS on survey theme description Fixed issue NixOS#19965: Lot of security issue in jspdf ``` Changes: https://github.com/LimeSurvey/LimeSurvey/blob/6.15.14%2B250924/docs/release_notes.txt

Multiple security issues are mentionned in the changelog: ``` Fixed issue #20271: [security] Reflected XSS in CSRF token handling Fixed issue [security]: Export survey/label set resources with limited permissions Fixed issue #20008: XSS on survey theme description Fixed issue #19965: Lot of security issue in jspdf ``` Changes: https://github.com/LimeSurvey/LimeSurvey/blob/6.15.14%2B250924/docs/release_notes.txt (cherry picked from commit 70fa9f4)

rasendubi reviewed Nov 9, 2016

View reviewed changes

ericsagnes force-pushed the feat/dm-eot branch from de270e8 to 07e5988 Compare November 9, 2016 22:44

ericsagnes mentioned this pull request Nov 17, 2016

i3 module: refactor #20385

Merged

7 tasks

danbst mentioned this pull request Feb 20, 2017

Finer-grained firewall: allow specific hosts, in particular to SSH #22586

Closed

7 tasks

ericsagnes force-pushed the feat/dm-eot branch from 07e5988 to 3e89500 Compare March 25, 2017 12:07

ericsagnes force-pushed the feat/dm-eot branch from 3e89500 to 9a6ed82 Compare April 24, 2017 23:44

ericsagnes mentioned this pull request May 12, 2017

Missing "display-manager" job when enabling sddm #25735

Closed

nbp self-requested a review May 30, 2017 20:21

nbp suggested changes May 31, 2017

View reviewed changes

ericsagnes force-pushed the feat/dm-eot branch 2 times, most recently from 36466a6 to 991fd69 Compare June 1, 2017 12:52

ericsagnes changed the title ~~display manager: use extensible option types~~ [WIP] displayManager, windowManager, desktopManager: use extensible option types Jun 1, 2017

ericsagnes force-pushed the feat/dm-eot branch 2 times, most recently from f2bcc97 to 5d197e9 Compare June 2, 2017 08:51

ericsagnes added 7 commits June 2, 2017 17:54

display manager: use extensible option types

d445fcb

nixos: update display manager usages

d3db9bc

desktop manager: use extensible option types

2eef325

nixos: update desktop manager usages

1521fc7

window manager: use extensible option types

e11c63f

nixos: update window manager usages

0987bea

nixos: select fitting defaults for displayManager and xserver.enable

210a7d9

ericsagnes force-pushed the feat/dm-eot branch from 5d197e9 to a7afa87 Compare June 2, 2017 08:58

nixos: update xserver documentation and usages

21d1dc7

ericsagnes force-pushed the feat/dm-eot branch from a7afa87 to 21d1dc7 Compare June 2, 2017 12:59

ericsagnes changed the title ~~[WIP] displayManager, windowManager, desktopManager: use extensible option types~~ displayManager, windowManager, desktopManager: use extensible option types Jun 2, 2017

nbp suggested changes Jun 3, 2017

View reviewed changes

bjornfor mentioned this pull request Aug 13, 2017

NixOS silently doesn't generate Device sections if videoDriver is not recognized #28235

Closed

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020

domenkozar closed this Jul 20, 2020

Uh oh!

Conversation

ericsagnes commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Uh oh!

mention-bot commented Nov 9, 2016

Uh oh!

FRidh commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danbst commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsagnes commented Dec 3, 2016

Uh oh!

ericsagnes commented Mar 25, 2017

Uh oh!

nbp commented May 26, 2017

Uh oh!

ericsagnes commented May 26, 2017

Uh oh!

nbp commented May 30, 2017

Uh oh!

nbp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsagnes commented Jun 1, 2017

Uh oh!

ericsagnes commented Jun 2, 2017

Uh oh!

nbp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsagnes commented Jun 4, 2017

Uh oh!

nbp commented Jun 10, 2017

Uh oh!

alunduil commented Jul 28, 2019

Uh oh!

stale bot commented Jun 1, 2020

Uh oh!

domenkozar commented Jul 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ericsagnes commented Nov 9, 2016 •

edited

Loading

FRidh commented Nov 9, 2016 •

edited

Loading