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 restrictive validation behavior for DM/WM defaults in the X module #37276

Closed
wants to merge 1 commit into from
Closed

Revert restrictive validation behavior for DM/WM defaults in the X module #37276

wants to merge 1 commit into from

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 17, 2018 •
edited

The original idea behind this change (described in ticket #11064) was to
improve the assertions to avoid that users of the X server accidentally
forget to configure a DM or WM.

However this caused several issues with setups that require X, but no DM
or WM. The keymap testcases became instable as well as now disabling DMs
needs to be done explicitly.
(see #31268 (comment))

In the end the idea behind the change and #11064 was obviously a
mistake, so reverting it completely for now should be fine.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@Ma27
Copy link
Member Author

Ma27 commented Mar 17, 2018

/cc @nbp @NeQuissimus @bendlas @PierreR I did a full revert, could you please have a look at this?

Note for the merger: as this is subjected to fix breackage, can we please do a backport to release-18.03?

@Ma27 Ma27 mentioned this pull request Mar 17, 2018
Merged
8 tasks
jtojnar
jtojnar requested changes Mar 17, 2018
View reviewed changes
nixos/doc/manual/release-notes/rl-1803.xml Outdated
@@ -247,14 +247,6 @@ following incompatible changes:</para>
overhead of a go package having non-versioned dependencies.
</para>
</listitem>
<listitem>
<para>
When using <option>services.xserver.libinput</option> (enabled by default in GNOME),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is from #31995, it should not be removed.

@Ma27
Revert restrictive validation behavior for DM/WM defaults in the X mo…
fe7bd82 
…dule

The original idea behind this change (described in ticket NixOS#11064) was to
improve the assertions to avoid that users of the X server accidentally
forget to configure a DM or WM.

However this caused several issues with setups that require X, but no DM
or WM. The keymap testcases became instable as well as now disabling DMs
needs to be done explicitly.
(see NixOS#31268 (comment))

In the end the idea behind the change and NixOS#11064 was obviously a
mistake, so reverting it completely for now should be fine.
@Ma27
Copy link
Member Author

Ma27 commented Mar 23, 2018

@jtojnar @bendlas @PierreR I could use some feedback as I guess that we want to have this in 18.03 %)

@nbp
Copy link
Member

nbp commented Mar 24, 2018

Why couldn't we use the services.xserver.plainX option in this case?

@bendlas
Copy link
Contributor

bendlas commented Mar 24, 2018 •
edited

@Ma27 I have applied this PR to my system and everything seems fine. When looking through the patch, I didn't see anything, that was off and you already took care of @jtojnar 's feedback, right? So .. LGTM

On a personal note: I suppose it can't have been easy pulling back on this, when you already put your time into developing it. I want to thank you and commend you for being able to stay cool about everything, even when the discussion got heated. I'm glad we share a community.

@nbp my most concise answer is: Nobody seems to want to own this feature and deal with the tickets falling out from it, including applying all the necessary plainX across nixos. Do you?

My slightly longer answer is: You want to require users, whose configurations have worked so far, to add a flag to express that they want to keep their configuration working? From the concepts I recognize, this is called a deprecation. So you want to deprecate undeclared X sessions. For the gain of preventing newbie mistakes. OK. I have a couple of questions:

  • What alternatives for preventing this particular mistake have been discussed?
  • How big is this problem, in the space of common newbie mistakes?
  • Why start with a hard error?
  • What's the set of modules affected by this change?

@vcunat it seems, you were a proponent of #11064
Have you been following?

@vcunat
Copy link
Member

vcunat commented Mar 25, 2018 •
edited

I haven't been following this lately, so I can't really have a strong opinion. I was (and am) a "proponent" of the high-level intention – to avoid users not having any WM or DM unless they really know they don't want it. There surely are multiple approaches how to reduce the risk, e.g. good comments in the generated initial config can cover part of that.

@Ma27
Copy link
Member Author

Ma27 commented Mar 26, 2018

I have applied this PR to my system and everything seems fine. When looking through the patch, I didn't see anything, that was off and you already took care of @jtojnar 's feedback, right? So .. LGTM

Yes, I just screwed up locally due to some merge conflicts and accidentally removed the wrong entry from the release notes, it's fixed now :-)

On a personal note: I suppose it can't have been easy pulling back on this, when you already put your time into developing it. I want to thank you and commend you for being able to stay cool about everything, even when the discussion got heated. I'm glad we share a community.

first of all I really appreciate that you add this sort of personal note! The thing is, I obviously underestimated the consequences of my patch(es) for the X server and I'd like to avoid any unnecessary breackage (several people asked to fix/revert this in the second PR so I decided to go ahead for now). In case I have more time we could discuss even more about this, but right now I unfortunately have to assign this job to other contributors.

I think that the aspect with the unnecessary high breackage @bendlas stated here and in the original PRs is why we should do the revert for now (and backport the revert to 18.03 to avoid any more unexpected breackage). Then we could reopen #11064 and continue discussing there to find a better solution for 18.09, right?

@bendlas
Copy link
Contributor

bendlas commented Mar 28, 2018

I've pushed this to master and release-18.03

Yes, let's continue discussion in a new ticket. Thanks!

@bendlas bendlas closed this Mar 28, 2018
@Ma27 Ma27 deleted the revert-x-assertions branch March 28, 2018 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar requested changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Ma27 @nbp @bendlas @vcunat @jtojnar @GrahamcOfBorg