Hi,
Bruno Victal <mi...@makinata.eu> writes:
> Hi Maxim,
>
> On 2023-07-25 21:48, Maxim Cournoyer wrote:
>> Hi Bruno,
>>
>> Bruno Victal <mi...@makinata.eu> writes:
>>
>>> Hi Maxim,
>>>
>>> On 2023-05-05 19:28, Maxim Cournoyer wrote:
>>>> * gnu/services/audio.scm (mpd-shepherd-service): Register a new update
>>>> action.
>>>> * doc/guix.texi (Audio Services): Document it.
>>>> ---
>>>> doc/guix.texi | 10 ++++++++++
>>>> gnu/services/audio.scm | 11 +++++++++++
>>>> 2 files changed, 21 insertions(+)
>>>>
>>>
>>> I've been looking at this part for the past few weeks in attempt to
>>> make it more robust and after countless hours, I'd advise against this
>>> (in its current form), reason being that this only works if your
>>> configuration happens to match the default values used by mpc.
>>>
>>> My attempts at getting the values from the configuration into
>>> something that mpc understands have been unsuccessful. Not only the
>>> decision “logic” of what values to pass is non-trivial, parsing the
>>> endpoints field has been so far a complete nightmare. (with interesting
>>> gems like IPv6 address formats that the daemon is happy to use yet
>>> mpc will reject)
>>>
>>> Having the proper hostname (and port) intelligently deduced from
>>> the endpoints field is a big minefield that is likely to end in
>>> unmaintainable spaghetti.
>>>
>>> Short of introducing additional fields like “internal-mpc-host” and
>>> “internal-mpc-port”, you could modify this to relay the
>>> 'environment-variables' field for mpc as well. (since it can make use
>>> of the MPD_HOST and MPD_PORT varibles if present)
>>
>> Apologies if it's been a couple weeks, but was the above comment really
>> meant for patch 02/16 of this series ("services: mpd: Add an 'update'
>> action to trigger a database update.") ? I don't see how they relate.
>
> Yes, since this action uses 'mpc' to issue the 'update' command and
> the way it works is that mpc is using a 'default' value to connect to MPD.
>
> The 'default' works until you change the 'endpoints' to something that
> _doesn't_ match the defaults that 'mpc' uses (e.g. [fe80::1234%eno1],
> /var/run/mpd/not-your-expected-name.socket, 2001:db8::1, etc.)
>
> Since the 'endpoints' field is equivalent to MPDs 'bind_to_address'
> directive [1], you have to take into account the flexible formats it allows
> and the 'port' field as well. This makes it somewhat non-trivial to
> inspect the 'endpoints' and 'port' fields to select an address for the
> shepherd 'update' action to use. You also need to take into account that
> you have to pass the address and port separately (if needed) for 'mpc'.
>
> One additional avenue (from the ones originally listed) we could explore
> is to make the endpoints field no longer a maybe-type. We set a default
> value (that is aligned with upstream, like localhost:6600) and also set
> an “internal” unix socket to be used primarily by shepherd.
>
> [1]: <https://mpd.readthedocs.io/en/latest/user.html#listeners>
Thanks for the analysis. I've reverted the commit, as I don't intend to
work toward fixing it.
--
Thanks,
Maxim
