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

Revert "nixos/desktop-managers/xterm: Disable by default" #70182

Closed
wants to merge 1 commit into from
Closed

Revert "nixos/desktop-managers/xterm: Disable by default" #70182

wants to merge 1 commit into from
+3 −10

Conversation

matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Oct 1, 2019 •
edited
Loading

This reverts commit f140dfb.
This reverts commit cf56cef.
This reverts commit 456c42c.

This has caused more issues than originally intended. It looks like many configurations rely on xterm being set to work correctly. This includes when you are starting a WM without going through a DE. We shouldn't break these setups (if stateVersion is unset). To make matters worse, most users don't actually know they are relying on xterm. LightDM seems to be the main one effected for now:

I expect more issues to come up as users of 19.03 upgrade to 19.09.

Motivation for this change
Things done
  • 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.

@matthewbauer
Revert "nixos/desktop-managers/xterm: Disable by default"
5554d7a 
This reverts commit f140dfb.
This reverts commit cf56cef.
This reverts commit 456c42c.
Ericson2314
Ericson2314 requested changes Oct 1, 2019
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

I don't like hacking around upstream bugs like this, despite having been impacted by this.

@Ericson2314
Copy link
Member

Plus, the state version is there to help with the migration.

@matthewbauer
Copy link
Member Author

I don't like hacking around upstream bugs like this, despite having been impacted by this.

I disagree that this is an upstream bug. The problem is we need some placeholder WM to be available when nothing else has been manually specified. For a long time on NixOS that has been xterm, so might as well keep it available for new users. And these are just defaults, so you can always disable xterm manually if you really don't like it for some reason.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 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 labels Oct 1, 2019
@Ericson2314
Copy link
Member

It looks like both issues were with lightdm? I think it is an upstream bug in that lightdm should give a better error message when there is no selected session. It's a classic "forgot about base case" (0) bug.

@worldofpeace
Copy link
Contributor

Yeah, I honestly don't understand why anyone was ever abusing the xterm session for this purpose.

As for having a placeholder WM there's #68371
Which makes much more sense for a placeholder, as a placeholder shouldn't really be much of anything. Though I'm not sure what's being supported. It feels like supporting users not configuring their systems, why is a placeholder session needed? Why can't these things be coordinated with home-manager to have a placeholder WM so they can start their .xsession?

@worldofpeace
Copy link
Contributor

It looks like both issues were with lightdm? I think it is an upstream bug in that lightdm should give a better error message when there is no selected session. It's a classic "forgot about base case" (0) bug.

It seems that lightdm notoriously gives no feedback in multiple cases of a session not starting.
And in particular here, where no session exists at all. Not sure if that code is greeter specific though.

@timhae
Copy link
Contributor

timhae commented Oct 3, 2019

Having xterm set as default caused a lot of headache on my side when I tried to setup autologin with any display manager. I failed to set desktopManager.default = "none"; because I did not read the documentation of nix options properly.. So essentially I experienced the same issue but in reverse since my window manager (xmonad) is just a little different.

@worldofpeace
Copy link
Contributor

Hmm, it appears at least in lightdm this failure feedback is greeter side

I guess lightdm-gtk-greeter lacks code like that ^ @Ericson2314.

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

@adisbladis adisbladis

@edolstra edolstra

@Ericson2314 Ericson2314

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 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.

None yet
4 participants
@matthewbauer @Ericson2314 @worldofpeace @timhae