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

Xsessions rework #102322

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Xsessions rework #102322

wants to merge 3 commits into from

Conversation

poscat0x04
Copy link
Contributor

@poscat0x04 poscat0x04 commented Nov 1, 2020 •
edited

Motivation for this change

Note: xcfg := config.services.xserver

A summary of how xsession files are currently generated and used:

  1. Enabled desktop managers and window managers append their startup scripts to xcfg.desktopManager.session and xcfg.windowManager.session resepctively.
  2. One final startup script is generated for every possible pair of desktop managers and window managers by concatenating their startup scripts together and is used to further generate the xsession files. (source) The generated packages are stored in xcfg.displayManager.sessionPackages.
  3. The packages are then combined into a single package (xcfg.displayManager.sessionData) which can be consumed by different display managers (sddm, gdm, etc.)

This generation method is less than ideal because:

  1. It will generate xsession files that start window managers for desktop managers that don't support external window managers (such as gnome)
  2. The generated startup script is overly simplistic: it simply starts the window manager before starting the desktop manager. This ignores the fact that some desktop managers need special treatments when used together with an external window manager. For example, KDE requires the environment variable KDEWM to be set. This also means that the window manager will not have its environment varibles set by the desktop manager, which causes issues like plasma5 + i3 ignore qt theme settings #94482.
Summary of this change

An option supportExternalWM which defaults to false is added for all desktop managers. Desktop managers that support external window managers should set this option to true and provide a function genStart that accepts a package (the binary/startup script of the WM) or null (when the DM is used without an external WM) and returns a string (the final startup script for the DM). The generation method is also changed to adapt to this option.

Note that #100057 will have to change if this pr is merged.

This pr will close #82074, close #94482

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.

@taku0
Copy link
Contributor

taku0 commented Jan 31, 2021

Code-wise LGTM. Please rebase on master.
One concern is renaming option from config.services.xserver.displayManager.session to config.services.xserver.displayManager.dmSession, which may break existing customizations. I'm not sure about compatibility policy of NixOS configuration file.

Poscat added 3 commits February 1, 2021 19:01
nixos/{desktop,display,window}-managers: rework xsessions generation
b4eea4b 
The previous xsessions generation method generates one xession file for
every pair of window managers and desktop managers. The desktops
managers are also agnostic with regards to window manager it is using.
This rework allows finer contol over which desktop managers are allowed
to generate xsession files.
nixos/window-managers/none: remove
bdeaec9
nixos/desktop-managers/{none,plasma5}: allow to use external window m…
7c5f54b 
…anagers
@poscat0x04
Copy link
Contributor Author

rebased.

@poscat0x04
Copy link
Contributor Author

We probably need some more testing. It worked when it was introduced, but I'm not sure if it still works now.

xaverdh
xaverdh reviewed Feb 4, 2021
View reviewed changes
nixos/modules/services/x11/display-managers/default.nix
({dm, wm}: optional dm.supportExternalWM
(let
sessionName = "${dm.name}+${wm.name}";
startup = dm.genStart wm.bin;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be wm.start instead of wm.bin?

Copy link
Contributor

Choose a reason for hiding this comment

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

my system fails to start from this pr (rebased on master) because the waitPID thing fails (the additional indirection introduced when calling the resulting script, hides the variable from the caller). If I replace this with wm.start as it used to be in the old code, the wm startup script gets included verbatim and it works again.

Copy link
Contributor

Choose a reason for hiding this comment

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

@xaverdh could you provide a reproducing configuration?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should do (tested in vm)

{ pkgs, ... }:
{
  users.users.bob.isNormalUser = true;
  services.xserver = {
    enable = true;
    windowManager.xmonad.enable = true;
    displayManager = {
      autoLogin = {
        enable = true;
        user = "bob";
      };
      lightdm = {
        enable = true;
        greeter.enable = false;
      };
    };
  };
}

You can leave out the auto login stuff and log in manually as bob if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

The configuration doesn't work on master (without this PR) on QEMU:

rm -f nixos.qcow2 && nixos-rebuild build-vm -I nixos-config=./configuration.nix -I nixpkgs=. && ./result/bin/run-nixos-vm

Copy link
Contributor

Choose a reason for hiding this comment

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

It works for me from master. What error do you get / in what way does it fail?
Note: If you just get a black screen, then everything is working (xmonad is a rather minimal window manager).

Copy link
Contributor

Choose a reason for hiding this comment

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

I got a black screen. OK, I proceed to testing the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current implementation generates the following script:

#! /nix/store/cwnwyy82wrq53820z6yg7869z8dl5s7g-bash-4.4-p23/bin/bash

# Legacy session script used to construct .desktop files from
# `services.xserver.displayManager.session` entries. Called from
# `sessionWrapper`.

# Start the window manager.
systemd-cat -t xmonad -- /nix/store/4nl2kcgi927xckk4p8lkxpxi4sxd65wy-xmonad-with-packages-8.10.4/bin/xmonad  &
waitPID=$!


# Start the desktop manager.
if [ -e $HOME/.background-image ]; then
  /nix/store/zv8gfabp44k5f1zxl3dkq7pvx6kqvpls-feh-3.6.3/bin/feh --bg-scale  $HOME/.background-image
fi




test -n "$waitPID" && wait "$waitPID"

/run/current-system/systemd/bin/systemctl --user stop graphical-session.target

exit 0

The new implementation generates the following script:

#!/nix/store/cwnwyy82wrq53820z6yg7869z8dl5s7g-bash-4.4-p23/bin/bash
# Legacy session script used to construct .desktop files from
# `services.xserver.displayManager.session` entries. Called from
# `sessionWrapper`.

# Run the startup script.
/nix/store/71ql8j8sdcnc5dpnn35syvy1h4gjwpv4-run-xmonad
if [ -e $HOME/.background-image ]; then
  /nix/store/zv8gfabp44k5f1zxl3dkq7pvx6kqvpls-feh-3.6.3/bin/feh --bg-scale  $HOME/.background-image
fi




test -n "$waitPID" && wait "$waitPID"

/run/current-system/systemd/bin/systemctl --user stop graphical-session.target

exit 0

where /nix/store/71ql8j8sdcnc5dpnn35syvy1h4gjwpv4-run-xmonad is the following.

#!/nix/store/cwnwyy82wrq53820z6yg7869z8dl5s7g-bash-4.4-p23/bin/bash
systemd-cat -t xmonad -- /nix/store/4nl2kcgi927xckk4p8lkxpxi4sxd65wy-xmonad-with-packages-8.10.4/bin/xmonad  &
waitPID=$!

The script sets waitPID but it is not propagated to the parent script.

So, using wm.start (raw script text) instead of wm.bin (script file name), as @xaverdh states, will fix this.

@poscat0x04 could you fix this?

@xaverdh
Copy link
Contributor

xaverdh commented Feb 4, 2021

Apart from the issue mentioned above, this works fine for me.
Using none+xmonad here.

@nrdxp
Copy link
Contributor

nrdxp commented Mar 24, 2021

@poscat0x04, what's the status on this? Could it be made ready for the next release?

@stale
Copy link

stale bot commented Sep 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 21, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 13, 2022
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 2, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank marked this pull request as draft March 20, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taku0 taku0 taku0 approved these changes

@xaverdh xaverdh xaverdh left review comments

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 6.topic: qt/kde 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1
Projects
None yet
Milestone
No milestone
6 participants
@poscat0x04 @taku0 @xaverdh @nrdxp @wegank @Artturin