Startx improvements #100774

Atemu · 2020-10-16T23:28:10Z

Motivation for this change

Typo-less and improved version of #80272

Things done

Atemu · 2020-10-16T23:29:15Z

/marvin opt-in
/status needs_reviewer

marvin-mk2 · 2020-10-16T23:29:17Z

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

doronbehar

Diff looks good.

nixos/modules/services/x11/xserver.nix

doronbehar · 2020-10-17T07:59:55Z

The first 2 commits are totally fine I suppose. Regarding the log file's location changing from ~/.xorg.log to /var/log/xorg.log, are we sure that won't cause any issues?

Atemu · 2020-10-31T20:38:20Z

This changes it to the default upstream Xorg logfile location. The logfile was hard-coded by us to a non-standard location because of an issue someone had in 2012 which was likely caused by Nixpkgs patching the log location to be inside the Nix store, which I also fixed.

The upstream default log location for regular users is inside their home dir, /var/log will only be used when manually running X as root.

I really don't think we need a tmpfiles rule for this. Xorg logs are kilobytes and are rotated automatically AFAICT. They're aren't even going to be there in most cases as you need to manually run X as root.

You probably don't even want them to be deleted automatically as you might want to audit them after a catastrophic failure.

If you sudo startx for some strange reason and care about a few kilobytes in /var/log, you should just delete them manually.

marvin-mk2 · 2020-11-03T23:01:28Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

doronbehar

I approve as all changes make sense. But I didn't test so I don't merge.

marvin-mk2 · 2020-11-07T11:02:50Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

marvin-mk2 · 2020-11-10T16:30:08Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

marvin-mk2 · 2020-11-13T16:33:03Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

marvin-mk2 · 2020-11-16T18:45:44Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

marvin-mk2 · 2020-11-19T23:20:06Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

marvin-mk2 · 2020-11-23T04:49:19Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

marvin-mk2 · 2020-11-26T09:52:03Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

marvin-mk2 · 2020-11-29T09:53:12Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

marvin-mk2 · 2020-12-02T11:29:15Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

timokau · 2020-12-02T12:14:42Z

Sorry for the spam. Unfortunately I can't look into this in much detail myself right now. @Atemu to what extent have you tested this?

Atemu · 2020-12-02T13:22:10Z

I sucessfully ran i3 with startx and the NixOS test still works too. I also tested running X as root.

Since this requires a rebuild of all Xorg dependent software, I didn't test much further but I could do that now.

Is there a specific scenario you would like to see tested?

timokau

That's probably sufficient, thank you :) I left some additional feedback.

pkgs/servers/x11/xorg/overrides.nix

nixos/modules/services/x11/xserver.nix

nixos/modules/services/x11/display-managers/startx.nix

marvin-mk2 · 2020-12-08T18:55:49Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

marvin-mk2 · 2020-12-12T15:53:44Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

timokau

Looks good now, please squash your fixups.

Slightly off-topic: Its usually easier if you explicitly re-request a review (or post a comment) when you have addressed all comments. GitHub doesn't send notifications when threads are resolved.

pkgs/servers/x11/xorg/overrides.nix

Previously it'd try to use the one under its output path which is read-only of course

Fixes NixOS#80198

Xorg creates the log-dir in its output path because X crashes if it can't write to its logfile. On a regular distro, this dir would be installed into the root to prevent that from happening but with Nix, it sits in the read-only Nix store. Ironically, when Xorg tries to write here, it fails and crashes. To make Xorg log to /var/log, we have to stop the build script from trying to create the log-dir as the sandbox doesn't (and shouldn't) have access to /var. This creates a runtime dependency on /var when running as root but that should exist on any Linux system (on NixOS, journald always creates /var/log). Previously, the startx displayManager required some workarounds for logfiles which are obsolete now. patchPhase -> postPatch because overriding the patchPhase prevents patches from being applied

It makes sense for it to be /dev/null for all the displayManagers but startx, it needs a different logFile configuration.

This partially reverts bf3d3dd. I don't know why we weren't getting a default logfile back then but Xorg definitely provides one now ($XDG_DATA_HOME for regular users and /var/log for root, see `man Xorg`)

Atemu · 2020-12-13T05:18:17Z

Squashed and rebased. Also changed base to staging since this requires a rebuild of all Xorg-server dependent software for something most people don't use.

Atemu · 2020-12-13T05:53:12Z

EditorConfig fail is unrelated, probably an issue in staging.

timokau

Thank you!

Atemu · 2020-12-14T09:23:07Z

Thank you for the thorough review!

Artturin · 2021-10-20T03:09:45Z

pkgs/servers/x11/xorg/overrides.nix

-    prePatch = ''
-      sed -i 's|^defaultserverargs="|&-logfile \"$HOME/.xorg.log\"|p' startx.cpp
+    postFixup = ''
+      substituteInPlace $out/bin/startx --replace $out/etc/X11/xinit/xserverrc /etc/X11/xinit/xserverrc


Hi, was not replacing
sysclientrc=/nix/store/mxd6541naki8673njjkq0d153vlwsskv-xinit-1.4.1/etc/X11/xinit/xinitrc
intentional?

Yes, xinitrc was out of scope.

Atemu requested a review from doronbehar October 16, 2020 23:28

Atemu mentioned this pull request Oct 16, 2020

Startx: Implement xserverArgs, improve logfile location #80272

Closed

10 tasks

marvin-mk2 bot added marvin needs_reviewer labels Oct 16, 2020

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-linux: 101-500 labels Oct 16, 2020

doronbehar requested changes Oct 17, 2020

View reviewed changes

nixos/modules/services/x11/xserver.nix Show resolved Hide resolved

marvin-mk2 bot added awaiting_changes and removed needs_reviewer labels Oct 17, 2020

marvin-mk2 bot added awaiting_reviewer and removed awaiting_changes labels Oct 31, 2020

doronbehar approved these changes Nov 4, 2020

View reviewed changes

timokau reviewed Dec 4, 2020

View reviewed changes

doronbehar requested review from rycee and Ericson2314 December 5, 2020 09:54

timokau requested changes Dec 12, 2020

View reviewed changes

pkgs/servers/x11/xorg/overrides.nix Show resolved Hide resolved

marvin-mk2 bot added awaiting_changes awaiting_reviewer and removed awaiting_reviewer awaiting_changes labels Dec 12, 2020

Atemu added 5 commits December 13, 2020 06:15

xinit: use the system xserverrc

338939d

Previously it'd try to use the one under its output path which is read-only of course

nixos/startx: provide xserverArgs via xserverrc

c72c02a

Fixes NixOS#80198

nixos/xserver: make logFile configurable

693a31a

It makes sense for it to be /dev/null for all the displayManagers but startx, it needs a different logFile configuration.

nixos/startx: send Xorg log to the default location

d3113a6

This partially reverts bf3d3dd. I don't know why we weren't getting a default logfile back then but Xorg definitely provides one now ($XDG_DATA_HOME for regular users and /var/log for root, see `man Xorg`)

Atemu force-pushed the startx-improvements branch from fe02e52 to d3113a6 Compare December 13, 2020 05:16

Atemu changed the base branch from master to staging December 13, 2020 05:16

timokau approved these changes Dec 13, 2020

View reviewed changes

timokau merged commit d6dba0d into NixOS:staging Dec 13, 2020

Atemu deleted the startx-improvements branch December 14, 2020 09:23

doronbehar mentioned this pull request Mar 16, 2021

xinit: startx upstream script creates a buggy X environment and requires patching #94375

Open

Artturin reviewed Oct 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Startx improvements #100774

Startx improvements #100774

Atemu commented Oct 16, 2020 •

edited

Atemu commented Oct 16, 2020

marvin-mk2 bot commented Oct 16, 2020

doronbehar left a comment

doronbehar commented Oct 17, 2020

Atemu commented Oct 31, 2020

marvin-mk2 bot commented Nov 3, 2020

doronbehar left a comment

marvin-mk2 bot commented Nov 7, 2020

marvin-mk2 bot commented Nov 10, 2020

marvin-mk2 bot commented Nov 13, 2020

marvin-mk2 bot commented Nov 16, 2020

marvin-mk2 bot commented Nov 19, 2020

marvin-mk2 bot commented Nov 23, 2020

marvin-mk2 bot commented Nov 26, 2020

marvin-mk2 bot commented Nov 29, 2020

marvin-mk2 bot commented Dec 2, 2020

timokau commented Dec 2, 2020

Atemu commented Dec 2, 2020

timokau left a comment

marvin-mk2 bot commented Dec 8, 2020

marvin-mk2 bot commented Dec 12, 2020

timokau left a comment

Atemu commented Dec 13, 2020

Atemu commented Dec 13, 2020

timokau left a comment

Atemu commented Dec 14, 2020

Artturin Oct 20, 2021

Atemu Oct 20, 2021

Startx improvements #100774

Startx improvements #100774

Conversation

Atemu commented Oct 16, 2020 • edited

Motivation for this change

Things done

Atemu commented Oct 16, 2020

marvin-mk2 bot commented Oct 16, 2020

doronbehar left a comment

Choose a reason for hiding this comment

doronbehar commented Oct 17, 2020

Atemu commented Oct 31, 2020

marvin-mk2 bot commented Nov 3, 2020

doronbehar left a comment

Choose a reason for hiding this comment

marvin-mk2 bot commented Nov 7, 2020

marvin-mk2 bot commented Nov 10, 2020

marvin-mk2 bot commented Nov 13, 2020

marvin-mk2 bot commented Nov 16, 2020

marvin-mk2 bot commented Nov 19, 2020

marvin-mk2 bot commented Nov 23, 2020

marvin-mk2 bot commented Nov 26, 2020

marvin-mk2 bot commented Nov 29, 2020

marvin-mk2 bot commented Dec 2, 2020

timokau commented Dec 2, 2020

Atemu commented Dec 2, 2020

timokau left a comment

Choose a reason for hiding this comment

marvin-mk2 bot commented Dec 8, 2020

marvin-mk2 bot commented Dec 12, 2020

timokau left a comment

Choose a reason for hiding this comment

Atemu commented Dec 13, 2020

Atemu commented Dec 13, 2020

timokau left a comment

Choose a reason for hiding this comment

Atemu commented Dec 14, 2020

Artturin Oct 20, 2021

Choose a reason for hiding this comment

Atemu Oct 20, 2021

Choose a reason for hiding this comment

Atemu commented Oct 16, 2020 •

edited