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.

Sign up for GitHub

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

nixos/lightdm: do not lock up with plymouth #71061

Merged
merged 1 commit into from
Oct 13, 2019
Merged

nixos/lightdm: do not lock up with plymouth #71061

merged 1 commit into from
Oct 13, 2019
+3 −2

Conversation

hedning
Copy link
Contributor

@hedning hedning commented Oct 12, 2019

Motivation for this change

Having display-manager conflict with plymouth causes this lock up:

  • plymouth-quit-wait starts up, waiting for plymouth-quit to run
  • lightdm starts up
  • plymouth-quit can't start, it conflicts with lightdm
  • plymouth-quit-wait keeps waiting on plymouth-quit to kill plymouthd

This config was borrowed from GDM, but doesn't work with LightDM as GDM fixes
the lock by killing plymouth itself.

fixes #71034

Things done

Confirmed in a VM that lightdm + plymouth works.

  • 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 nix-review --run "nix-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 @

Sorry, something went wrong.

@worldofpeace
Copy link
Contributor

I wonder how fedora doesn't have this issue.
Is this patch relevant canonical/lightdm#3. They added it when using this.

@hedning
Copy link
Contributor Author

hedning commented Oct 12, 2019 •
edited
Loading

At a quick glance it looks like we aren't patching the path to plymouth:
https://github.com/canonical/lightdm/blob/ff1c38c0616b3df66a5002ed7f531d53f218dfab/src/plymouth.c#L25-L35

@worldofpeace
Copy link
Contributor

Hmm, @hedning I do think it runs plymouth --quit. I typically see in the logs that it can't find the path to the executable. (even when enabled).

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 12, 2019
@hedning
Copy link
Contributor Author

hedning commented Oct 12, 2019

Right, plymouth is in systemPackages. Then I'm really not sure, but it sure looks like it doesn't kill it for some reason 😕

@worldofpeace
Copy link
Contributor

worldofpeace commented Oct 12, 2019 •
edited
Loading

This probably never happens because plymouth --ping doesn't work https://github.com/canonical/lightdm/blob/95e48f409295bc3ff9dbf3d1affd0fcd7a6d1698/src/seat-local.c#L119

@worldofpeace
Copy link
Contributor

It's weird, shouldn't the display manager service have /run/current-system/sw/bin in PATH?
And for sure now there's the PAM env. Perhaps hardcoding it is an option. Hopefully harmless when it's not enabled as a service.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 12, 2019
@worldofpeace worldofpeace mentioned this pull request Oct 13, 2019
Merged
10 tasks
@worldofpeace
Copy link
Contributor

worldofpeace commented Oct 13, 2019 •
edited
Loading

@hedning with #71064 I think it's fine to merge this.

Can you also edit your commit message to clarify this issue was triggered because plymouth handling in lightdm was already broken in nixos?

@hedning hedning force-pushed the fix-lightdm-plymouth branch from 7f412e6 to 632469f Compare October 13, 2019 07:15
@hedning
nixos/lightdm: do not conflict with plymouth
1cf6000 
Having `display-manager` conflict with plymouth causes this lock up:

 - `plymouth-quit-wait` starts up, waiting for plymouth-quit to run
 - `lightdm` starts up
 - `plymouth-quit` can't start, it conflicts with lightdm
 - `plymouth-quit-wait` keeps waiting on plymouth-quit to kill plymouthd

The idea is having LightDM control when plymouth quits, but communication with
plymouth was broken: NixOS#71064

Unfortunately having the conflict breaks switching to configurations with
plymouth enabled. So we still need to remove the conflict.

fixes NixOS#71034
@hedning hedning force-pushed the fix-lightdm-plymouth branch from 632469f to 1cf6000 Compare October 13, 2019 07:30
@hedning
Copy link
Contributor Author

hedning commented Oct 13, 2019

Right, did a quick test of #71064 to see if lightdm had the same switch configuration issue as gdm, which was the case, so we really need to remove the conflict.

@hedning hedning merged commit d15e5b0 into NixOS:master Oct 13, 2019
@worldofpeace
Copy link
Contributor

@hedning I actually also tested using tty1 with lightdm and there wasn't issues. So perhaps there's something up in GDM's source or the service config.

@hedning
Copy link
Contributor Author

hedning commented Oct 13, 2019

@worldofpeace hmm, could have something to do with the lightdm greeter sticking around while gdm's greeter gets reaped while not in use? Just spitballing though, I mean the display-manager service sticks around in both cases so...

@worldofpeace worldofpeace mentioned this pull request Feb 29, 2020
Merged
9 tasks
@hedning hedning deleted the fix-lightdm-plymouth branch March 1, 2020 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

plymouthd consumes 60% CPU on one core
2 participants
@hedning @worldofpeace