Startx: Implement xserverArgs, improve logfile location #80272
Conversation
|
@aszlig does this PR work for your setup which needed your change that I partially reverted? |
Atemu
force-pushed
the
startx-imporvements
branch
2 times, most recently
from
64ee4ec
to
68cacca
Compare
|
Just telling users to set some option if they want to use startx as root was not a very good solution, so I somewhat hackily fixed the log location of xorg-server which understandably requires quite a rebuild and that probably isn't ideal. I have a solution that would not require that rebuild and would work almost as well but isn't as clean; should I go with that instead? |
Previously it'd try to use the one in its output path which is read-only of course
Not sure why it was done this way. Now we can apply patches again
Xorg does this because X would crash if it couldn't write to the logfile. Not a problem on NixOS as journald should create the dir automatically and the logfile location is set to /dev/null for all displayManagers but startx.
It was in ${xorg.xorgserver}/var/log before which isn't writable and that would
crash X.
This required some workarounds for the startx displayManager which are no longer
needed now.
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 (see `man Xorg`). It's in a much better place aswell ($XDG_DATA_HOME)
|
My previous patchset was clumsy, hacky and had some unecessary changes in it. What I just force-pushed should be a lot better. |
Atemu
changed the title
|
I successfully ran the i3-wm test and manually confirmed that sway and i3 work. I did not have the time to build and test every package that depends on xorgserver. |
|
/marvin opt-in |
|
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. |
|
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 |
pkgs/servers/x11/xorg/overrides.nix
Outdated
| prePatch = '' | ||
| sed -i 's|^defaultserverargs="|&-logfile \"$HOME/.xorg.log\"|p' startx.cpp | ||
| ''; |
There was a problem hiding this comment.
I think this should be part of the commit were you patched xorg.
There was a problem hiding this comment.
It's one logical step: Old solution with hard-coded path in xinit -> new solution with upstream path and configurable override in NixOS that was set up in the previous commits.
|
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 |
|
@timokau this PR was reviewed yet @marvin-mk2 still nags it to be reviewed. |
|
@doronbehar I think you left your review as "comment" instead of "request changes", which left the status as "awaiting review". Its debatable if this is correct or not, but at least its not a bug. My intention is to avoid having PRs stuck in "awaiting changes" where they can be forgotten, so I err'ed on the side of "awaiting review". /status awaiting_changes |
marvin-mk2
bot
added
awaiting_changes
awaiting_reviewer
and removed
awaiting_reviewer
awaiting_changes
labels
|
Continued in #100774 |
Motivation for this change
Closes: #80198
The Xorg log was hard-coded to ~/.xorg.log, not ideal.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)