redshift: Fix default value of $DISPLAY#17746
Merged
garbas merged 1 commit intoNixOS:masterfrom Aug 17, 2016
Merged
Conversation
Before commit 54fa0cf, the `redshift` service was run with the environment variable `DISPLAY` set to `:0`. Commit 54fa0cf changed this to instead use the value of the `services.xserver.display` configuration option in the value of the `DISPLAY` variable. In so doing, no default value was provided for the case where `services.xserver.display` is `null`. While the default value of `services.xserver.display` is `0`, use of which by the `redshift` module would result in `DISPLAY` again being set to `:0`, `services.xserver.display` may also be `null`, to which value it is set by, e.g., the `lightdm` module. In the case that `services.xserver.display` is `null`, with the change made in commit 54fa0cf, the `DISPLAY` variable in the environment of the `redshift` service would be set to `:` (a single colon), which, according to my personal experience, would result in — - the `redshift` service failing to start; and - systemd repeatedly attempting to restart the `redshift` service, looping indefinitely, while the hapless `redshift` spews error messages into the journal. It can be observed that the malformed value of `DISPLAY` is likely at fault for this issue by executing the following commands in an ordinary shell, with a suitable `redshift` executable, and the X11 display not already tinted: - `redshift -O 2500` — This command should reduce the color temperature of the display (making it more reddish). - `DISPLAY=':' redshift -O 6500` — This command should raise the color temperature back up, were it not for the `DISPLAY` environment variable being set to `:` for it, which should cause it to, instead, fail with several error messages. This commit attempts to fix this issue by having the `DISPLAY` environment variable for the `redshift` service default to its old value of `:0` in the case that `services.xserver.display` is `null`. I have tested this solution on NixOS, albeit without the benefit of a system with multiple displays.
|
@8573, thanks for your PR! By analyzing the annotation information on this pull request, we identified @anderspapitto, @nckx and @ocharles to be potential reviewers |
Contributor
|
hmm. any idea why lightdm sets DISPLAY to null? there's also a number of other services that want to know what the display is - it would be good to have a fix that addresses all of them (e.g. by making lightdm not mess with DISPLAY) |
Contributor
Author
|
@anderspapitto I don't know, alas. — it's only NixOS's configuration option (* Edit: In normal user environments, not in the environment of the Redshift service, that is.) |
8573
added a commit
to 8573/nixos-config
that referenced
this pull request
Aug 16, 2016
Before NixOS/nixpkgs#54fa0cfe4eef7e54e23380704af70ee7b65473ce, the
`redshift` service was run with the environment variable `DISPLAY` set
to `:0`.
NixOS/nixpkgs#54fa0cfe4eef7e54e23380704af70ee7b65473ce changed this to
instead use the value of the `services.xserver.display` configuration
option in the value of the `DISPLAY` variable. In so doing, no default
value was provided for the case where `services.xserver.display` is
`null`.
While the default value of `services.xserver.display` is `0`, use of
which by the `redshift` module would result in `DISPLAY` again being
set to `:0`, `services.xserver.display` may also be `null`, to which
value it is set by, e.g., the `lightdm` module.
In the case that `services.xserver.display` is `null`, with the change
made in NixOS/nixpkgs#54fa0cfe4eef7e54e23380704af70ee7b65473ce, the
`DISPLAY` variable in the environment of the `redshift` service would
be set to `:` (a single colon), which, according to my personal
experience, would result in —
- the `redshift` service failing to start; and
- systemd repeatedly attempting to restart the `redshift` service,
looping indefinitely, while the hapless `redshift` spews error
messages into the journal.
It can be observed that the malformed value of `DISPLAY` is likely at
fault for this issue by executing the following commands in an
ordinary shell, with a suitable `redshift` executable, and the X11
display not already tinted:
- `redshift -O 2500` — This command should reduce the color
temperature of the display (making it more reddish).
- `DISPLAY=':' redshift -O 6500` — This command should raise the
color temperature back up, were it not for the `DISPLAY`
environment variable being set to `:` for it, which should cause
it to, instead, fail with several error messages.
I have written a patch that attempts to fix this issue by having the
`DISPLAY` environment variable for the `redshift` service default to
its old value of `:0` in the case that `services.xserver.display` is
`null`.
I have filed a GitHub pull request [1] to NixOS/nixpkgs, requesting
that my patch be pulled; pending the resolution of said request, I am
working around this issue by forcibly setting the NixOS configuration
option `systemd.user.services.redshift.environment.DISPLAY`, which
controls the value of the environment variable `DISPLAY` in the
environment of the Redshift service, to `:0`.
[1]: <NixOS/nixpkgs#17746>
Contributor
Author
|
Thanks for merging! |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before commit 54fa0cf, the
redshiftservice was run with the environment variable
DISPLAYset to:0.Commit 54fa0cf changed this to
instead use the value of the
services.xserver.displayconfigurationoption in the value of the
DISPLAYvariable. In so doing, no defaultvalue was provided for the case where
services.xserver.displayisnull.While the default value of
services.xserver.displayis0, use ofwhich by the
redshiftmodule would result inDISPLAYagain beingset to
:0,services.xserver.displaymay also benull, to whichvalue it is set by, e.g., the
lightdmmodule.In the case that
services.xserver.displayisnull, with the changemade in commit 54fa0cf, the
DISPLAYvariable in the environment of the
redshiftservice would be set to:(a single colon), which, according to my personal experience,would result in —
redshiftservice failing to start; andredshiftservice,looping indefinitely, while the hapless
redshiftspews errormessages into the journal.
It can be observed that the malformed value of
DISPLAYis likely atfault for this issue by executing the following commands in an
ordinary shell, with a suitable
redshiftexecutable, and the X11display not already tinted:
redshift -O 2500— This command should reduce the colortemperature of the display (making it more reddish).
DISPLAY=':' redshift -O 6500— This command should raise thecolor temperature back up, were it not for the
DISPLAYenvironment variable being set to
:for it, which should causeit to, instead, fail with several error messages.
This commit attempts to fix this issue by having the
DISPLAYenvironment variable for the
redshiftservice default to its oldvalue of
:0in the case thatservices.xserver.displayisnull.I have tested this solution on NixOS, albeit without the benefit of a
system with multiple displays.