bug#63082: [PATCH 14/17] services: mpd: Obsolete 'environment-variables' field.

[Bruno Victal]

On 2023-04-29 18:04, Maxim Cournoyer wrote:
> Hi Bruno,
> 
> Bruno Victal <mirai@makinata.eu> writes:
> 
>> On 2023-04-28 15:27, Maxim Cournoyer wrote:
>>> Rationale: Services can be extended via the simple-service mechanism instead
>>> of having to expose fields on service configurations that are not directly
>>> connected to the service's configuration.
>>>
>>> * gnu/services/audio.scm (mpd-environment-variables-sanitizer): New 
>>> sanitizer.
>>> (mpd-configuration): Use it.
>>> (mpd-shepherd-service): Hard code the useful environment variables inside 
>>> the
>>> Shepherd service.
>>> ---
>>
>> This field shouldn't be deprecated as one of it's primary purposes is to 
>> allow for
>> the pulseaudio daemon configuration to be set to another one.
>> What you're doing here is effectively hardcoding the pulseaudio 
>> configuration.
> 
> Our only means to declare a pulseaudio configuration
> (pulseaudio-service-type) places it at this location, so it seems
> reasonable to hard code it.  What use case do you have for a custom
> pulseaudio configuration that pulseaudio-service-type could not cater
> to?  This prevents users defining another environment variable and
> forgetting to replace these, then wondering why the default pulse
> configuration doesn't work, and it felt out of place to me (an
> implementation detail better encapsulated).

Indeed but note that there's a small subtlety to pulseaudio-service-type, 
chiefly that
the service is not your typical ¿monodaemonic? process that is used throughout 
the system,
rather it simply provides you a default config for the pulseaudio daemon.
The fact that multiple pulseaudio daemons can be launched alongside is a strong 
indicator that
perhaps you will want for some of them to use different configurations, which 
is done via the
environment variables.

Right now this would be mainly achieved using local-file, text-file or 
specifying a path
but in theory the procedures for pulseaudio-service-type could be reused for 
serializing
configurations to be used outside of the service.

Regarding the users forgetting the variables, it looks obvious that if you omit 
the default
values there then the behavior will also change accordingly.
If you strongly feel that this is very pitfall prone (IMO it's no worse than 
forgetting to add %base-services
at the end of the services field) then perhaps documenting it would suffice?

> 
>> I'd consider this field to be within the same category as
>> 'shepherd-requirement', it's for flexibility
> 
> I like the idea of more flexibility, but I don't like that these fields
> need to be duplicated for each service, somewhat encumbering the view.
> Perhaps we need to devise some 'always nice to have' set that would be
> configurable for any service without having to expose these fields as
> part of their main configuration?
> 

Right, it's not optimal but these are fields with legitimate uses, instituting 
rigidity here will simply
make the services overly opinionated to some particular kind of setup, which 
drastically reduces their value.

Regarding their duplication, perhaps an improvement for define-record-type* 
would be better?
SRFI-136 seems something that would address these concerns.