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

Jump to bottom

gdm: refactor to properly handle wayland sessions #76985

Merged
merged 2 commits into from Jan 19, 2020
Merged

gdm: refactor to properly handle wayland sessions #76985

merged 2 commits into from Jan 19, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 5, 2020

Motivation for this change

This PR tries to address issue #76169.

It is a draft PR since I'm currently stuck on using systemd.packages as mentioned by @hedning here.

Here is the current state of this PR :

  • What works :

    • Listing GNOME session only if user explicitly enables it or if users enable desktopManager.gnome3
    • Listing Wayland sessions

    If you're using a wayland compositor (like Sway) and you want to use GDM, then it works correctly in the current state.

  • What does not work :

    Having those 2 options enabled :

     displayManager.gdm.enable = true;
 desktopManager.gnome3.enable = true;

I have the feeling it is due to the duplication of gnome-session (one with the exposing session file flag, the other one not) in systemd.packages :

$ nixos-option systemd.packages
Value:
[
  «derivation /nix/store/(hash of not exposed one)-gnome-session-3.34.2.drv»
  «derivation /nix/store/(hash of exposed one)-gnome-session-3.34.2.drv»
]

I'm opening this PR to seek help of where I should be heading to from this point. It clearly is a naive implementation, and if you believe this is not the way it should be fixed we can simply dismiss this PR :).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @lethalman @jtojnar @hedning @worldofpeace

@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos 8.has: module (update) 8.has: package (new) labels Jan 5, 2020
@ofborg ofborg bot requested a review from hedning January 7, 2020 10:17
@ghost
Copy link
Author

ghost commented Jan 7, 2020

I haven't been able to test if it works correctly for standard Gnome setups : I have a build failure on some Gnome3 components on current master.

@ofborg ofborg bot requested a review from hedning January 7, 2020 15:06
@hedning
Copy link
Contributor

hedning commented Jan 7, 2020

With services.xserver.desktopManager.gnome3.enable = true and gdm we'll get a build error around here:

nixpkgs/nixos/modules/system/boot/systemd-lib.nix

Line 150 in 09e3b35

for i in ${toString cfg.packages}; do

The solution with the least resistance is probably adding an option to the gnome desktop that adds gnome-session as a systemd package, adds gnome-shell as a portal and turns portals on (which also adds the systemd stuff) and installs adwaita-icon-theme. Not very pretty though, but works around the limitation of not being able to add duplicate systemd packages.

@worldofpeace
Copy link
Contributor

With services.xserver.desktopManager.gnome3.enable = true and gdm we'll get a build error around here:

nixpkgs/nixos/modules/system/boot/systemd-lib.nix

Line 150 in 09e3b35

for i in ${toString cfg.packages}; do

The solution with the least resistance is probably adding an option to the gnome desktop that adds gnome-session as a systemd package, adds gnome-shell as a portal and turns portals on (which also adds the systemd stuff) and installs adwaita-icon-theme. Not very pretty though, but works around the limitation of not being able to add duplicate systemd packages.

I'd much rather fix the bug of duplicates not working in systemd.packages.
I've ran into it before.

@worldofpeace
Copy link
Contributor

worldofpeace commented Jan 8, 2020 •
edited

Actually, core-shell doesn't add gnome-shell in systemd.packages

nixpkgs/nixos/modules/services/x11/desktop-managers/gnome3.nix

Line 239 in 9e323fb

(mkIf serviceCfg.core-shell.enable {
. It's being added because of being in xdg.portals is totally an accident and we should fix that.

@worldofpeace
Copy link
Contributor

I think I fixed the duplicates problem btw #77294.

@hedning
Copy link
Contributor

hedning commented Jan 8, 2020

It's being added because of being in xdg.portals is totally an accident and we should fix that.

Yeah, with the duplicates being fixed we should add gnome-shell explicitly in core-shell. IIRC I tried adding gnome-shell to systemd.packages while we added systemd support in 3.34 and ran into the problem that gnome-shell was already added through the portal.

@ghost
Copy link
Author

ghost commented Jan 12, 2020

Hello, sorry, I'm a bit short at the moment : I will wait for #77294 to be merged then I will rebase this pull request. I will have more time next week to finish the work on this :).

@hedning
Copy link
Contributor

hedning commented Jan 12, 2020

Yep, makes sense to wait for #77294, this PR should be fine then :)

@worldofpeace
Copy link
Contributor

I've merged #77294 ✨

@lovesegfault
Copy link
Member

🎉

@ghost ghost marked this pull request as ready for review January 18, 2020 09:57
@ghost
Copy link
Author

ghost commented Jan 18, 2020

I've rebased the branch, I'll make some tests throughout the week-end.

@hedning
Copy link
Contributor

hedning commented Jan 18, 2020

@GrahamcOfBorg test gnome3

hedning
hedning approved these changes Jan 19, 2020
View reviewed changes
Copy link
Contributor

@hedning hedning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM at least :)

@worldofpeace
Copy link
Contributor

I've rebased the branch, I'll make some tests throughout the week-end.

Oof, gdm tests are pretty out there. I've had that on my todo list for a while.
It would be a great idea to add them, but I don't think it should block this contribution right away.
I don't believe them to be as straightforward.

@worldofpeace
nixos/gnome3: add gnome-shell to systemd.packages
c6022f7 
This actually gets added via xdg.portal having gnome-shell.
However, that could change and it still for sure should be here.
worldofpeace
worldofpeace approved these changes Jan 19, 2020
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started sway in a vm with

services.xserver.enable = true;
  services.xserver.displayManager.gdm.enable = true;
  programs.sway.enable = true;
  environment.systemPackages = [ pkgs.dmenu ];
  programs.gnome-terminal.enable = true;
  programs.dconf.enable = true;

from gdm.

@worldofpeace worldofpeace merged commit 765afd3 into NixOS:master Jan 19, 2020
@ofborg ofborg bot requested review from worldofpeace and hedning January 19, 2020 01:53
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/january-2020-in-nixos/5771/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar Awaiting requested review from jtojnar

@worldofpeace worldofpeace Awaiting requested review from worldofpeace

@hedning hedning Awaiting requested review from hedning

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hedning @worldofpeace @lovesegfault @nixos-discourse