@infinisil I have seen you on other reviews for redshift. Can you talke a look or do I need somebody else to look?

@yegortimoshenko @globin it looks like you are listed as maintainers for redshift. Is this something that you could review? Thanks!

A better way of doing this I think, would simply to have an option that can but set to something like this: "${pkgs.redshift}/bin/redshift"

@rissson is your suggestion to make the executable parameter default to ${cfg.package}/bin/redshift and then use that in the ExecStart?

Do you know of any other examples of something similar to this? (I'm new to this, so I don't really know where to look.)

Yes, that's what I meant. I wasn't able to find another example of this, I'll leave a review as an example of how it could be done.

I guess it does not have to be so generic. I can change over to having a useGtk option maybe.

Actually, what if we did it like the shells: Have pkgs.redshift.binaryPath be "/bin/redshift" by setting that in a passthru. Then this module can just use ${cfg.package}${cfg.package.binaryPath or "/bin/redshift"} as the executable, and we won't need an additional option, meaning this is a one-line change. The user could then make it use the gtk version with:

{
  package = pkgs.redshift // { binaryPath = "/bin/redshift-gtk"; };
}

How the shells do this:

@infinisil I like how your suggestion is generic and scales. Surely redshift won't be the last time we see this issue.

@infinisil I like that idea, but I'm a bit confused as to why an option is not required? Also, do I have to add that passthru set somewhere?

As someone who currently uses a bit of a hack to accomplish this, I'm excited to see this get merged! I was wondering what the benefits were to the passthru solution vs having the executable option. It seems like the passthru option suffers from being non-discoverable since it would never show up when searching through available options on the Redshift module. Would the lack of discoverability be a big benefit for just using the executable option?

@sumnerevans This is all that's required for that to work:

diff --git a/nixos/modules/services/x11/redshift.nix b/nixos/modules/services/x11/redshift.nix
index 21b0b33553a..77a5eb3b32b 100644
--- a/nixos/modules/services/x11/redshift.nix
+++ b/nixos/modules/services/x11/redshift.nix
@@ -114,7 +114,7 @@ in {
       partOf = [ "graphical-session.target" ];
       serviceConfig = {
         ExecStart = ''
-          ${cfg.package}/bin/redshift \
+          ${cfg.package}${cfg.package.binaryPath or "/bin/redshift"} \
             -l ${providerString} \
             -t ${toString cfg.temperature.day}:${toString cfg.temperature.night} \
             -b ${toString cfg.brightness.day}:${toString cfg.brightness.night} \

With this you could set this in your configuration.nix:

{ pkgs, ... }: {
  services.redshift.package = pkgs.redshift // { binaryPath = "/bin/redshift-gtk"; };
}

@jhgarner Yeah discoverability won't be great with this. So I think the current approach might be better after all.

In addition, what we need here is different than the shell packages I talked about above: The shell packages have one canonical binary path, meaning users shouldn't ever need to change that binary path. However in redshift's case, there are multiple binaries, and users are expected to switch between them. This is another reason for making this a separate option.

I'll just go ahead and merge this now, I think there are good enough reasons for doing so.