[bug#53466] [PATCH] home: Add redshift service.

["Ludovic Courtès" <ludo@gnu.org>]

Hi Andrew,

Andrew Tropin <andrew@trop.in> skribis:

> On 2022-01-23 12:11, Ludovic Courtès wrote:
>
>> * gnu/home/services/desktop.scm: New file.
>> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
>> * doc/guix.texi (Desktop Home Services): New node.

[...]

>> +(define-configuration home-redshift-configuration
>
> (redshift
>  (package redshift))
>
> would be useful, especially for people who would like to use a patched
> redshift supporting wayland or some other extended version of a package.

Oops, indeed; I’ll add it.

> Another good candidate for a separate field is shepherd? or
> shepherd-service? to make it possible to remove an integration with
> shepherd if it's not needed.

What would the service do when set to #f?

>> +  (latitude
>> +   (maybe-inexact-number 'disabled)
>> +   "Latitude, when @code{location-provider} is @code{'manual}.")
>> +  (longitude
>> +   (maybe-inexact-number 'disabled)
>> +   "Longitude, when @code{location-provider} is @code{'manual}."))
>
> While I like the naming of the fields more than original option names I
> still not a big fan of this approach for various reasons:

For the record, the whole project avoids abbreviations.  I think it’s an
important part of making things intelligible, especially to non-native
speakers.

> 1. Users of this home service would need to deal with one more level of
> abstraction and keep in mind latitude -> manual.lat,
> nighttime-brightness -> redshift.brightness-night, etc mappings.  Maybe
> for completely new users it's not even necessary to think about
> internals, but for person reading man pages or non-guix specific
> articles it would be a headache.  It's not that bad for very simple
> programs like redshift, but becomes much more significant for software
> supporting much more options like sway or git.

Yes, that’s the usual tradeoff.  The choice made so far in Guix has been
to choose clarity over faithfulness to upstream’s name choices.

> 2. With the current configuration implementation 8 options are missing
> and not possible to set, also it's not possible to reuse already
> existing configuration.

Oops yes, I’ll add an escape hatch.

> Escape hatch with extra-content field solves this problem only
> partially and extra-content can NOT be combined with the rest of
> fields representing redshift options.  So it should be a named a
> "file", not extra-content, and when used will remove the effect of all
> other fields.  extra-options is possible but will blow the mind, we
> have mappings and all that stuff, also need a custom serialization
> logic to merge sections.

I’ll look into it, and I think that’ll help me understand why this
file-like vs. string is so important to you.

> 3. We copy the documentation and part of implementation for software we
> are wrapping and now we have to maintain it ourselves.  Probably,
> redshift is quite stable and both documentation and options doesn't
> change frequently, but for the bigger projects it will lead to outdated
> docs and missing options quite fast or will put a huge maintanance
> burden.

Yes.  Again, that’s the choice we made in Guix: providing bindings for
config file formats.  It’s ambitious, but it’s worked well so far.  If
it worked for the Dovecot, surely it won’t be a problem here.  :-)

> 4. If we decide one day to continue development of guix home import
> command it would be a little nightmare to write importers from existing
> configuration to guix services configurations.

I view ‘guix home import’ as a helper, like ‘guix import’.  I don’t
think it would make sense to have it automatically handle all the config
files that could possibly be handled by Guix Home services.

> I would prefer to have one config field for all the fields above:
>
> (redshift-conf
>  '((redshift
>     ((temp-day . 5700)
>      (temp-night . 3600)
>      (gamma . 0.8)
>      ;; any other number of option some one would like to set
>      #~"dawn-time=6:00\ndusk-time=18:00"
>      ;; or a nasty slurp-file-gexp, which reads the existing
>      ;; configuration or part of it if we migrate step by step
>      (adjustment-method . randr)
>      (location-provider . manual)))
>    (manual
>     ((lat . 55.7)
>      (lon . 12.6)))))

I can see the appeal of alists, but the choice made in Guix is to use
records for configuration; that has advantages, such as type checking,
detection of incorrect field names, and the ability to use all the bells
and whistles of (guix records).

[...]

>> +  (list (shepherd-service
>> +         (documentation "Redshift program.")
>> +         (provision '(redshift))
>> +         (start #~(make-forkexec-constructor
>
> There is a possibility that shepherd is launched before X or wayland
> session started and redshift won't be able to access necessary
> environment variables.  I have a few hacky solutions for other
> applications, but need to come up with a better and more generic way to
> handle it.

Oh, I see.  I’ll add a FIXME.  In practice, that problem would manifest
only if someone logs in at the console first, right?

Perhaps we could define a pseudo ‘xserver-xorg’ Shepherd service that
would be down when ‘DISPLAY’ is undefined, or something like that?

>> +                   (list #$(file-append redshift "/bin/redshift")
>> +                         "-c" #$config-file)))
>> +         (stop #~(make-kill-destructor)))))
>> +
>> +(define home-redshift-service-type
>> +  (service-type
>> +   (name 'home-redshift)
>> +   (extensions (list (service-extension home-shepherd-service-type
>> +                                        redshift-shepherd-service)))
>
> It would be good to extend home-files-service-type with config-file
> generated above and home-profile-service-type with the value of
> `redshift` field.

Regarding the former, that’s not something we usually do for system
services.

As for the latter, I thought about it but I’m not sure what it would be
used for.  WDYT?

> This way user will be able to launch redshift himself or using other
> mechanism like wm startup file, it maybe necessary for
> testing/debugging purpose or to be sure that redshift has DISPLAY or
> other variables available or maybe some other use cases.

In that case it may be best to let users explicitly install it in their
profile maybe?

> Yes, it's possible to use the approach proposed by this patch for
> implementing configuration for such simple program, but I still have
> a lot of concerns about applying it to more complex software.

I understand your concerns, but I think they’re beyond the scope of this
review.  I also think that there’s ample experience with system services
showing that writing “nice” configuration bindings actually works in
practice.

Thanks,
Ludo’.