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

[Andrew Tropin <andrew@trop.in>]

[-- Attachment #1: Type: text/plain, Size: 9754 bytes --]

On 2022-01-28 19:37, Ludovic Courtès wrote:

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

Good, thank you.

>> 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?
>

Only add a package to profile and configuration to home-files.

>>> +  (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.
>

It's really cool. 

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

Maybe I'm wrong, but it's very likely that most of the users will be
checking out upstream documentation anyway during configuration of some
programs and those renamings will bring a lot of confusion and
especially, when the record fields names will be combined with names in
escape hatches.

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

I'll make another reply to the second version of the patch and highlight
this topic a little.

>> 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 treat it the same, I forgot to add that guix home import is just an
example, any generation of configurations or other automated processing
becomes magnitudes harder.  Even including sophisticated "type checks".

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

Type checks are possible with data structure driven approach as well and
in a fact it's much more flexible and powerful, however to be fair it
will require some work to prepare a good framework for that like
https://github.com/plumatic/schema
or
https://github.com/metosin/spec-tools/blob/master/docs/02_data_specs.md
for Clojure.

It's also possible to generate documentation for such specs.

Potentially, such approach is more powerful.

However, the big pros of guix records, that it's already have some
whistles and bells.

>
> [...]
>
>>> +  (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?

Mostly yes.

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

It's only a part of the puzzle, we also need somehow to make this
service to share environment variables with other services, when X
window manager or Wayland compositor is finally started.

>>> +                   (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.

Imagine terminal or almost any other user space program, which doesn't
have a configuration in ~/.config and binary in the profile.  Many of
home services will require both to make underlying programs operate
correctly (also think about setting search paths and similar things).  I
can imagine some exceptions, but better to keep it consistent and do it
for all home services not to confuse people.

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

It can be used for debugging, for man pages or when redshift don't use
shepherd service and started in different way (by wm for example).

>> 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?
>

I think for home services it's ok to always add a package to profile.

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

I saw how well-written, but macros-based solutions in Clojure ecosystem
slowly died and substituted with data-based.  I understand that Guile
ecosystem has a slightly weaker toolkit for processing datastructures,
but by the end of the day I think we will be here sooner or later.
Using macros instead of datastructures feels for me like remaking the
same mistake again knowing the consequences.  Maybe I'm wrong.

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]