Skip to content
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/displayManager: introduce defaultSession #53843

Merged
merged 6 commits into from Dec 18, 2019

Conversation

hedning
Copy link
Contributor

@hedning hedning commented Jan 12, 2019

Motivation for this change

Integrate services.xserver.displayManager.sessionPackages better with the existing session system.

resolves #54089

services.xserver.displayManager.defaultSession

There's two ways of providing graphical sessions now:

  1. displayManager.session via. desktopManager.session and
    windowManager.session
  2. displayManager.sessionPackages

sessionPackages doesn't make a distinction between desktop and window
managers. This makes selecting a session provided by a package using
desktopManager.default nonsensical.

We therefor introduce displayManager.defaultSession which can select a session
from either displayManager.session or displayManager.sessionPackages.

It will default to desktopManager.default + windowManager.default as before.
If the dm default is "none" it will select the first provided session from
sessionPackages.

services.xserver.displayManager.sessionPackages

Note extraSessionFilePackages is renamed to the less verbose sessionPackages.

When adding a package, foo, to sessionPackages session names are now required to be specified in foo.passthru.providedSessions, this is used to validate defaultSession at evaluation time.

At build time mkDesktops will check that the sessions provided by a package is actually present, bailing out if not, hopefully preventing typos etc.

closes #39871

Things done

Ran these tests succesfully:

  • gnome3
  • sddm
  • plasma5
  • lightdm
  • xfce

After default options deprecation:

  • gdm+xfce+openbox
  • lightdm+xfce+openbox
  • lightdm+gnome-flashback-metacity

@jtojnar
Copy link
Contributor

jtojnar commented Jan 12, 2019

Should we perhaps deprecate desktopManager.default and windowManager.default?

@hedning
Copy link
Contributor Author

hedning commented Jan 12, 2019

Hmm, perhaps, but I'd imagine they're both used quite heavily. Also recreating the the desktopManager.default+windowManager.default manually also seems like chore, though I'm not certain how common it us to mix those in reality.

@hedning hedning changed the title nixos/displayManager: introduce sessionDefault nixos/displayManager: introduce defaultSession Jan 14, 2019
@hedning
Copy link
Contributor Author

hedning commented Jan 14, 2019

We should probably add some documentation too.

@hedning
Copy link
Contributor Author

hedning commented Jan 14, 2019

Added share/wayland-sessions to lightdm's sessions-directory, this won't work with gnome due to duplicate names, but is necessary when having a default session which is only found there. It should also be useful for sway et. al. Slim doesn't seem to support default session.

@aanderse
Copy link
Member

aanderse commented Aug 4, 2019

ping (triage)

@worldofpeace
Copy link
Contributor

Last time I checked @hedning was rethinking this approach.

@hedning
Copy link
Contributor Author

hedning commented Sep 18, 2019

So it's possible to reuse desktopManager.default for selecting sessions from sessionPackages. But we still need to construct a defaultSession for lightdm/sddm which require the same logic as in displayManager.defaultSession (ie. checking if the name cannot be found in displayManager.session.names it's available in displayManager.sessionPackages).

So thinking on this some more I think it might be better to stick with an explicit defaultSession that the various display managers can use without trouble. The downside would be users who've set desktopManager.default = "gnome" which now would fail with an unhelpful error message. And somewhat a proliferation of similar options.

So I guess the question is this:
Do we want to select default sessions using desktopManager.default (and windowManager.default) or add a unified option that can override these?

@ofborg ofborg bot added the 6.topic: pantheon The Pantheon desktop environment label Sep 18, 2019
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/wayland-sessions/4315/6

@jtojnar
Copy link
Contributor

jtojnar commented Dec 14, 2019

Yeah, I noticed that too, though on unstable it retains lightdm background for me for some reason.

hedning and others added 6 commits December 15, 2019 04:14
We want access to the valid session names at evaluation time.
There's two ways of providing graphical sessions now:
- `displayManager.session` via. `desktopManager.session` and
  `windowManager.session`
- `displayManager.sessionPackages`

`sessionPackages` doesn't make a distinction between desktop and window
managers. This makes selecting a session provided by a package using
`desktopManager.default` nonsensical.

We therefor introduce `displayManager.defaultSession` which can select a session
from either `displayManager.session` or `displayManager.sessionPackages`.

It will default to `desktopManager.default + windowManager.default` as before.
If the dm default is "none" it will select the first provided session from
`sessionPackages`.
Note: can't launch gnome on wayland due to duplicate entry names:
  canonical/lightdm#16
The upstream session files display managers use have no concept of sessions being composed from
desktop manager and window manager. To be able to set upstream session files as default
session, we need a single option. Having two different ways to set default session would be confusing,
though, so we decided to deprecate the old method.

We also created separate script for each session, just like we already had a separate desktop
file for each one, and started using displayManager.sessionPackages mechanism to make the
session handling more uniform.
@jtojnar
Copy link
Contributor

jtojnar commented Dec 15, 2019

Rebased onto the sway changes and squashed the commits. Is there anything remaining to do? I successfully ran gnome-flashback-metacity from GDM, and none+xmonad and pantheon from LightDM, and successfully ran all the DE/DM tests.

@worldofpeace
Copy link
Contributor

It's good with me @jtojnar, and mostly because I don't think I can look over it again differently 😄

I'd say, maybe one more reviewer with a different set of eyes.
If waiting doesn't help we can just merge.

@hedning
Copy link
Contributor Author

hedning commented Dec 15, 2019

Went over the new code and it looks good to me. 👍 on deprecating the old default options and implementing legacy sessions through sessionPackages. Thanks for bringing this to the finish line :)

@worldofpeace
Copy link
Contributor

@Profpatsch Agreed to QA this by using it.

@grahamc
Copy link
Member

grahamc commented Dec 16, 2019

✔️ I upgraded from 19.09 to nixos-unstable, and added this to my configuration:

services.xserver = {
      enable = true;
      libinput.enable = true;
      displayManager.sddm.enable = true;
};

After deleting an old ~/.config/i3/config, sway started just fine out of the box. After this change my gpg pinentry windows changed to be more plasma-ey, but I'm assuming that isn't unsurprising. I did have to select "sway" in the drop-down by hand each boot, but since I erase my / on every boot I'm assuming that is normal too :).

@hedning
Copy link
Contributor Author

hedning commented Dec 16, 2019

@grahamc: with this pr LightDM will launch wayland sessions (just double checked that it works with sway), and you'll be able to use displayManager.defaultSession to force LightDM to select the sway session by default :)

@worldofpeace
Copy link
Contributor

@grahamc: with this pr LightDM will launch wayland sessions (just double checked that it works with sway), and you'll be able to use displayManager.defaultSession to force LightDM to select the sway session by default :)

This reminds me I have a branch with canonical/lightdm@03f2189 on top of this. Will PR this after, I'm not sure how it works for sway without the commit though (perhaps it uses the key in the session that forces it)

@xaverdh
Copy link
Contributor

xaverdh commented Dec 17, 2019

Just tried this with my xmonad + lightdm autologin setup. Works perfectly. I was a bit surprised to get an xterm window though, when not explicitly setting defaultSession to "none+xmonad"
(it chose "xterm+xmonad" otherwise) even though I set services.xserver.desktopManager.xterm.enable to false. I guess xterm somehow always winds up in the sessions?

@xaverdh
Copy link
Contributor

xaverdh commented Dec 17, 2019

Ah sorry services.xserver.desktopManager.xterm.enable was not set to false (explicitly). The defaultText here is misleading. When stateVersion is older then "19.09" it defaults to xSessionEnabled, not false..
EDIT: The issue with the code is that the manual gets generated on the hydra builders, not locally, so the stateVersion used is the one from the builder. This yields an incorrect defaultText.

@worldofpeace
Copy link
Contributor

worldofpeace commented Dec 17, 2019

@xaverdh Autologin requries a defaultSession to be selected. So I believe it choose head cfg.displayManager.sessionData.sessionNames when you didn't set one. I believe the option description describe this well. As for #53843 (comment), we hope to eliminate this in 20.03 (default to false) #68371.

@jtojnar jtojnar merged commit 6be14ee into NixOS:master Dec 18, 2019
@hedning hedning deleted the session-default branch December 20, 2019 15:02
worldofpeace referenced this pull request Feb 10, 2020
This module allows root autoLogin, so we would break that for users, but
they shouldn't be using it anyways. This gives the impression like auto
is some special display manager, when it's just lightdm and special pam
rules to allow root autoLogin. It was created for NixOS's testing
so I believe this is where it belongs.
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/setting-up-urxvt-i3/7847/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce services.xserver.displayManager.defaultSession Investigate using upstream desktop session files
10 participants