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

gdm: refactor to properly handle wayland sessions #76985

Merged
merged 2 commits into from Jan 19, 2020

Conversation

@Elyhaka
Copy link
Contributor

@Elyhaka Elyhaka 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

@Elyhaka Elyhaka force-pushed the Elyhaka:gdm_hides_gnome branch from 64bc4dd to ff40adf Jan 6, 2020
@Elyhaka Elyhaka force-pushed the Elyhaka:gdm_hides_gnome branch from ff40adf to b8bb974 Jan 7, 2020
@ofborg ofborg bot requested a review from hedning Jan 7, 2020
@Elyhaka Elyhaka force-pushed the Elyhaka:gdm_hides_gnome branch from b8bb974 to cafcde4 Jan 7, 2020
@Elyhaka
Copy link
Contributor Author

@Elyhaka Elyhaka 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 Jan 7, 2020
@hedning
Copy link
Contributor

@hedning hedning commented Jan 7, 2020

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

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
Member

@worldofpeace worldofpeace commented Jan 8, 2020

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

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
Member

@worldofpeace worldofpeace commented Jan 8, 2020

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

(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
Member

@worldofpeace worldofpeace commented Jan 8, 2020

I think I fixed the duplicates problem btw #77294.

@hedning
Copy link
Contributor

@hedning 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.

@Elyhaka
Copy link
Contributor Author

@Elyhaka Elyhaka 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 hedning commented Jan 12, 2020

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

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Jan 17, 2020

I've merged #77294

@lovesegfault
Copy link
Contributor

@lovesegfault lovesegfault commented Jan 17, 2020

🎉

@Elyhaka Elyhaka force-pushed the Elyhaka:gdm_hides_gnome branch from cafcde4 to d18ac2b Jan 18, 2020
@Elyhaka Elyhaka marked this pull request as ready for review Jan 18, 2020
@Elyhaka
Copy link
Contributor Author

@Elyhaka Elyhaka commented Jan 18, 2020

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

@hedning
Copy link
Contributor

@hedning hedning commented Jan 18, 2020

@GrahamcOfBorg test gnome3

Copy link
Contributor

@hedning hedning left a comment

LGTM at least :)

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Jan 19, 2020

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.

This actually gets added via xdg.portal having gnome-shell.
However, that could change and it still for sure should be here.
Copy link
Member

@worldofpeace worldofpeace left a comment

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
1 check was pending
1 check was pending
grahamcofborg-eval Checking original out paths
Details
@ofborg ofborg bot requested review from worldofpeace and hedning Jan 19, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Feb 5, 2020

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.