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

[Maxim Cournoyer]

Hi Bruno,

Bruno Victal <mirai@makinata.eu> writes:

> Hi Maxim,
>
> On 2023-05-03 02:44, Maxim Cournoyer wrote:
>> Hi Bruno,
>> 
>> Bruno Victal <mirai@makinata.eu> writes:
>> 
>>> 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?
>> 
>> Is this a use case in actual use?  It seems a bit of a stretch in my
>> mind, especially considering the service was already difficult to get
>> working in its default configuration; I doubt someone would go out of
>> their way to manage multiple distinctly configured pulseaudio daemons
>> :-).  But if it's something in actual use providing value, I don't mind
>> to keep it until we have a better way to extend a common set of basic
>> properties for services in general.
>
> I added this field because while I was refactoring this in #59866 I cleared 
> #:environment-variables
> from the service constructor as they were setting “wrong variables”
> (stemming from the abuse of the service
> as a home service) and since I was mostly interested on the network
> (http) outputs which didn't require any
> variables set I didn't notice any issues until I learned about
> PulseAudio's rtp modules and tried using them.
>
> With this, I realized that the environment variables should be
> adjustable in order for the service to be flexible
> enough for general usage. (in general I prefer the services to be less 
> opinionated)
>
> Though originally thought towards multiple PulseAudio configurations, I 
> should stress that PulseAudio
> (and PipeWire when we get there) understand a plethora of environment 
> variables:

In general I try to stick to the YAGNI principle (You Ain't Gonna Need
It), because adding things without a current real world test often means
they'll need to be adjusted in the future when someone really starts
using them, and it's then harder because they've been made public and
can't easily be changed.

This commit mostly meant to open a discussion, which has now been had
(thanks!), so I'll drop the commit removing #:environment-variables and
push a fresh series.

Thanks for your patience!

-- 
Thanks,
Maxim