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.

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

redshift: Fix default value of $DISPLAY #17746

Merged
merged 1 commit into from
Aug 17, 2016

Conversation

8573
Copy link
Contributor

@8573 8573 commented Aug 15, 2016

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.

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.
@mention-bot
Copy link

@8573, thanks for your PR! By analyzing the annotation information on this pull request, we identified @anderspapitto, @nckx and @ocharles to be potential reviewers

@anderspapitto
Copy link
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)

@8573
Copy link
Contributor Author

8573 commented Aug 15, 2016

@anderspapitto I don't know, alas. DISPLAY itself seems fine though* —

$ echo $DISPLAY
:0

— it's only NixOS's configuration option services.xserver.display that's set to null.

(* 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>
@garbas garbas merged commit 34435a9 into NixOS:master Aug 17, 2016
@8573
Copy link
Contributor Author

8573 commented Aug 17, 2016

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants