nixos/redshift: rework to use config file #50979

hyperfekt · 2018-11-24T05:25:22Z

Motivation for this change

Redshift has some more options that are not reachable via the CLI (for example dawn-time and dusk-time) - this inspired me to rework the module to allow configuring it via the config file directly, automatically allowing users to take advantage of any options redshift might offer with a convenient (although less directly documented) interface while decreasing the burden of maintaining the module.

This is more of a proposal and not yet tested at all, I'd like to gather some input if this model is something that has a chance of getting merged at all, if there are any big improvements that could be made to it, any big problems I've overlooked etc.
Specifically I'd be interested in adding back some level of in-line documentation, implementing backwards-compatibility if wished and the most elegant way to check if an attribute is set (for the asserts) - that last one might interact with the first one; I've just put a placeholder for now, checking for null would obviously throw errors.

Things done

infinisil

While I like the config option in general, I'm not sure if it's fit for redshift, because there's not too many things you can configure anyways, and I can't find any docs on what options are supported in general (link me to it if there is). This combination makes it not very attractive to use such an unchecked, undocumented config option. Let me know if you have a good reason for it.

I introduced the same thing for #45470, but there ZNC has a lot of options, which are all documented well, and there are too many for NixOS, and they change a lot in addition, which wouldn't be very maintainable.

nixos/modules/services/x11/redshift.nix

infinisil · 2018-11-25T16:11:18Z

nixos/modules/services/x11/redshift.nix

-          between <literal>0.1</literal> and <literal>1.0</literal>.
-        '';
-      };
-      night = mkOption {


You would have to introduce backwards compatible aliases for these removed options with mkAliasOptionModule

I've chosen mkRenamedOptionModule in order to get people to migrate to the new configuration method in case the config file options change in a way that doesn't allow an alias in the future.

infinisil · 2018-11-25T16:11:38Z

nixos/modules/services/x11/redshift.nix

-        Your current latitude, between
-        <literal>-90.0</literal> and <literal>90.0</literal>. Must be provided
-        along with longitude.
+        Configuration to give to redshift.


The description should be a lot longer, explain what this option does and how it can be used.

Referring to man page now, I hope that's sufficient.

infinisil · 2018-11-25T16:22:35Z

Ah and I just read the description again, you mention the problems with unset attributes and that you didn't test it yet, disregard my comments on that. You can check whether a nested attribute exists with:

foo.bar ? baz.qux

And this also works even when foo.bar doesn't have a baz:

foo.bar.baz.qux or "fallback value"

hyperfekt · 2018-11-25T17:10:50Z

EDIT: Ignore this for now, I think I can use your approach in the ZNC module. Thanks for the pointer!

I can't find any docs on what options are supported in general

You're correct, unfortunately redshift does not provide any documentation for its config file. What do you think about leaving the existing options in place, but exposing the config file as an option (and realizing the existing ones through that in order to allow composition for any settings defined therein?

I'm not sure how to recreate the existing options with documentation though, since I can't set the configuration file options in the config part of the module, nor can I attach documentation with mkAliasOptionModule.

There would either need to be a way to further detail some of the attributes in the configuration file option, or we could manually merge a configuration generated by the other options and the configuration file option.

If neither of those is a good option, we could leave them implemented as command line arguments and leave redshift to handle any collisions, but that seems highly suboptimal. I think I'll attempt the second method for now unless you have good idea on getting the first one to work.

infinisil · 2018-11-25T19:56:44Z

One of the main motivations for the config option for ZNC was that there isn't the need anymore to maintain docs in NixOS. Docs should be part of the package and if redshift doesn't have that then it should be fixed on the redshift side imo.

hyperfekt · 2018-11-27T18:59:06Z

It turns out that redshift, contrary to several statements on the web, actually has a complete documentation for the config file: It can be found on the man page.
As a result, I've referred to that as documentation for the config option and gone ahead with the change without providing any further documentation for individual options.
If you'd still like me to reduce the abstractions in the assertions, I can do that.

xaverdh · 2019-03-24T00:03:32Z

Just noticed this pull request.
You may be interested in #57975.

infinisil · 2019-03-25T20:36:18Z

Btw I've since opened an RFC for such config options: NixOS/rfcs#42

hyperfekt · 2019-03-30T22:51:27Z

Wow, I only just noticed I never removed the [WIP] tag. -_-

hyperfekt · 2019-03-31T00:08:42Z

This is ultimately up to @infinisil (and the RFC, I guess); but the way @xaverdh did it in #57975 a collision between a module-provided option and a config attribute set option will have the latter silently overwrite the former instead of failing like I think it should.
I've integrated the earlier target for the drm method here, I wasn't sure how to test that but I assume @xaverdh did so.

infinisil · 2019-08-12T02:58:12Z

#64309 is very relevant for this and almost in shape to be merged, this PR will need some adjustments after that. Pinging @eadwu as well

hyperfekt · 2019-09-30T22:07:29Z

Adjusted accordingly. Thanks for the help.

also uses an earlier target for the drm adjustment method, and allows using dawn / dusk time even if a location provider is set

infinisil · 2019-10-01T13:06:57Z

nixos/modules/services/x11/redshift.nix

+      apply = c:
+        if !(c ? redshift.dawn-time || c ? redshift.dusk-time) && !(c ? redshift.location-provider) && options.locations.provider.isDefined then
+          c // {
+            redshift.location-provider = lcfg.provider;


This clears all other redshift.* keys, you need to use recursiveUpdate instead

And location.provider is always defined because it has a default, also c ? dawn-time == c ? dusk-time as per assertion, so this can be

c: if ! c ? redshift.dawn-time && ! c ? redshift.location-provider then recursiveUpdate c { redshift.location-provider = lcfg.provider; } else c

infinisil · 2019-10-01T13:34:18Z

nixos/modules/services/x11/redshift.nix

+    } ];
+
+    services.redshift.settings.manual = {
+      lat = mkIf options.location.latitude.isDefined lcfg.latitude;


I think this should be something like

let needsLocationProvider = ! cfg.settings ? redshift.dawn-time; in mkIf (needsLocationProvider && lcfg.provider == "manual") { lat = lcfg.latitude; lon = lcfg.longitude; }

Setting lcfg == "manual" should force the user to set longitude and latitude if it's needed. isDefined doesn't do that.

infinisil · 2019-10-01T13:45:39Z

nixos/modules/services/x11/redshift.nix

+
+    assertions = mkIf (cfg.configFile == generatedConfig) [ {
+        assertion = (cfg.settings ? redshift.dawn-time) == (cfg.settings ? redshift.dusk-time);
+        message = "Time of dawn and time of dusk must be provided together.";


The assertion message should also mention that this is about redshift

infinisil · 2019-10-01T13:45:53Z

nixos/modules/services/x11/redshift.nix

+    services.redshift.configFile = mkDefault generatedConfig;
+
+    assertions = mkIf (cfg.configFile == generatedConfig) [ {
+        assertion = (cfg.settings ? redshift.dawn-time) == (cfg.settings ? redshift.dusk-time);


No parens needed

infinisil · 2019-10-01T13:49:56Z

nixos/modules/services/x11/redshift.nix

-        '';
-      };
+    settings = mkOption {
+      type = with types; attrsOf (attrsOf (nullOr (oneOf [ str float bool int ])));


toINI doesn't seem to support floats yet. And you should either remove nullOr or make sure null results in nothing being generated.

infinisil · 2019-10-01T13:50:59Z

nixos/modules/services/x11/redshift.nix

+      default = {};
+      description = ''
+        The configuration to pass to redshift.
+        See <command>man redshift</command> under the section


This is the nasty syntax for man pages: <citerefentry><refentrytitle>redshift</refentrytitle><manvolnum>1</manvolnum></citerefentry>

Valodim · 2020-03-16T09:26:21Z

nixos/modules/services/x11/redshift.nix

+        <option>settings</option> option instead.
+        </para>
+        <para>
+        Setting this option will override any configuration file auto-generated


Shouldn't this be disallowed by assertion? Allowing a settings object that's going to have no effect since it's overridden adds potential for confusion but little value, doesn't it?

xaverdh · 2020-08-22T14:31:15Z

friendly ping (is somebody still working on this?)

hyperfekt · 2020-08-22T15:08:01Z

Thanks for the ping, I was waiting on RFC 42 but the code seems to have been merged a week ago! Will get it into shape when I find the time.

infinisil · 2020-08-22T15:10:18Z

Note that the two relevant PR's are #75584 and #82743 :)

xaverdh · 2021-01-07T11:52:31Z

@hyperfekt are you still working on this?
(people started implementing this again in #108625)

stale · 2021-07-21T00:12:08Z

I marked this as stale due to inactivity. → More info

Lassulus · 2022-04-11T14:02:11Z

seems like the other PRs were abandoned. Do you want to continue this?

hyperfekt requested a review from infinisil as a code owner November 24, 2018 05:25

infinisil self-assigned this Nov 24, 2018

infinisil requested changes Nov 25, 2018

View reviewed changes

hyperfekt force-pushed the redshift_options branch from 28211f1 to 258168a Compare November 27, 2018 18:48

hyperfekt force-pushed the redshift_options branch 2 times, most recently from 2d2b176 to aab93af Compare November 27, 2018 19:42

ryantm added the 2.status: work-in-progress This PR isn't done label Feb 24, 2019

hyperfekt changed the title ~~[WIP] nixos/redshift: rework to use config file~~ nixos/redshift: rework to use config file Mar 30, 2019

infinisil removed the 2.status: work-in-progress This PR isn't done label Mar 30, 2019

hyperfekt force-pushed the redshift_options branch from aab93af to f4287cd Compare March 31, 2019 00:08

hyperfekt force-pushed the redshift_options branch from f4287cd to d33edb8 Compare September 30, 2019 22:04

hyperfekt force-pushed the redshift_options branch from d33edb8 to 7dba54c Compare September 30, 2019 22:17

ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1-10 labels Sep 30, 2019

hyperfekt force-pushed the redshift_options branch from 7dba54c to 5fff70e Compare October 1, 2019 00:28

nixos/redshift: rework to use config files

3eb1de3

also uses an earlier target for the drm adjustment method, and allows using dawn / dusk time even if a location provider is set

hyperfekt force-pushed the redshift_options branch from 5fff70e to 3eb1de3 Compare October 1, 2019 00:35

hyperfekt requested a review from infinisil October 1, 2019 00:37

infinisil requested changes Oct 1, 2019

View reviewed changes

infinisil mentioned this pull request Oct 2, 2019

Redshift add early startup option, rewrite to use toINI and add more options #57975

Closed

Valodim reviewed Mar 16, 2020

View reviewed changes

xaverdh mentioned this pull request Jan 7, 2021

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

Closed

10 tasks

thiagokokada mentioned this pull request Jan 8, 2021

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

Closed

10 tasks

SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 18, 2021

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 11, 2022

Artturin closed this May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/redshift: rework to use config file #50979

nixos/redshift: rework to use config file #50979

hyperfekt commented Nov 24, 2018 •

edited

Loading

infinisil left a comment

infinisil Nov 25, 2018

hyperfekt Nov 27, 2018 •

edited

Loading

infinisil Nov 25, 2018

hyperfekt Nov 27, 2018

infinisil commented Nov 25, 2018 •

edited

Loading

hyperfekt commented Nov 25, 2018 •

edited

Loading

infinisil commented Nov 25, 2018

hyperfekt commented Nov 27, 2018 •

edited

Loading

xaverdh commented Mar 24, 2019 •

edited

Loading

infinisil commented Mar 25, 2019

hyperfekt commented Mar 30, 2019

hyperfekt commented Mar 31, 2019 •

edited

Loading

infinisil commented Aug 12, 2019

hyperfekt commented Sep 30, 2019 •

edited

Loading

infinisil Oct 1, 2019

infinisil Oct 1, 2019

infinisil Oct 1, 2019

infinisil Oct 1, 2019

infinisil Oct 1, 2019

infinisil Oct 1, 2019

infinisil Oct 1, 2019

Valodim Mar 16, 2020

xaverdh commented Aug 22, 2020

hyperfekt commented Aug 22, 2020

infinisil commented Aug 22, 2020

xaverdh commented Jan 7, 2021

stale bot commented Jul 21, 2021

Lassulus commented Apr 11, 2022

nixos/redshift: rework to use config file #50979

nixos/redshift: rework to use config file #50979

Conversation

hyperfekt commented Nov 24, 2018 • edited Loading

Motivation for this change

Things done

infinisil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperfekt Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Nov 25, 2018 • edited Loading

hyperfekt commented Nov 25, 2018 • edited Loading

infinisil commented Nov 25, 2018

hyperfekt commented Nov 27, 2018 • edited Loading

xaverdh commented Mar 24, 2019 • edited Loading

infinisil commented Mar 25, 2019

hyperfekt commented Mar 30, 2019

hyperfekt commented Mar 31, 2019 • edited Loading

infinisil commented Aug 12, 2019

hyperfekt commented Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xaverdh commented Aug 22, 2020

hyperfekt commented Aug 22, 2020

infinisil commented Aug 22, 2020

xaverdh commented Jan 7, 2021

stale bot commented Jul 21, 2021

Lassulus commented Apr 11, 2022

hyperfekt commented Nov 24, 2018 •

edited

Loading

hyperfekt Nov 27, 2018 •

edited

Loading

infinisil commented Nov 25, 2018 •

edited

Loading

hyperfekt commented Nov 25, 2018 •

edited

Loading

hyperfekt commented Nov 27, 2018 •

edited

Loading

xaverdh commented Mar 24, 2019 •

edited

Loading

hyperfekt commented Mar 31, 2019 •

edited

Loading

hyperfekt commented Sep 30, 2019 •

edited

Loading