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

thiagokokada · 2021-01-08T01:08:37Z

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.

This is a breaking change since there is no support for some of the older options like extraOptions. Also, the way to pass settings completely changed (but there is some aliases).

Second try of PR #108625, now following RFC 42. Also some parts of it taken from PR #50979.

Things done

nixos/modules/services/x11/redshift.nix

thiagokokada · 2021-01-08T16:16:05Z

Commit 73622d6 add tests so we can make sure this module works.

nixos-discourse · 2021-01-09T00:35:46Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/437

thiagokokada · 2021-01-11T17:39:55Z

/marvin opt-in
/status needs_reviewer

marvin-mk2 · 2021-01-11T17:39:57Z

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

nixos/modules/services/x11/redshift.nix

infinisil · 2021-01-12T20:28:13Z

nixos/modules/services/x11/redshift.nix

+      ${mainSection}.location-provider = lcfg.provider;
+      manual = mkIf isManual {
+        lat = mkIf options.location.latitude.isDefined lcfg.latitude;
+        lon = mkIf options.location.longitude.isDefined lcfg.longitude;


Don't check for them being defined here, because we want an error if they're not defined.

It is perfectly possible to use provider = manual and set dawn-time and dusk-time instead (this is what I do). Otherwise we need a hack like setting latitude and longitude to 0 just to use dawn-time and dusk-time.

Can location provider be set to null 🤔 ? If yes, I think I can remove those mkIfs for latitude/longitude.

Hum... Maybe checking if not dawn-time and dusk-time is set too would be better.

Ah so by selecting manual you have to either use lat/lon OR dawn-time/dusk-time? What happens if all four are set?

What I am thinking is a check like this:

manual = mkIf isManual && !((cfg.settings ? ${mainSection}.dawn-time) && (cfg.settings ? ${mainSection}.dusk-time)) { ... }

It is quite big, but I think would work. WDYT @infinisil 🤔 ?

Well, this doesn't work, infinite recursion.

Ok, going to remove the mkIfs. Can't think of a better solution and setting location = { latitude = 0.0; longitude = 0.0; } does the trick.

I'll take a look at this again later, but this isn't a great solution at the moment. There's a mismatch between redshift's idea of "manual" and the location provider's idea of "manual". I think the location provider module might need some change.

I also don't have a problem of having this merge as is and after those changes in location is done I end up fixing this. Like, it is not ideal to have to set location to use time options but it works.

BTW, I think location should support a location.provider = null. This would work in this case because if provider isn't set we just use full manual mode.

pkgs/applications/misc/redshift/default.nix

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. This is a breaking change since there is no support for some of the older options like `extraConfig`. Also, the way to pass settings completely changed (but there is some aliases).

thiagokokada · 2021-01-15T00:31:02Z

@infinisil Gave it a try in commit e59328b. It is kinda messy but I think it should look ok enough.

thiagokokada · 2021-01-15T00:36:57Z

nixos/modules/services/x11/redshift.nix

+  (map (option: mkRenamedOptionModule ([ "services" "redshift" ] ++ option.old) [ "services" "redshift" "settings" "redshift" option.new ]) [
+      { old = [ "temperature" "day" ];    new = "temp-day"; }
+      { old = [ "temperature" "night" ];  new = "temp-night"; }
+      { old = [ "brightness" "day" ];     new = "brightness-day"; }
+      { old = [ "brightness" "night" ];   new = "brightness-night"; }
+    ]);


Since we are already adding some options, maybe I could simply get those options back too with their old descriptions.

WDYT @infinisil ?

thiagokokada · 2021-01-15T00:45:01Z

nixos/modules/services/x11/redshift.nix

+    latitude = mkOption {
+      type = types.nullOr types.float;
+      default = null;
+      example = "0.0";


I don't know if this is a bug, but setting it as a float was making it not appear in manual.

…ptions This should make it more discoverable for the user what is the available options to needed to make the module work.

marvin-mk2 · 2021-01-18T05:00:05Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR ~~will be~~ should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

thiagokokada · 2021-01-19T14:54:20Z

@infinisil Sorry to bother, but can you re-review this? I think this is now what you requested in comment #108739 (comment).

infinisil · 2021-01-21T23:17:47Z

I don't have a lot of time recently sorry. The redshift module unfortunately is rather complicated and some changes are still needed to make it work nicely. I made some initial ones here, but I'm out of brain power for today, I hope to have time for some clean module writing soon!

thiagokokada · 2021-01-21T23:23:44Z

@infinisil Good to know that this PR will be in good hands 😄 . I kinda of want of it merged ASAP, but since I use the release channels and there is still sometime until 21.05 it probably can wait.

marvin-mk2 · 2021-01-25T00:29:41Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR ~~will be~~ should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

marvin-mk2 · 2021-01-28T02:28:30Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR ~~will be~~ should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

marvin-mk2 · 2021-01-31T04:08:21Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR ~~will be~~ should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

marvin-mk2 · 2021-02-03T07:17:26Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR ~~will be~~ should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

xaverdh · 2021-02-03T11:43:37Z

I tested this with my configuration, looks good so far.

marvin-mk2 · 2021-02-06T13:17:06Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR ~~will be~~ should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

marvin-mk2 · 2021-02-09T17:46:23Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR ~~will be~~ should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

marvin-mk2 · 2021-02-12T21:07:45Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR ~~will be~~ should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

marvin-mk2 · 2021-02-15T21:09:09Z

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR ~~will be~~ should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

stale · 2021-08-21T02:54:48Z

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

thiagokokada · 2021-11-11T22:27:49Z

If there is someone interested in this PR I can rebase it to a newer release, however I will probably remove myself as maintainer since I don't use it anymore (I am using the equivalent from Home-Manager).

thiagokokada · 2022-01-11T19:08:03Z

Closing some old PRs of mine.

ofborg bot added 6.topic: nixos 8.has: module (update) labels Jan 8, 2021

This comment has been minimized.

Sign in to view

thiagokokada mentioned this pull request Jan 8, 2021

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

Closed

10 tasks

This comment has been minimized.

Sign in to view

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jan 8, 2021

thiagokokada commented Jan 8, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

thiagokokada commented Jan 8, 2021

View reviewed changes

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

thiagokokada requested a review from infinisil January 9, 2021 03:27

This comment has been minimized.

Sign in to view

marvin-mk2 bot added the marvin label Jan 11, 2021

marvin-mk2 bot added the needs_reviewer label Jan 11, 2021

marvin-mk2 bot requested a review from bennyandresen January 12, 2021 11:40

marvin-mk2 bot added awaiting_reviewer and removed needs_reviewer labels Jan 12, 2021

infinisil requested changes Jan 12, 2021

View reviewed changes

marvin-mk2 bot added awaiting_changes awaiting_reviewer and removed awaiting_reviewer awaiting_changes labels Jan 12, 2021

marvin-mk2 bot added awaiting_reviewer and removed awaiting_changes labels Jan 14, 2021

thiagokokada commented Jan 15, 2021

View reviewed changes

thiagokokada requested a review from infinisil January 15, 2021 00:38

thiagokokada commented Jan 15, 2021

View reviewed changes

nixos/redshift: add dawnTime/duskTime/latitude/longitude/useGeoclue o…

8e5f670

…ptions This should make it more discoverable for the user what is the available options to needed to make the module work.

thiagokokada mentioned this pull request Jan 28, 2021

redshift/gammastep: use ini file instead of passing parameters nix-community/home-manager#1751

Merged

7 tasks

bennyandresen removed their request for review February 17, 2021 06:59

thiagokokada removed awaiting_reviewer marvin labels Feb 19, 2021

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

thiagokokada closed this Jan 11, 2022

thiagokokada deleted the redshift-module-refactor-rfc42 branch January 11, 2022 19:08

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

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

Conversation

thiagokokada commented Jan 8, 2021 • edited

Motivation for this change

Things done

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

thiagokokada commented Jan 8, 2021 • edited

nixos-discourse commented Jan 9, 2021

This comment has been minimized.

This comment has been minimized.

thiagokokada commented Jan 11, 2021

marvin-mk2 bot commented Jan 11, 2021

Choose a reason for hiding this comment

thiagokokada Jan 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil Jan 12, 2021 • edited

Choose a reason for hiding this comment

thiagokokada Jan 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagokokada Jan 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagokokada commented Jan 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvin-mk2 bot commented Jan 18, 2021

thiagokokada commented Jan 19, 2021

infinisil commented Jan 21, 2021

thiagokokada commented Jan 21, 2021

marvin-mk2 bot commented Jan 25, 2021

marvin-mk2 bot commented Jan 28, 2021

marvin-mk2 bot commented Jan 31, 2021

marvin-mk2 bot commented Feb 3, 2021

xaverdh commented Feb 3, 2021

marvin-mk2 bot commented Feb 6, 2021

marvin-mk2 bot commented Feb 9, 2021

marvin-mk2 bot commented Feb 12, 2021

marvin-mk2 bot commented Feb 15, 2021

stale bot commented Aug 21, 2021

thiagokokada commented Nov 11, 2021

thiagokokada commented Jan 11, 2022

thiagokokada commented Jan 8, 2021 •

edited

thiagokokada commented Jan 8, 2021 •

edited

thiagokokada Jan 12, 2021 •

edited

infinisil Jan 12, 2021 •

edited

thiagokokada Jan 12, 2021 •

edited

thiagokokada Jan 12, 2021 •

edited

thiagokokada commented Jan 15, 2021 •

edited