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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

thiagokokada
Copy link
Contributor

@thiagokokada thiagokokada commented Jan 7, 2021

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@thiagokokada
Copy link
Contributor Author

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
Copy link
Contributor Author

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

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

nixos/modules/services/x11/redshift.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/redshift.nix Outdated Show resolved Hide resolved
Comment on lines +59 to +63
type = types.float;
default = 1.0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 161 to 164
sectionString = if strings.hasInfix "gammastep" cfg.executable
# https://gitlab.com/chinstrap/gammastep/-/commit/1608ed61154cc652b087e85c4ce6125643e76e2f
then "[general]"
else "[redshift]";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

thiagokokada commented Jan 7, 2021

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?

type = types.listOf types.str;
default = [];
example = [ "-v" "-m randr" ];
extraConfig = mkOption {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

RestartSec = 3;
Restart = "always";
Restart = "on-failure";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

eadwu commented Jan 7, 2021

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
Copy link
Contributor Author

thiagokokada commented Jan 7, 2021

@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).

Copy link
Member

@eadwu eadwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

thiagokokada commented Jan 7, 2021

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
Copy link
Contributor

xaverdh commented Jan 7, 2021

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
Copy link
Contributor Author

thiagokokada commented Jan 7, 2021

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
Copy link
Member

aanderse commented Jan 7, 2021

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.

thiagokokada added a commit to thiagokokada/dotfiles that referenced this pull request Jan 7, 2021
I think this will probably be my last bump, since I will probably
rewrite this module in the future:
NixOS/nixpkgs#108625 (comment)
@thiagokokada
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@aanderse
Copy link
Member

aanderse commented Jan 7, 2021

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
Copy link
Contributor Author

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
Copy link
Member

aanderse commented Jan 7, 2021

Please never mistake my opinions for a "must" 😆

@thiagokokada
Copy link
Contributor Author

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
Copy link
Contributor Author

Substitute PR: #108739.

@thiagokokada thiagokokada deleted the redshift-module-refactor branch December 8, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants