services.xserver: assert that either desktop- or window manager is not "none" #30517

Ma27 · 2017-10-17T18:01:38Z

Motivation for this change

setting a DM or WM is something I tend to forget when I want to built test machines with an X11 inside, so this assertion might catch unexpected behavior when building machines with X.

resolves #11064 (/cc @nbp )

I tested this against the following expressions:

{
  # VMs without X11 still evaluate
  empty = { pkgs, ... }: {
  };

  # needs to be commented out to avoid evaluation errors
  # no DM/WM set, should break
  onlyX11 = { pkgs, ... }: {
    services.xserver.enable = true;
  };

  # DM set, evaluates
  x11WithDM = { pkgs, ... }: {
    services.xserver = {
      enable = true;
      desktopManager.plasma5.enable = true;
      desktopManager.default = "plasma5";
    };
  };

  # WM set, evaluates
  x11WithWM = { pkgs, ... }: {
    services.xserver = {
      enable = true;
      windowManager.i3.enable = true;
      windowManager.default = "i3";
    };
  };

  # plain X sessions must be enabled explicitly
  x11Plain = { pkgs, ... }:{
    services.xserver.enable = true;
    services.xserver.plainX = true;
  };
}

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

rycee · 2017-10-17T20:54:16Z

I'm not fond of this. Especially since it breaks my setup, which uses ~/.xsession. My complete services.xserver configuration for my laptop, including irrelevant stuff is

  services.xserver = {
    enable = true;
    layout = "us";
    displayManager.lightdm.enable = true;
    monitorSection = ''
      # Set the right DPI. Screen is 344x193 mm.
      DisplaySize 344 193
    '';
    libinput.enable = true;
  };

vyp · 2017-10-17T23:53:59Z

But what if we just want to use a plain X session?

Ma27 · 2017-10-18T20:48:54Z

interesting, didn't know there are people using X without a DM or a WM. Just out of interest: what's the reason for a plain X session?

rycee · 2017-10-18T21:23:23Z

I'm using Home Manager to manage my X session. It mainly uses systemd user services started from ~/.xsession.

vyp · 2017-10-18T21:37:25Z

@Ma27 I'm not using without a DM/WM, but I find it quite useful as a debug step sometimes to figure out if an issue is due to the window manager, or say a regression in X sometimes (and obviously knowing that can save a lot of time). Also helps with debugging say a video driver with not so good linux kernel support, since you can rule out potential effects of DM/WM. (And the NixOS generations feature makes this easy to test as well.) I'm not opposed to say some configuration option that allows this, since my use case is admittedly rare, I just don't want it to be impossible. Edit: That way, the user has explicitly made clear that they know what they're doing. And maybe the evaluation error could also mention that you would need to set an option for this, somehow?

Ma27 · 2017-10-22T19:26:01Z

@vyp @rycee thanks a lot for your explanations.

I'm not opposed to say some configuration option that allows this

so you mean a configuration option like services.xserver.plain which defaults to false and needs to be set to true explicitly?

vyp · 2017-10-22T21:30:53Z

so you mean a configuration option like services.xserver.plain which defaults to false and needs to be set to true explicitly?

Yep 👍

Ma27 · 2017-10-23T04:47:03Z

@rycee if you're fine with this, I'd implement that additional option.

If not, I'd consider closing this for now :-)

Ma27 · 2017-10-23T10:25:57Z

@vyp just found some time to implement this. Would you mind having a look at the change to confirm that this is a better solution? :)

rycee · 2017-10-23T10:30:51Z

Yeah, I think this is a decent compromise. I would prefer a name more along the lines of allowXSessionFile or allowXSessionOverride but it doesn't really matter.

vyp · 2017-10-23T10:40:55Z

@Ma27 unfortunately am really busy (time-sensitive) for at least a few days so afraid not right now. :/ (only just have time for making short responses from my phone when traveling tbh, if you're curious)

Ma27 · 2017-10-23T17:16:10Z

rebased onto latest master which should fix the build.

Yeah, I think this is a decent compromise. I would prefer a name more along the lines of allowXSessionFile or allowXSessionOverride but it doesn't really matter.

I don't like this as setting a DM/WM doesn't prohibit overriding the XSession file. The only reason why this option exists is to "tell" Nix whether plain X sessions without a DM/WM are allowed.

(only just have time for making short responses from my phone when traveling tbh, if you're curious)

ok, I'll try to summarize it for you: I added an option services.xserver.plainX (default=false) which allows to set neither a WM nor a DM if true.

rycee · 2017-11-01T22:13:05Z

I'm fine with this. But I am thinking that it warrants a release note since it is a change that will break some people's installs. In particular, it should be clear what steps you need to take (enable services.xserver.plainX) to get things working again.

Ma27 · 2017-11-01T22:23:01Z

makes sense, will add this :-)

Ma27 · 2017-11-01T22:36:25Z

@rycee done :-)

rycee · 2017-11-01T22:56:58Z

Thanks!

nbp · 2017-11-02T01:27:21Z

nixos/modules/services/x11/xserver.nix

+        type = types.bool;
+        default = false;
+        description = ''
+          Whether the X11 session can be plain (without DM/WM) or not.


This description should explain that the Xsession is then used as a fallback.

Ma27 · 2017-11-02T07:48:33Z

@nbp done :-)

nbp · 2017-11-02T19:24:40Z

nixos/modules/services/x11/xserver.nix

@@ -552,6 +561,9 @@ in
                + "${toString (length primaryHeads)} heads set to primary: "
                + concatMapStringsSep ", " (x: x.output) primaryHeads;
      })
+      { assertion = cfg.desktopManager.default == "none" && cfg.windowManager.default == "none" -> cfg.plainX;
+        message = "Either the desktop manager or the window manager shouldn't be `none`!";


The best error message is the error message which let you fix the issue which is being reported.
This message should include some hint to fix the configuration.

agreed. Added another sentence which mentions the plainX option ...

…t "none" resolves NixOS#11064

NeQuissimus · 2017-11-04T15:31:53Z

hmm, I am quite unhappy with this (since my rebuild just broke)
If I enable only a single windowmanager (i.e. xmonad), why would I need to explicitly define default = "xmonad"?

Some sort of default should be figured out automatically.
Can we give the modules something like default = mkDefault "xmonad"; ?

Ma27 · 2017-11-04T15:43:10Z

I'm not 100% sure what you mean, sorry.
Could you please share the relevant parts of you config? :-)

NeQuissimus · 2017-11-04T19:03:06Z

This is what I do to add XMonad to any config of mine:
https://github.com/NeQuissimus/DevSetup/blob/master/nixos-xmonad.nix#L42

I am already defining that I want to enable xmonad. Why should I have to add it again to "default"?

Ma27 · 2017-11-04T19:14:59Z

I added an option services.xserver.plainX to explicitly allow this.

Can we give the modules something like default = mkDefault "xmonad"; ?

Sounds good, will think about this, however my I'm not sure how many time I'll have for this in the next days...

nbp · 2017-11-04T21:39:05Z

How is that possible to have both default to none, when there is one window manager enabled? I thought this code used to pick the only active element as the default.

I guess we should back it out, and figure out the issue with the default computation.

Ma27 · 2017-11-04T21:44:32Z

enable a WM/DM != setting a default value.
I think that's why it has been suggested to set a default when enabling a WM/DM.

nbp · 2017-11-04T21:47:43Z

I think the default should be the only enabled WM/DM if any, and otherwise be the first one, or none if multiple.

Ma27 · 2017-11-04T21:55:33Z

makes sense, will open a PR in the next days for that...

@nbp

…er is not "none"" This reverts commit 93c54ac. This reopens #30517 @nbp @Ma27 Breaking people's config for this is hardly reasonable as is. If it absolutely cannot be avoided, at the very least, we need to provide clear instructions on what people need to upgrade in their config. I actually had to bisect to the commit, to even find out what property I should change or define, as the error message was useless. It didn't even mention a property name. Discussion on the PR seems to be ongoing, so I'm reverting this, so we don't break people's systems on unstable.

Ma27 force-pushed the x11/assert-that-either-dm-or-wm-is-not-none branch from bb38700 to 59ff939 Compare October 23, 2017 10:24

Ma27 force-pushed the x11/assert-that-either-dm-or-wm-is-not-none branch from 59ff939 to 2ae8633 Compare October 23, 2017 17:07

Ma27 force-pushed the x11/assert-that-either-dm-or-wm-is-not-none branch from 2ae8633 to 86625cd Compare November 1, 2017 22:35

Ma27 requested a review from nbp as a code owner November 1, 2017 22:35

nbp approved these changes Nov 2, 2017

View reviewed changes

nbp added 0.kind: enhancement 6.topic: nixos 6.topic: user experience 8.has: changelog labels Nov 2, 2017

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 2, 2017

Ma27 force-pushed the x11/assert-that-either-dm-or-wm-is-not-none branch from 86625cd to a5244cf Compare November 2, 2017 07:48

nbp reviewed Nov 2, 2017

View reviewed changes

services.xserver: assert that either desktop- or window manager is no…

2752358

…t "none" resolves NixOS#11064

Ma27 force-pushed the x11/assert-that-either-dm-or-wm-is-not-none branch from c2388e0 to 2752358 Compare November 2, 2017 20:49

nbp approved these changes Nov 4, 2017

View reviewed changes

nbp merged commit 93c54ac into NixOS:master Nov 4, 2017

Ma27 deleted the x11/assert-that-either-dm-or-wm-is-not-none branch November 4, 2017 14:35

Ma27 mentioned this pull request Nov 5, 2017

services.xserver: fix defaults of X11 #31268

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

services.xserver: assert that either desktop- or window manager is not "none" #30517

services.xserver: assert that either desktop- or window manager is not "none" #30517

Ma27 commented Oct 17, 2017 •

edited

rycee commented Oct 17, 2017

vyp commented Oct 17, 2017

Ma27 commented Oct 18, 2017

rycee commented Oct 18, 2017

vyp commented Oct 18, 2017 •

edited

Ma27 commented Oct 22, 2017

vyp commented Oct 22, 2017

Ma27 commented Oct 23, 2017

Ma27 commented Oct 23, 2017

rycee commented Oct 23, 2017

vyp commented Oct 23, 2017

Ma27 commented Oct 23, 2017

rycee commented Nov 1, 2017 •

edited

Ma27 commented Nov 1, 2017

Ma27 commented Nov 1, 2017

rycee commented Nov 1, 2017

nbp Nov 2, 2017

Ma27 commented Nov 2, 2017

nbp Nov 2, 2017

Ma27 Nov 2, 2017

NeQuissimus commented Nov 4, 2017

Ma27 commented Nov 4, 2017

NeQuissimus commented Nov 4, 2017

Ma27 commented Nov 4, 2017

nbp commented Nov 4, 2017

Ma27 commented Nov 4, 2017

nbp commented Nov 4, 2017

Ma27 commented Nov 4, 2017

services.xserver: assert that either desktop- or window manager is not "none" #30517

services.xserver: assert that either desktop- or window manager is not "none" #30517

Conversation

Ma27 commented Oct 17, 2017 • edited

Motivation for this change

Things done

rycee commented Oct 17, 2017

vyp commented Oct 17, 2017

Ma27 commented Oct 18, 2017

rycee commented Oct 18, 2017

vyp commented Oct 18, 2017 • edited

Ma27 commented Oct 22, 2017

vyp commented Oct 22, 2017

Ma27 commented Oct 23, 2017

Ma27 commented Oct 23, 2017

rycee commented Oct 23, 2017

vyp commented Oct 23, 2017

Ma27 commented Oct 23, 2017

rycee commented Nov 1, 2017 • edited

Ma27 commented Nov 1, 2017

Ma27 commented Nov 1, 2017

rycee commented Nov 1, 2017

nbp Nov 2, 2017

Choose a reason for hiding this comment

Ma27 commented Nov 2, 2017

nbp Nov 2, 2017

Choose a reason for hiding this comment

Ma27 Nov 2, 2017

Choose a reason for hiding this comment

NeQuissimus commented Nov 4, 2017

Ma27 commented Nov 4, 2017

NeQuissimus commented Nov 4, 2017

Ma27 commented Nov 4, 2017

nbp commented Nov 4, 2017

Ma27 commented Nov 4, 2017

nbp commented Nov 4, 2017

Ma27 commented Nov 4, 2017

Ma27 commented Oct 17, 2017 •

edited

vyp commented Oct 18, 2017 •

edited

rycee commented Nov 1, 2017 •

edited