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

Add gnome wayland support #44497

Merged
merged 7 commits into from Dec 10, 2018

Conversation

@hedning
Copy link
Contributor

hedning commented Aug 5, 2018

Motivation for this change

This adds support for launching gnome wayland sessions from GDM and SDDM, using the upstream session file. See #39871 and #32806 for some more discussion.

Edit: Updated against master now that gnome 3.30 is merged.

sessionCommand support

This should work quite well, but one remaining issue is the lack of support for displayManager.sessionCommands and therefore also gnome3.sessionPath. Some options here:

  • We could patch GDM, gnome-session, 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.

Edit: We landed on simply not supporting sessionCommands on wayland for now.

patching gnome-session more?

6171bc0 might be better expressed by patching gnome-session to always look for session files in its own share/ folder.

Writing a test

Edit: gnome3-gdm now runs a wayland gnome session (GDM prefers the wayland session and we don't have a good way of telling it otherwise), so I ported it to work with wayland.

cc @jtojnar

Things done

Edit:

  • Tested succesfully in a VM (at a6c1ffe).

  • Ran gnome3, gnome3-gdm and sddm tests succesfully.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)

  • Determined the impact on package closure size (by running nix path-info -S before and after)

  • Fits CONTRIBUTING.md.


Show resolved Hide resolved pkgs/desktops/gnome-3/core/gnome-session/fix-paths.patch Outdated
@hedning

This comment has been minimized.

Copy link
Contributor

hedning commented Aug 6, 2018

Replying to #39871 (comment) here instead.

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

Agreed, and in Gnome's case it seems the session is set up correctly with eg. pulseaudio, journal logging and dbus without the wrapper, mostly leaving /etc/profile which is sourced correctly. The only missing things seems to be sessionsCommands and graphical-session.target I think.

Any idea how to get the graphical-session.target running?

For what is worth, sessionPath might actually make sense outside of GNOME (e.g. for elementary), so moving it to /etc/profile should work.

True, there's not really any reason to switch on XDG_CURRENT_SESSION, users probably want functional software if they happen to currently run something else than Gnome. I'm guessing it even might make sense in a console session, as there could be cli tools requiring something exposed in these variables (probably not likely, but it shouldn't hurt).

As for sessionCommands, I would just add a comment it is not supported for sessions created from extraSessionFilePackages.

I'm not opposed to that. It should be noted that sessionCommands are currently sourced for X sessions from extraSessionFilePackages, due to the xsessionWrapper, so it's only wayland sessions that won't run it, which is even less breakage :)

Default session

One more problem, the wayland session have the same name as the xorg session (there's an extra session file that's explicitly for xorg) and seem to take priority. So people might end up «upgrading» to wayland without any warning which could be jarring.

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Aug 6, 2018

Any idea how to get the graphical-session.target running?

Not sure. These might be relevant:

The following presentation looks extra interesting.

https://lists.freedesktop.org/archives/systemd-devel/2018-June/040952.html

True, there's not really any reason to switch on XDG_CURRENT_SESSION, users probably want functional software if they happen to currently run something else than Gnome.

For GNOME Flashback and Elementary, I have changed it to jtojnar@b5b2d8d but removal of the condition might make sense too.

I'm not opposed to that. It should be noted that sessionCommands are currently sourced for X sessions from extraSessionFilePackages, due to the xsessionWrapper.

Right. I would actually expect most of the things in the xsessionWrapper to be in the xsession instead. After all, upstream sessions should be launching most of these themselves, there should only be the Xorg things, see ${gnome3.gdm}/etc/gdm/Xsession.

So people might end up «upgrading» to wayland without any warning which could be jarring.

I would not worry about that. If the Wayland session fails to start for some reason, gnome-xorg will be selected automatically.

Actually, I am not sure GDM even respects default DE nixos option. I think it uses the user’s last selected one.

@hedning

This comment has been minimized.

Copy link
Contributor

hedning commented Aug 6, 2018

Hmm, so looks like we're abusing graphical-session.target. In that case I'm at least fine with breaking that in the wayland session.

Right. I would actually expect most of the things in the xsessionWrapper to be in the xsession instead. After all, upstream sessions should be launching most of these themselves, there should only be the Xorg things, see ${gnome3.gdm}/etc/gdm/Xsession.

Hmm, true, I didn't think that through fully when I wrote it. The wrapper should model gdm's Xsession more closely, though ${sessionCommands} will need to go there too. Perhaps out of scope of this PR, though could of course put it here anyway if it's wanted?

@hedning

This comment has been minimized.

Copy link
Contributor

hedning commented Aug 6, 2018

Another issue I bumped into (dogfooding the PR at the moment):

gnome-session launches a login shell using the user's preferred shell (as opposed to xsessionWrapper sourcing /etc/profile). In my case ZSH.

This results in any new interactive bash shells to source /etc/profile. In particular running nix run nixpkgs.some-package will result in an environment with a PATH that doesn't include the package. nix-shell is unaffected, so this might be a bug in nix, but it's never the less very sub-optimal. We're basically hitting a strange variant of #33219

The problem boils down to $__ETC_PROFILE_DONE never being set.

In lieu of /etc/environment we would probably need some sort of shell agnostic variant of __ETC_PROFILE_DONE that guards against sourcing the environment again for interactive shells.

@jtojnar jtojnar added this to GNOME/GTK specific in Wayland Aug 6, 2018

@jtojnar jtojnar added this to In Progress in GNOME Aug 6, 2018

@jtojnar jtojnar referenced this pull request Aug 15, 2018

Merged

freedesktop modules: init #45058

3 of 9 tasks complete

@hedning hedning referenced this pull request Oct 7, 2018

Open

Add Wayland Support #5071

6 of 13 tasks complete

jtojnar referenced this pull request Oct 15, 2018

gnome3.gdm: fix session chooser
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
@bkchr

This comment has been minimized.

Copy link
Contributor

bkchr commented Nov 11, 2018

Any update on this pull request?

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 11, 2018

This is blocked by #45950 which is blocked by #46020

@nathyong

This comment has been minimized.

Copy link

nathyong commented Dec 4, 2018

What's the status on this now that gnome3 has been updated?

@hedning

This comment has been minimized.

Copy link
Contributor

hedning commented Dec 5, 2018

@nathyong gnome-3.30 is in staging-next, I'll fix up this PR when it hits master, then we should have wayland support pretty soon :)

@hedning hedning force-pushed the hedning:gnome-upstream-wayland branch Dec 9, 2018

@hedning hedning requested a review from Infinisil as a code owner Dec 9, 2018

@hedning

This comment has been minimized.

Copy link
Contributor

hedning commented Dec 9, 2018

Ported the gnome3-gdm test to wayland (it broke since gdm prefers the wayland session. It's not super pretty, but should work, so we'll have test too :)

I think this should be ready now 🎉

@hedning hedning changed the title [WIP] Add gnome wayland support Add gnome wayland support Dec 9, 2018

@jtojnar jtojnar dismissed their stale review Dec 9, 2018

lgtm

gnome3.gnome-session: prevent crash when launching wayland sessions
gnome-session inherits GDMS PATH, which is at the moment non-functional. In X11
this didn't matter as the `Xsession` wrapper would populate the environment
beforehand. Wayland sessions doesn't source `Xesssion` (duh), so we patch
`bin/gnome-session` to use absolute paths for `grep` and `bash`.

In addition `bin/gnome-session` is a simple wrapper around
`libexec/gnome-session-binary` mostly responsible for sourcing the users profile
before launching the binary. This made our wrapping of `bin/gnome-session`
ineffective on wayland as the profile would reset the environment. Simply wrap
`libexec/gnome-session-binary` instead.

hedning added some commits Aug 4, 2018

nixos/sddm: Enable wayland-sessions
LightDM is unable to separate between `wayland-sessions/gnome.desktop` and
`xsessions/gnome.desktop` so I ommitted adding this to LightDM.
nixos/gnome3: Implement `sessionPath` through `environment.extraInit`
This will simply make the `sessionPath` more likely to work.
nixos/tests/gnome3: select X11 gnome shell explicitely
This isn't strictly necessary yet as LightDM doesn't read the wayland sessions,
but there's no harm in being explicit.
nixos/tests/gnome3-gdm: port to wayland
The test now runs wayland, which means we can no longer use X11 style testing.
Instead we get gnome shell to execute javascript through its dbus interface.

@hedning hedning force-pushed the hedning:gnome-upstream-wayland branch to 75e223b Dec 10, 2018

@hedning

This comment has been minimized.

Copy link
Contributor

hedning commented Dec 10, 2018

Found a much simpler fix for gnome-session: simply wrap libexec/gnome-session-binary instead of bin/gnome-session, that way sourcing the profile in gnome-session won't clobber the wrapping environment.

Reran the tests, will merge later today if no one has any objections.

@hedning

This comment has been minimized.

Copy link
Contributor

hedning commented Dec 10, 2018

@GrahamcOfBorg build nixosTests.gnome3-gdm

@hedning hedning merged commit 59d1fb6 into NixOS:master Dec 10, 2018

9 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details

GNOME automation moved this from In Progress to Done Dec 10, 2018

@hedning

This comment has been minimized.

Copy link
Contributor

hedning commented Dec 10, 2018

The test timed out on ofborg, hopefully not due to testing for active gnome shell too early, putting too much pressure on the machine. Lets see how it dose on hydra.

@jtojnar jtojnar referenced this pull request Jan 18, 2019

Open

elementary: init a 5.0 Juno #48637

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment