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

Investigate using upstream desktop session files #39871

Closed
jtojnar opened this issue May 2, 2018 · 24 comments · Fixed by #53843
Closed

Investigate using upstream desktop session files #39871

jtojnar opened this issue May 2, 2018 · 24 comments · Fixed by #53843
Labels
0.kind: enhancement Add something new 6.topic: freedesktop 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS
Milestone

Comments

@jtojnar
Copy link
Member

jtojnar commented May 2, 2018

GDM distinguishes wayland sessions by them being in wayland-sessions directory, so if we want to support those, we need abandon the flat desktops derivation (af03c20c548a6836e5cc62f3431a4f905f7b564b). GDM_SESSIONS_DIR variable will not be enough either, we would need to add GDM_WAYLAND_SESSIONS_DIR, or even better, load the session files from XDG_DATA_DIRS. I tried that but it picked up session files as well. Perhaps we should get rid of our xsession code and just use the upstream files.

We are doing some extra things but they could be moved to pam or removed completely, see the following conversation from irc.gnome.org#gdm

jtojnar  I still need to figure out why the upstream session files do not work though
halfline yea you'd think the session file would just have Exec=gnome-session in it or whatever
         so as long as PATH was right 
jtojnar  at the moment we execute this monstrosity https://paste.gnome.org/pf4v67irc
         would starting GDM with the variables work, or is the user session launched in a different environment like the greeter?
halfline woah that's quite a script.  yea user session doesn't get the GDM's environment
         you could use pam_env or ~/.config/environment.d/foo.conf to adjust user environment
         well maybe prefix it with a number
         see man environment.d
         or of course you've got a big script
         so you could set environment variables in it
jtojnar  and I suppose running GNOME with a different WM (XMonad) will not work either
halfline as you already do i guess
         no, GNOME is gnome-shell really
         you can't really have gnome without gnome-shell
         but you can tweak gnome-shell with extensions
         there is an xmonad like extension for gnome-shell i think
         (i mean you could run bits of gnome with xmonad but things won't work right, like screen locking etc
jtojnar  I thin some of our users are running XMonad with GNOME Shell
         but I am not sure how that works
halfline sounds unpossible ! 
         </ralph wiggum>
         well i guess yo ucould have two screens 0.0 and 0.1
         and run gnome-shell on 0.0
         and run xmonad on 0.1
         but you souldn't be able to move windows between the two screens
         s/souldn't/wouldn/
jtojnar  apparently, they just mean GNOME services, not the whole shell https://wiki.haskell.org/Xmonad/Using_xmonad_in_Gnome#Gnome3
         or flashback (but we don't have that packaged)
         what is supposed to run xdg-user-dirs-update?
halfline it ships an autostart file
jtojnar  and I suppose pulseaudio can be socket activated
halfline yea that's what upstream pulse recommends these days in fact
jtojnar  not sure what https://paste.gnome.org/pf4v67irc#line-31 is supposed to do
         and launching dbus session (line 45)  will gnome-session take care of
         xrdb-merge probably too
         (line 60)
         what about letting systemd know, graphical-target is on (line 89)?
         maybe using presession hook?
halfline line 31 is restarting the scrip with the output connected to the systemd journal
         to make sure its logged properly i suppose
         line 45 should indeed be handled by gdm before that runs
         or by gnome-session after that runs
         but it's harmless since it won't run because of the test on line 44
         i don't think we do xrdb -merge anywhere these days
         it's affected start up time in the past since it invokes the c preprocessor
         and it's only used by really old applications like xterm
         and some versions of emacs
         you're just trying to clean the script up i guess
@jtojnar jtojnar mentioned this issue May 2, 2018
8 tasks
@jD91mZM2
Copy link
Member

jD91mZM2 commented May 6, 2018

Currently NixOS' own session files aren't sourcing ~/.profile either, which we're in the minority with

@jtojnar jtojnar added 0.kind: enhancement Add something new 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: freedesktop labels May 16, 2018
@hedning
Copy link
Contributor

hedning commented May 22, 2018

I like the idea of using XDG_DATA_DIRS, but for some reason not a single display manager does that by default. They would all require patches apart from lightdm, which can be configured to look for session files in a : separated list of directories. The rest will only look at a single directory for xsessions and one for wayland sessions (gdm looks in a few more directories for xsessions). So there might be too much friction against upstream ☹️

Using pam/systemd to deal with the environment seems like good idea, so we could simplify and possibly remove the huge session script. We could use the wayland sessions as a testing ground for using upstream desktop files perhaps? Though it might just bring more complexity than it's worth to support both at the same time, not sure if it's easy to get pam to do different stuff depending on the session being launched. Also not sure how to deal with launching eg. dbus and pulseaudio?

Another complication using upstream session files might be launching desktop environments with a custom window manager? Though that isn't a problem with wayland.

Somewhat off topic, I'm guessing the most straightforward way of adding wayland support would be bypassing the existing xsession system since it's quite intricate and adds X specific things in a lot of places.

So simply make a separate store path for wayland .desktop files (eg. /nix/store/*wayland-sessions/wayland-sessions/) populated using a separate waylandSession option. The waylandSession wouldn't need to be consolidated in a shared script like xsession, but could rather be desktop specific, possibly with some shared defaults that launches dbus etc. Does that seem like a reasonable way to go?

@jtojnar
Copy link
Member Author

jtojnar commented May 22, 2018

So there might be too much friction against upstream ☹️

GDM (@halfline) is open to merging our XDG_DATA_DIRS patch, not sure about others.

Also not sure how to deal with launching eg. dbus and pulseaudio?

Pulseaudio should be socket activated, dbus session is launched by gnome-session, I think.

Another complication using upstream session files might be launching desktop environments with a custom window manager? Though that isn't a problem with wayland.

I think that will still require custom desktop files – for example, flashback uses RequiredComponents in the desktop file to launch the window manager (one file has metacity, the other compiz). Hopefully, it will be cleaner than the long script thanks to PAM and everything.

Somewhat off topic, I'm guessing the most straightforward way of adding wayland support would be bypassing the existing xsession system since it's quite intricate and adds X specific things in a lot of places.

So simply make a separate store path for wayland .desktop files (eg. /nix/store/*wayland-sessions/wayland-sessions/) populated using a separate waylandSession option. The waylandSession wouldn't need to be consolidated in a shared script like xsession, but could rather be desktop specific, possibly with some shared defaults that launches dbus etc. Does that seem like a reasonable way to go?

I still think the following procedure (i.e. skipping the waylandSession) is the way to go:

  1. Change the xsession system to use $out/share/xsessions and modify NixOS display manager modules to use that path.
  2. Apply the XDG_DATA_DIRS patch to GDM and change the GDM module to use XDG_DATA_DIRS instead of GDM_SESSIONS_DIR.
  3. Incrementally port the xsession system to PAM
  4. Try to make the wayland session work.

@hedning
Copy link
Contributor

hedning commented May 22, 2018

GDM (@halfline) is open to merging our XDG_DATA_DIRS patch, not sure about others.

Ah, that's good.

Pulseaudio should be socket activated, dbus session is launched by gnome-session, I think.

Right, that sounds reasonable. I'm guessing the same goes for all the desktop environments. Stand alone window managers would probably need some custom code, but that should be doable.

I still think the following procedure (i.e. skipping the waylandSession) is the way to go:

It's absolutely a lot cleaner that's for sure so if we could make it work it's preferable. Seems like good plan.

@jtojnar
Copy link
Member Author

jtojnar commented Jun 12, 2018

Also relevant: [gnome-session] Integrate better with systemd: https://bugzilla.gnome.org/show_bug.cgi?id=690866

@jtojnar
Copy link
Member Author

jtojnar commented Jul 22, 2018

Another thing that the current solution fails to do is setting DesktopNames key in the desktop files, making the display manager unable to set XDG_CURRENT_DESKTOP in accordance with the XDG Desktop Entry spec.

@jtojnar
Copy link
Member Author

jtojnar commented Jul 22, 2018

Related: #43984

@jtojnar jtojnar assigned jtojnar and unassigned jtojnar Jul 22, 2018
@jtojnar jtojnar mentioned this issue Jul 23, 2018
16 tasks
@jtojnar
Copy link
Member Author

jtojnar commented Jul 23, 2018

See #43992 for inital attempt.

@jtojnar
Copy link
Member Author

jtojnar commented Jul 23, 2018

There is now an initiative for the systemd session stuff: https://wiki.gnome.org/Initiatives/SystemdUser

@worldofpeace
Copy link
Contributor

Ah, so our work will null soon as expected 🤣
That in theory sounds nicer from a systemd point of view.
And would probably be easier on us in the future.

@bkchr
Copy link
Contributor

bkchr commented Jul 26, 2018

For Plasma on Wayland I would also like to use the upstream desktop files.

@hedning
Copy link
Contributor

hedning commented Aug 3, 2018

Tested wayland using the new upstream session support. Unsurprisingly the /etc/gdm/Xsession script isn't run when a wayland session is launched, and there's no wayland equivalent it seems. So we'll need to use PAM or systemd to populate the environment I think.

Did get it to run though after some troubles trying to patch in the Xsession script (which worked, but the session failed in the same manner as #32806). Simply fixing the SessionManager like in #32806 and wrapping gnome-session with XCURSOR_PATH was enough (in addition to installing wayland-sessions and patching gdm of course). Though without a useful environment obviously: hedning@1af76e6

@hedning
Copy link
Contributor

hedning commented Aug 4, 2018

Okay, think I've figured out the problem.

gnome-session attempts to run a login shell when the session type is wayland: https://gitlab.gnome.org/GNOME/gnome-session/blob/master/gnome-session/gnome-session.in#L3.

  • If it's running without a functional environment it fails to execute the login shell as it can't find eg. grep in the PATH. Ignoring the login shell happens to result in a working wayland session.

  • If we supply gnome-session with a basic $PATH it will run our login shell successfully, sourcing /etc/profile. gnome-session is wrapped with its own share/ directory in XDG_DATA_DIRS, but /etc/profile resets it. Adding ${pkgs.gnome3.gnome-session}/share to XDG_DATA_DIRS in eg. gnome3.nix fixes the problem.

I think the underlying issue was that gnome-session was unable to find its gnome.session file resulting in a botched session.

image

@hedning
Copy link
Contributor

hedning commented Aug 4, 2018

Pushed a cleaned up branch here: https://github.com/hedning/nixpkgs/commits/gnome-upstream-wayland

@jtojnar
Copy link
Member Author

jtojnar commented Aug 4, 2018

Very good job! I opened a merge request with the XDG_DATA_DIRS patch against GDM as discussed. Can confirm it works even with the patch: https://github.com/jtojnar/nixpkgs/commits/gnome-upstream-wayland

@hedning
Copy link
Contributor

hedning commented Aug 5, 2018

Ah, nice, that's much cleaner. I'll rebase and get rid of the GDM_WAYLAND_DIR stuff and open a PR. Guess it's still a work in progress since sessionPath isn't being loaded due to the xsessionWrapper not being run:

  • We could patch gdm to run the wrapper for wayland-sessions too, not super happy about it, but might be good as it makes gdm act more like sddm and lightdm.
  • Or we could move sessionCommands into /etc/profile checking for eg. XDG_SESSION_TYPE, but that seems really ugly.
  • Move more stuff into pam/systemd, which is probably best.

I'm also thinking that it might be better to patch gnome-session to look for its own schema in the proper place, should be more robust.

@hedning hedning mentioned this issue Aug 5, 2018
9 tasks
@jtojnar
Copy link
Member Author

jtojnar commented Aug 6, 2018

I would not call the xsessionWrapper particularly elegant solution either. I would like to use the Xsession provided by GDM in the end.

I guess upstream does not really a way to run different commands per session other than switching on XDG_CURRENT_DESKTOP environment variable.

For what is worth, sessionPath might actually make sense outside of GNOME (e.g. for elementary), so moving it to /etc/profile should work. As for sessionCommands, I would just add a comment it is not supported for sessions created from extraSessionFilePackages.

jtojnar referenced this issue Oct 15, 2018
We are patching GDM to respect GDM_SESSIONS_DIR environment
variable, which we are setting in the GDM module. Previously, we
only took care of a single code path, the one that handled session
start-up; missing the one obtaining the list of sessions.

This commit patches the second code path, and also whitelists the
GDM_SESSIONS_DIR so that it can be passed to the greeter.

Fixes #34101
@matthewbauer matthewbauer added this to the 19.03 milestone Dec 15, 2018
@hedning
Copy link
Contributor

hedning commented Jan 11, 2019

This is basically done right?

@jtojnar
Copy link
Member Author

jtojnar commented Jan 11, 2019

Do we still want to declare the sessions in passthru?

@hedning
Copy link
Contributor

hedning commented Jan 11, 2019

Do we still want to declare the sessions in passthru?

Right, there's that too. Yes, I'm not happy about seeing the trace all time.

I'd say either in passthru or something like this:

extraSessionsFilePackages = [ 
  {
    package: pkgs.gnome3.gnome-session;
    sessions = [ "gnome" "gnome-xorg" ];
  }
];

This might be cleaner, as the it's a bit closer to where it's being used?

Though it should be possible to do some sanity checking in mkDesktops too I think (as that's done at build time).

@hedning
Copy link
Contributor

hedning commented Jan 11, 2019

Passing session names through extraSessionsFilePackages also makes it clear that this is required when adding new extraSessionsFilePackages. It's not very obvious that session names are required in the package passthru. Furthermore it will be easier for users to add an package to extraSessionsFilePackages in configuration.nix without having to modify the package itself.

If that sounds reasonable I can write up a PR.

@jtojnar
Copy link
Member Author

jtojnar commented Jan 12, 2019

This might be cleaner, as the it's a bit closer to where it's being used?

I am not a fan of that, since it moves the responsibility for listing the sessions in the package from packager to user. It might be useful as a filter when package contains more sessions but I think that is a rare use case and would be more of a nuisance most of the time.

Though it should be possible to do some sanity checking in mkDesktops too I think (as that's done at build time).

While technically feasible, I do not think it is a good idea since it mixes two different concerns: making session files available and choosing a default session file.

Passing session names through extraSessionsFilePackages also makes it clear that this is required when adding new extraSessionsFilePackages.

Yeah, I think this should be validated in the module. Perhaps something like

types.listOf (types.package // {
  name = "session-package";
  check = a: types.package.check a && a ? passthru.providedSessions;
})

Furthermore it will be easier for users to add an package to extraSessionsFilePackages in configuration.nix without having to modify the package itself.

I think this really should be packager’s responsibility. Since the session files are a new mechanism there should not be any breakage.

@hedning
Copy link
Contributor

hedning commented Jan 12, 2019

I am not a fan of that, since it moves the responsibility for listing the sessions in the package from packager to user.

Yes, that's the downside. Lets go forward with passthru then 👍

Yeah, I think this should be validated in the module.

That would be good.

While technically feasible, I do not think it is a good idea since it mixes two different concerns: making session files available and choosing a default session file.

Yes, I was thinking of checking that the sessions provided in passthru is actually present, hopefully catching any typos or changing .desktop names as early as possible. I agree that it doesn't make much sense checking the default session there.

Since the session files are a new mechanism there should not be any breakage.

Regarding this, could we rename to the simpler sessionPackages?

I'll try to write something like this up shortly.

@jtojnar
Copy link
Member Author

jtojnar commented Jan 12, 2019

Regarding this, could we rename to the simpler sessionPackages?

I used the extra prefix, since the option complements services.xserver.displayManager.session. But I guess it is not necessary to highlight this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: enhancement Add something new 6.topic: freedesktop 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants