nixos/redshift: use config file instead of passing parameters #108625

thiagokokada · 2021-01-07T00:10:38Z

Motivation for this change

Not every option is exposed by redshift/gammastep parameters, for example gamma options are only exposed in configuration file. So this PR refactors this module to generate a configuration file and pass it to the redshift/gammastep using -c parameter.

Also, it implements fade/gamma options not available in the previous version of this module (since it was not exposed by parameters).

This is a slightly breaking change, since extraOptions was renamed to extraConfig and now accept a attribute set instead of a list of string

Things done

thiagokokada · 2021-01-07T00:17:29Z

With the following configuration:

let
    redshiftModuleRefactor = fetchTarball {
    url = "https://github.com/thiagokokada/nixpkgs/archive/1bea263ca0937e4d7dd08b85545a2426223c7a79.tar.gz";
    sha256 = "0gawqndyj2n23mhsp93rlacrfb8x86rr6hd7d9waknh6fpqbnixr";
  };
in{
  # Backport module from unstable.
  imports = [
    "${redshiftModuleRefactor}/nixos/modules/services/x11/redshift.nix"
  ];

  disabledModules = [
    "services/x11/redshift.nix"
  ];

  location = {
    latitude = -23.5;
    longitude = -46.6;
  };

  services = {
    redshift = {
      enable = true;
      temperature = {
        day = 5500;
        night = 3700;
      };
      gamma = {
        day = 1.0;
        night = 0.8;
      };
      package = pkgs.unstable.gammastep;
      executable = "/bin/gammastep-indicator";
    };
  };
}

I had the following result:

~
❯ systemctl cat --user redshift.service     
# /nix/store/8bxr1gi8y9qa8ya4qya88vx97mzdm6g3-unit-redshift.service/redshift.service
[Unit]
Description=Redshift colour temperature adjuster
PartOf=graphical-session.target

[Service]
Environment="LOCALE_ARCHIVE=/nix/store/a2px4kdz1jm03f8ifr1pzir0csfmyrlv-glibc-locales-2.31/lib/locale/locale-archive"
Environment="PATH=/nix/store/z1qvlavy35wanw5k54fvvfffws5bvigj-coreutils-8.31/bin:/nix/store/3fvzxz59gacagpwyzpfdiinc1yv46hw1-findutils-4>
Environment="TZDIR=/nix/store/w1g27pgslf28nh1py1szj7lk4xksdhqq-tzdata-2020c/share/zoneinfo"



ExecStart=/nix/store/f38nk3a9qq415zhar429j7fr7czarvcg-gammastep-2.0.6/bin/gammastep-indicator -c /nix/store/xswxcml4r3fdzviyqwd399lpzsia>
Restart=always
RestartSec=3


~ 12s
❯ cat /nix/store/xswxcml4r3fdzviyqwd399lpzsia3v27-redshift.conf 
[general]
temp-day=5500
temp-night=3700
fade=1
location-provider=manual
brightness-day=1.000000
brightness-night=1.000000
gamma-day=1.000000
gamma-night=0.800000



[manual]
lat=-23.500000
lon=-46.600000

For redshift case:

let
    redshiftModuleRefactor = fetchTarball {
    url = "https://github.com/thiagokokada/nixpkgs/archive/1bea263ca0937e4d7dd08b85545a2426223c7a79.tar.gz";
    sha256 = "0gawqndyj2n23mhsp93rlacrfb8x86rr6hd7d9waknh6fpqbnixr";
  };
in{
  # Backport module from unstable.
  imports = [
    "${redshiftModuleRefactor}/nixos/modules/services/x11/redshift.nix"
  ];

  disabledModules = [
    "services/x11/redshift.nix"
  ];

  location.provider = "geoclue2";

  services = {
    redshift = {
      enable = true;
      temperature = {
        day = 5500;
        night = 3700;
      };
      gamma = {
        day = 1.0;
        night = 0.8;
      };
      package = pkgs.unstable.redshift;
      executable = "/bin/redshift-gtk";
    };
  };
}

❯ systemctl cat --user redshift.service
# /nix/store/9xqyys1w2lb6f9wma2zjjfyhr4hsjsc7-unit-redshift.service/redshift.service
[Unit]
Description=Redshift colour temperature adjuster
PartOf=graphical-session.target

[Service]
Environment="LOCALE_ARCHIVE=/nix/store/a2px4kdz1jm03f8ifr1pzir0csfmyrlv-glibc-locales-2.31/lib/locale/locale-archive"
Environment="PATH=/nix/store/z1qvlavy35wanw5k54fvvfffws5bvigj-coreutils-8.31/bin:/nix/store/3fvzxz59gacagpwyzpfdiinc1yv46hw1-findutils-4>
Environment="TZDIR=/nix/store/w1g27pgslf28nh1py1szj7lk4xksdhqq-tzdata-2020c/share/zoneinfo"



ExecStart=/nix/store/kx8r729wmn9nsm0plqiqv6gvkiah8jwy-redshift-1.12/bin/redshift-gtk -c /nix/store/6ffjf4pbb68ffx6xr0hy2dcjg5rkyi3x-reds>
Restart=always
RestartSec=3


~
❯ cat /nix/store/6ffjf4pbb68ffx6xr0hy2dcjg5rkyi3x-redshift.conf 
[redshift]
temp-day=5500
temp-night=3700
fade=1
location-provider=geoclue2
brightness-day=1.000000
brightness-night=1.000000
gamma-day=1.000000
gamma-night=0.800000

thiagokokada · 2021-01-07T00:21:32Z

CC @yegortimoshenko @globin @primeos (redshift/gammastep package maintainers).

Also @sumnerevans @infinisil @michaelpj (last people to touch this module).

nixos/modules/services/x11/redshift.nix

thiagokokada · 2021-01-07T00:26:45Z

nixos/modules/services/x11/redshift.nix

+        type = types.float;
+        default = 1.0;


This is also technically a breaking change. I can document it in release notes if necessary.

thiagokokada · 2021-01-07T00:28:02Z

nixos/modules/services/x11/redshift.nix

+      sectionString = if strings.hasInfix "gammastep" cfg.executable
+        # https://gitlab.com/chinstrap/gammastep/-/commit/1608ed61154cc652b087e85c4ce6125643e76e2f
+        then "[general]"
+        else "[redshift]";


Not really necessary since gammastep still supports [redshift], but this avoids a warning at startup.

Not every option is exposed by redshift/gammastep parameters, for example gamma options are only exposed in configuration file. So this PR refactors this module to generate a configuration file and pass it to the redshift/gammastep using -c parameter. Also, it implements fade/gamma options not available in the previous version of this module (since it was not exposed by parameters). This is a slightly breaking change, since extraOptions was renamed to extraConfig and now accept a attribute set instead of a list of strings.

thiagokokada · 2021-01-07T03:44:39Z

With the changes in 460e276, the following config file:

{
  location.provider = "geoclue2";

  services = {
    redshift = {
      enable = true;
      temperature = {
        day = 5500;
        night = 3700;
      };
      gamma = {
        day = 1.0;
        night = 0.8;
      };
      package = pkgs.unstable.redshift;
      executable = "/bin/redshift-gtk";
      extraConfig = {
        randr = {
          display = 0;
        };
     };
    };
  };
}

[randr]
screen=0

[redshift]
brightness-day=1.000000
brightness-night=1.000000
fade=1
gamma-day=1.000000
gamma-night=0.800000
location-provider=geoclue2
temp-day=5500
temp-night=3700

This worked great! Thanks for the suggestion @sumnerevans.

Can you review again?

thiagokokada · 2021-01-07T03:45:57Z

nixos/modules/services/x11/redshift.nix

-      type = types.listOf types.str;
-      default = [];
-      example = [ "-v" "-m randr" ];
+    extraConfig = mkOption {


Renamed because this is technically not an option anymore, and extraConfig seems to be the more standard name for this.

thiagokokada · 2021-01-07T03:59:02Z

nixos/modules/services/x11/redshift.nix

        RestartSec = 3;
-        Restart = "always";
+        Restart = "on-failure";


This avoids the service to keep restarting in case of an error in config (that may happen for example when setting a wrong extraConfig value).

eadwu · 2021-01-07T04:00:18Z

I don't believe I'm a maintainer for redshift nor do I use redshift locally anymore.

Anyway if you're going to generate a configuration file it might be better to convert it to use a structured attribute set (i.e. settings) and generate the file with contents converted from generators.toINI (or at least it looks pretty INI-like from your snippet).

If you don't know what I'm talking about, you can probably take a reference from https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/x11/picom.nix#L237.

Nevermind you are using it.

thiagokokada · 2021-01-07T04:01:27Z

@eadwu This is exactly what I did in commit 460e276.

(Sorry, just asked your review because this was what GitHub suggested and modules doesn't have maintainers like packages).

eadwu

Looks fine to me. Not quite sure what about the gamma configuration though, it looks like it needs an external dependency (gammastep) to work, though if it doesn't break the configuration parsing it's probably fine (though it will probably fall under unexpected behavior).

Nothing else for me to note except nitpicks which probably don't matter. Doesn't break system evaluation on my end.

You can probably put the entire configuration inside extraConfig but I don't think it would provide any extra benefit unless you are using nixos-option to view the attribute set.

thiagokokada · 2021-01-07T04:34:54Z

Not quite sure what about the gamma configuration though, it looks like it needs an external dependency (gammastep) to work

gamma-day and gamma-night are available in redshift: https://github.com/jonls/redshift/blob/master/redshift.conf.sample#L39-L40.

BTW, gammastep (https://gitlab.com/chinstrap/gammastep/) is a fork of redshift that supports Wayland, not a dependency of redshift. It supports pretty much the same settings. It is the recommended fork for sway (AFAIK) after redshift-wlr was abandoned.

Edit: Looking at gamma, never knew it supported gamma-{day,night}=0.8:0.7:0.8 format (that is, one for each RGB color, I always used the single float version). I will change this later to support with types; (either float str), maybe someone will want to use it that way.

xaverdh · 2021-01-07T11:49:17Z

see #50979 for prior art

Redshift support setting each color channel individually in the following format: "0.8:07:0.8". So support this case too. Also, improve type checking in `extraConfig`.

thiagokokada · 2021-01-07T12:14:23Z

Commit 36156ba adds support for setting gamma-{day,night} = "0.8:0.7:0.8" (actually any string, not going to validate the format) so the user can set the gamma by channel like available here: https://github.com/jonls/redshift/blob/master/redshift.conf.sample#L39-L40. Also improves type checking in extraConfig.

I think this PR is pretty much ready to merge.

see #50979 for prior art

Huh... I think this other proposal is kinda abandoned? Well, if the original author still wants to merge I am okay with it too, but I think this PR is ready to merge and the other one still needs some fixes right?

Anyway, it helped to improve the types of extraConfig, so I am not complaining either.

aanderse · 2021-01-07T12:31:50Z

A few conventions that we use would include calling extraOptions settings, using (pkgs.formats.ini {}).format for settings.type, using (pkgs.formats.ini {}).generate to generate to configuration file, and using mkMerge when appropriate. As mentioned, please see RFC42 for details.

I think this will probably be my last bump, since I will probably rewrite this module in the future: NixOS/nixpkgs#108625 (comment)

thiagokokada · 2021-01-07T13:19:28Z

@aanderse Didn't know we had a RFC for this (it is not merged and I didn't find it in https://github.com/NixOS/rfcs/, had to look on the open PRs). TIL.

I was trying to keep this module was similar as the current module breaking as little as possible, but I think this RFC points is fair. Following it would mean probably a completely different module as the current one, and I think this is kinda what #50979 is.

Well, will follow the #50979 PR for now and if the author doesn't answer in one or two weeks, I can probably try to open another PR following more closely RFC 42.

thiagokokada · 2021-01-07T13:20:24Z

BTW, should I close this PR or is there still interest in merging it?

aanderse · 2021-01-07T13:21:45Z

BTW, should I close this PR or is there still interest in merging it?

Let's wait and see if the other PR picks up or not.

thiagokokada · 2021-01-07T13:23:27Z

Let's wait and see if the other PR picks up or not.

Asking more because I won't adapt this PR to follow RFC 42 since this will probably would be better made in another PR (it will be a full rewrite anyway). So if RFC 42 support is a must I don't see many reasons to keep this open.

aanderse · 2021-01-07T13:25:30Z

Please never mistake my opinions for a "must" 😆

thiagokokada · 2021-01-07T13:30:46Z

Ok, so I will keep this open and if the maintainers of the module thinks this is good enough they can go and approve/merge.

BTW, for me it is in a good enough state (RFC 42 really seems to make things more elegant, but I am using this module in my configuration and it works good enough for me).

thiagokokada · 2021-01-08T01:16:19Z

Substitute PR: #108739.

ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 7, 2021

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jan 7, 2021

thiagokokada commented Jan 7, 2021

View reviewed changes

nixos/modules/services/x11/redshift.nix Outdated Show resolved Hide resolved

nixos/modules/services/x11/redshift.nix Outdated Show resolved Hide resolved

thiagokokada requested a review from eadwu January 7, 2021 00:26

thiagokokada commented Jan 7, 2021

View reviewed changes

eadwu approved these changes Jan 7, 2021

View reviewed changes

xaverdh mentioned this pull request Jan 7, 2021

nixos/redshift: rework to use config file #50979

Closed

10 tasks

nixos/redshift: allow setting gamma.{day,night} as a string

36156ba

Redshift support setting each color channel individually in the following format: "0.8:07:0.8". So support this case too. Also, improve type checking in `extraConfig`.

thiagokokada added a commit to thiagokokada/dotfiles that referenced this pull request Jan 7, 2021

_nixos: Bump redshiftModuleRefactor, again

5ef0f9e

I think this will probably be my last bump, since I will probably rewrite this module in the future: NixOS/nixpkgs#108625 (comment)

thiagokokada mentioned this pull request Jan 8, 2021

nixos/redshift: use config file instead of passing parameters, using RFC42 #108739

Closed

10 tasks

thiagokokada closed this Jan 8, 2021

thiagokokada deleted the redshift-module-refactor branch December 8, 2023 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/redshift: use config file instead of passing parameters #108625

nixos/redshift: use config file instead of passing parameters #108625

thiagokokada commented Jan 7, 2021 •

edited

Loading

thiagokokada commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

thiagokokada Jan 7, 2021

thiagokokada Jan 7, 2021

thiagokokada commented Jan 7, 2021 •

edited

Loading

thiagokokada Jan 7, 2021 •

edited

Loading

thiagokokada Jan 7, 2021

eadwu commented Jan 7, 2021 •

edited

Loading

thiagokokada commented Jan 7, 2021 •

edited

Loading

eadwu left a comment

thiagokokada commented Jan 7, 2021 •

edited

Loading

xaverdh commented Jan 7, 2021

thiagokokada commented Jan 7, 2021 •

edited

Loading

aanderse commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

aanderse commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

aanderse commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

thiagokokada commented Jan 8, 2021

nixos/redshift: use config file instead of passing parameters #108625

nixos/redshift: use config file instead of passing parameters #108625

Conversation

thiagokokada commented Jan 7, 2021 • edited Loading

Motivation for this change

Things done

thiagokokada commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

thiagokokada Jan 7, 2021

Choose a reason for hiding this comment

thiagokokada Jan 7, 2021

Choose a reason for hiding this comment

thiagokokada commented Jan 7, 2021 • edited Loading

thiagokokada Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

thiagokokada Jan 7, 2021

Choose a reason for hiding this comment

eadwu commented Jan 7, 2021 • edited Loading

thiagokokada commented Jan 7, 2021 • edited Loading

eadwu left a comment

Choose a reason for hiding this comment

thiagokokada commented Jan 7, 2021 • edited Loading

xaverdh commented Jan 7, 2021

thiagokokada commented Jan 7, 2021 • edited Loading

aanderse commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

aanderse commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

aanderse commented Jan 7, 2021

thiagokokada commented Jan 7, 2021

thiagokokada commented Jan 8, 2021

thiagokokada commented Jan 7, 2021 •

edited

Loading

thiagokokada commented Jan 7, 2021 •

edited

Loading

thiagokokada Jan 7, 2021 •

edited

Loading

eadwu commented Jan 7, 2021 •

edited

Loading

thiagokokada commented Jan 7, 2021 •

edited

Loading

thiagokokada commented Jan 7, 2021 •

edited

Loading

thiagokokada commented Jan 7, 2021 •

edited

Loading