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

sddm: 0.19.0 -> 0.20.0 #239389

Merged
merged 1 commit into from
Jul 8, 2023
Merged

sddm: 0.19.0 -> 0.20.0 #239389

merged 1 commit into from
Jul 8, 2023

Conversation

K900
Copy link
Contributor

@K900 K900 commented Jun 23, 2023

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@K900
Copy link
Contributor Author

K900 commented Jun 23, 2023

For some reason, kwin_wayland just explodes when running in SDDM for me, but X11 works fine, so I guess this is not a regression?

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

diff LGTM, didn't test

Copy link
Contributor

@raphaelr raphaelr left a comment

Choose a reason for hiding this comment

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

I tested the dwm, dwl, plasma x11, and plasma wayland sessions. All of them worked, even plasma wayland on nvidia.

@aviallon
Copy link
Contributor

aviallon commented Jun 23, 2023

imho this should be backported to 23.05

@K900
Copy link
Contributor Author

K900 commented Jun 23, 2023

Absolutely not. This is over three years of development in a single release.

@raphaelr
Copy link
Contributor

@ofborg build plasma5Packages.sddm
@ofborg test sddm

Copy link
Contributor

@oxalica oxalica left a comment

Choose a reason for hiding this comment

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

Old configuration from 0.19.0 still works with my plasma/wayland setup.


The new introduced greeter-on-wayland requires additional kirigami2 and layer-shell-qt plugins added into sddm's buildInputs, and the following configurations (copied from Arch wiki):

services.xserver.sddm.settings = {
  General.GreeterEnvironment = "QT_WAYLAND_SHELL_INTEGRATION=layer-shell";
  General.DisplayServer = "wayland";
  General.InputMethod = "";
  Wayland.CompositorCommand = "${pkgs.kwin}/bin/kwin_wayland --no-global-shortcuts --no-lockscreen --inputmethod maliit-keyboard --locale1";
};

And it works for me, 🎉 completely without X server! I'm also able to login into and logout from the plasma/wayland session.

I'm not sure if kirigami2 and layer-shell-qt should be wrapped directly into sddm, or should be retrieved from system profile. kirigami2 is already in systemPackages for sddm, but cannot be not picked up by greeter-on-wayland setup. Not really sure why.

Have not yet successfully make the wayland greeter work inside NixOS tests. The greeter just exited any without useful message.

pkgs/applications/display-managers/sddm/default.nix Outdated Show resolved Hide resolved
@K900
Copy link
Contributor Author

K900 commented Jun 24, 2023

Are you sure kirigami is necessary? I can't find any references to it in the code. It might be used by Maliit?

@oxalica
Copy link
Contributor

oxalica commented Jun 24, 2023

Are you sure kirigami is necessary? I can't find any references to it in the code. It might be used by Maliit?

Yes. Without it (also without maliit-keyboard), the greeter shows up for ~0.5s before returning to getty. I guess it might be kwin: https://invent.kde.org/plasma/kwin/-/blob/v5.27.6/CMakeLists.txt?ref_type=tags#L129

@K900
Copy link
Contributor Author

K900 commented Jun 24, 2023

Then we should probably add it to the KWin wrapper instead.

@K900
Copy link
Contributor Author

K900 commented Jun 24, 2023

Also I think we can ignore maliit for now by not passing --inputmethod to kwin, though we'll probably want it eventually for touch users.

@@ -69,6 +69,13 @@ let
Session = autoLoginSessionName;
Relogin = cfg.autoLogin.relogin;
};
} // lib.optionalAttrs cfg.enableKwinWayland {
General = {
GreeterEnvironment = "QT_PLUGIN_PATH=${pkgs.plasma5Packages.layer-shell-qt}/${pkgs.plasma5Packages.qtbase.qtPluginPrefix},QT_WAYLAND_SHELL_INTEGRATION=layer-shell";
Copy link
Contributor

Choose a reason for hiding this comment

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

QT_PLUGIN_PATH here is not applied somehow. Don't know why.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why to force layer-shell when upstream uses fullscreen-shell-v1 (I see it's hardcoded in sddm source code)? I don't think kwin supports layer-shell atm, I heard there were talks about switching to it in Plasma 6, but right now AFAIK kwin supports only Plasma's own analogue, plasma-shell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied the config from upstream Plasma to see if it worked (it didn't), and never got back around to looking into it. I might mess with it more today.

Copy link
Contributor

@Shawn8901 Shawn8901 Jul 1, 2023

Choose a reason for hiding this comment

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

If someone is interested in the link: https://invent.kde.org/plasma/plasma-workspace/-/blob/master/sddm-wayland-session/plasma-wayland.conf . But I also never got that working (when the version was not released) due the error I posted below

Copy link
Contributor

@ilya-fedin ilya-fedin Jul 1, 2023

Choose a reason for hiding this comment

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

sddm code seem to have some special handling of QT_PLUGIN_PATH passing it through processes, so passing it to systemd service might be a better choice. I also don't understand how this is supposed to work given that it doesn't merge the value with the older one but overwrites it, so Qt likely won't even see the wayland platform plugin itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I think I've found where the variable is being reset. sddm uses Qt's QProcessEnvironment::insert(const QProcessEnvironment &e) API shadowing feature a lot (in other words, it inserts a list of variables into another one and if there are conflicts, the newly inserted ones silently get prioritized).

GreeterEnvironment option is being read in HelperApp.cpp. The array is then being shadowed by the result of pam_getenvlist in PamBackend::openSession and by setclassenvironment calls in Backend::openSession.

My guess is QT_PLUGIN_PATH most likely gets overriden by pam_getenvlist as /etc/pam/environment contains QT_PLUGIN_PATH thanks to environment.profileRelativeSessionVariables.

Copy link
Contributor

@ilya-fedin ilya-fedin Jul 7, 2023

Choose a reason for hiding this comment

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

Patching nixpkgs to get rid of QT_PLUGIN_PATH in environment.profileRelativeSessionVariables makes sddm start with Wayland on my machine. But it obviously doesn't see Plasma Wayland session due to the above-mentioned problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

replacing sessionEnv.insert(m_pam->getEnv()); with sessionEnv = m_pam->getEnv().insert(sessionEnv); should also work in theory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I wonder if we should take this upstream then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's a good question, depends on how it's intended to work

@oxalica
Copy link
Contributor

oxalica commented Jun 24, 2023

Also I think we can ignore maliit for now by not passing --inputmethod to kwin, though we'll probably want it eventually for touch users.

Adding kirigami2 to kwin's buildInputs works. Note that it's already a transitive dependency through plasma-framework (that's why Arch works out-of-box). Can we just also propagate it?

@K900
Copy link
Contributor Author

K900 commented Jun 24, 2023

Does kwin depend on Plasma-workspace? If so, yes, we should.

@oxalica
Copy link
Contributor

oxalica commented Jun 24, 2023

Does kwin depend on Plasma-workspace? If so, yes, we should.

Oh, sorry my bad. It's plasma-framework that is both the dependency of kwin and the dependent of kirigami2.

@K900
Copy link
Contributor Author

K900 commented Jun 24, 2023

Yeah, that should definitely propagate kirigami then.

@K900
Copy link
Contributor Author

K900 commented Jul 7, 2023

I'm not sure. Maybe it's specifically trying to grab vt1 for those? Either way, I don't think dropping the conflict will be a regression compared to where we stand, and we can figure out the vt1 situation in a followup.

@ilya-fedin
Copy link
Contributor

We have it (x11-user) already working, why to drop it and make a follow-up? This will just make the feature unavailable for some time without any reason IMO.

@K900
Copy link
Contributor Author

K900 commented Jul 7, 2023

Because making it conflict with getty@tty1 makes SDDM die on any subsequent rebuild, and making it conflict with getty@tty7 probably doesn't actually work.

@ilya-fedin
Copy link
Contributor

Why you say it doesn't work with tty7? It works for @alois31, it works for me.

@K900
Copy link
Contributor Author

K900 commented Jul 7, 2023

Then it should continue working even if we remove the conflict, because those getties are started on-demand.

@alois31
Copy link
Contributor

alois31 commented Jul 7, 2023

It does run on tty7 for me, but it also crashed with that weird PAM error before the getty conflict was added, which seemed not to be the case for @Shawn8901 and @oxalica . Possibly there is a logind configuration that changes the behavior here.

@K900
Copy link
Contributor Author

K900 commented Jul 7, 2023

Then I guess we can merge this as-is, but we'll have to look over our VT handling in general in the future...

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Jul 7, 2023

Apparently some configurations make getty@tty1 running from start. Because it doesn't work for me without such a change. Moreover, sddm even with the current nixos-unstable starts not at every launch for me, maybe due to the same reason, but restarting it helps.

@K900
Copy link
Contributor Author

K900 commented Jul 7, 2023

Wait, which change? With how the branch is right now, we should get a getty on tty1, SDDM on tty2, session on tty3 with rootful X11.

@ilya-fedin
Copy link
Contributor

It also seems somewhat logical to me as the system starts with tty1 enabled, so systemd could just fine start getty with a race condition to sddm

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Jul 7, 2023

Wait, which change?

The SDDM_INITIAL_VT=7. Without this sddm doesn't work in Wayland/x11-user modes for me.

@K900
Copy link
Contributor Author

K900 commented Jul 7, 2023

What VT does the greeter end up on then? 7?

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Jul 7, 2023

What VT does the greeter end up on then? 7?

It says Using VT 7 but I haven't checked what it uses actually. I don't really care as long as it fixes sddm launch after all. sddm has multiple functions to get the VT number, so I can imagine allocated SDDM_INITIAL_VT blocks it from start even though it would actually allocate a new VT when starting the greeter.

@K900
Copy link
Contributor Author

K900 commented Jul 7, 2023

In that case we should probably merge this as-is, and then have followups for Wayland and general VT allocation logic.

@ilya-fedin
Copy link
Contributor

but it also crashed with that weird PAM error before the getty conflict was added

It's not a PAM error, your log says PAM works just fine. SDDM::Auth::HELPER_TTY_ERROR suggests it's a problem with using the tty. I had a more comprehensive error in /var/log/sddm.log where it was saying the tty is already used by root (which is getty) or by me (when I was logged in getty).

@alois31
Copy link
Contributor

alois31 commented Jul 7, 2023

FWIW, Wayland works as well using the following configuration:

services.xserver.displayManager.sddm = {
  enable = true;
  settings = {
    General = {
      DisplayServer = "wayland";
      InputMethod = "";
    };
    Wayland.CompositorCommand = "${pkgs.weston}/bin/weston --shell=fullscreen-shell.so";
  };
};

The greeter is stable and even feels a bit less glitchy than x11-user, but the keyboard layout is US English.

@K900
Copy link
Contributor Author

K900 commented Jul 7, 2023

Iiiinteresting. If we can use Weston here (or maybe even Cage?), that would save us a lot of headache potentially.

@alois31
Copy link
Contributor

alois31 commented Jul 7, 2023

Weston is the upstream default, so it's just what I tried. In fact, the CompositorCommand thing is only required to set the full path.

What should be sorted out though are the input method (without that, I got a full-screen virtual keyboard popping up, and a black screen upon dismissing that), as well as the keyboard layout problem.

@oxalica
Copy link
Contributor

oxalica commented Jul 7, 2023

Iiiinteresting. If we can use Weston here (or maybe even Cage?), that would save us a lot of headache potentially.

I prefer a single common compositor on a system if possible. Wayland compositors handle all GPU driver and input stuff. I'm worrying that if someone has a exotic keyboard and a uncommon nvidia GPU, they may have double headache trying to mess around both, especially when they have different supported devices.

@K900
Copy link
Contributor Author

K900 commented Jul 7, 2023

I do wonder what the intended setup for this is. Like, does upstream expect to keep using weston? Will distros use whatever compositor they already happen to have? Either way, I think we can discuss all of this on the Wayland PR, and I want to get this (or something similar to it) in first.

@K900 K900 merged commit 400aafb into NixOS:master Jul 8, 2023
18 checks passed
@SuperSamus
Copy link
Contributor

#228045 isn't closed yet.

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Aug 10, 2023

#228045 isn't closed yet.

Yeah, sddm can't correctly shutdown Wayland sessions when it uses X11 for greeter, it's a long-standing upstream issue. The only workarounds are to use Wayland for sddm itself or to not to use sddm at all.

@dantefromhell
Copy link
Contributor

Sadly/ weirdly services.xserver.displayManager.sddm has no package option [1], so when running NixOS-23.05 sddm-0.20.0 is not available.

Will sddm-0.20.0 be "backported" (I hope that's the correct term) to 23.05?

Instead the package to use is hardcoded https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/x11/display-managers/sddm.nix#L10

@K900
Copy link
Contributor Author

K900 commented Aug 22, 2023

You can use a nixpkgs overlay instead. Backporting is unlikely.

@dantefromhell
Copy link
Contributor

Would adding a services.xserver.displayManager.sddm.package option be a possibility?

I'm not a huge fan of overlays, and the package option seems to be a common pattern with NixOS services.

@K900
Copy link
Contributor Author

K900 commented Aug 22, 2023

I don't see why not, really.

@ilya-fedin
Copy link
Contributor

It's very, very, very likely to produce regressions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet