bug#61570: Backward incompatible changes in mpd-service-type

[Bruno Victal]

Hi Maxim,

On 2023-02-17 12:53, Maxim Cournoyer wrote:
> Hi!
> 
> I wanted to note some findings regarding the improved
> mpd-configuration/mdp-service-type (thanks!)
> (5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1, "services: mpd: Refactor MPD
> service.") after having reconfigured my machine yesterday.  I've noted
> these two backward incompatible changes:
> 
> 1. the mixer type must now be a string instead of a symbol, and takes
> "none" instead of 'null for the null mixer:

Indeed, this was an unintentional API breakage. IMO taking symbols here
always stood out since most of the fields were happy taking strings as values.
A deprecated warning and a symbol->string can be added here for compatibility.

> --8<---------------cut here---------------start------------->8---
> @@ -239,7 +239,7 @@
>                         (mpd-output
>                          (name "streaming")
>                          (type "httpd")
> -                        (mixer-type 'null)
> +                        (mixer-type "none")
>                          (extra-options
>                           `((encoder . "lame")
>                             (port    . "6601")
> --8<---------------cut here---------------end--------------->8---
> 
> It'd be nice if there was some deprecation warning and automatic healing
> code for now.  The doc should also be updated, because it still mention
> the @code{null} value:
> 
> --8<---------------cut here---------------start------------->8---
>      ‘mixer-type’ (default: ‘"none"’) (type: string)
>           This field accepts a string that specifies which mixer should
>           be used for this audio output: the ‘hardware’ mixer, the
>           ‘software’ mixer, the ‘null’ mixer (allows setting the volume,
>           but with no effect; this can be used as a trick to implement
>           an external mixer External Mixer) or no mixer (‘none’).
> --8<---------------cut here---------------end--------------->8---

There's no mistake here, 'null' mixer is distinct from 'none' mixer.
<https://mpd.readthedocs.io/en/latest/user.html#configuring-audio-outputs>

> 2.  The MPD user appears to be created instead of using an existing
> one.  I was using my own account, like this:
> 
> --8<---------------cut here---------------start------------->8---
>       (service mpd-service-type
>                (mpd-configuration
>                 (user "maxim")
>                 (address "0.0.0.0")
>                 (outputs
>                  (list (mpd-output)
>                        (mpd-output
>                         (name "streaming")
>                         (type "httpd")
>                         (mixer-type "none")
>                         (extra-options
>                          `((encoder . "lame")
>                            (port    . "6601")
>                            (bitrate . "320"))))))))
> --8<---------------cut here---------------end--------------->8---
> 
> and 'guix system reconfigure' is now warning that a user is defined
> twice, [...]

This is an unfortunate situation arising from a bug before the service was 
refactored.
Before d7fd9ec209f72e9cfff04a48bf16e092f258d8ff (actually 
5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1)
mpd-service-type contained a service-extension for %mpd-accounts where the 
values for both group and user were hardcoded to "mpd"
but this was actually never used since shepherd would launch the service using 
root and mpd would downgrade its permissions and switch
to the user specified in the mpd-configuration record since this field is 
serialized to the configuration file.

No "warning for user defined twice" would show precisely because the 
service-extension %mpd-accounts for account-service-type was
hardcoded, unless your "user-account" happened to be named "mpd".

%mpd-accounts being hardcoded also meant that the mpd-service-activation 
procedure made little sense. (since it was
chown'ing and chmod'ing directories from the "mpd" user to the value in the 
'user' field from mpd-configuration record type)

> [...] and my user description following reconfiguration is: "Music
> Player Daemon (MPD) user" :-).

AFAIK, the 'users' (resp. 'groups') fields from the 'operating-system' record 
type do not have precedence over account-service-type service extensions
(though this should be the case) so it's "whoever remains" after duplicates are 
filtered out, which in this case mpd-service-type happened to "win the
race".
I'd expect this behavior to happen with any service whose configuration 
record-type contains a user (resp. group) field that is relayed to a
account-service-type service-extension. (examples would be some of the git 
services if I'm not mistaken)

Another way to trigger this is with 'radicale', by manually creating the 
'radicale' user in 'operating-system' (say that you want to change the
HOME directory since its where the data is saved to) and adding 
'radicale-service-type' to 'services'.


> Perhaps it should check if a user already exists and not add it if it
> does?  Else an error rather than a warning when multiple same-name users
> are defined would be more appropriate, I think. 

I don't think service definitions have access to the operating-system field,
this was one of the hurdles when #60735 was being tackled.

IMO, this "issue" is avoided by not using the mpd-service-type as a "home 
service", which is what you're trying to do
when you pass it your own "interactive" user account.
Forcing mpd-service-type as a home service is fraught with subtle 
peculiarities, one of them was noted in commit message
637a9c3b264fe8eb76b6ed9f104b635d95632be6 and was discussed in #59866.

This service should be used "system-wide", which could be streaming oriented 
setups (via HTTP, icecast, PulseAudio + rtp, ...) or
system-wide PulseAudio. (not recommended)
"Home service" use cases should get its own home service definition, either by 
reusing many of the record-types and procedures here
or simply by inheriting and selectively deleting some of the service 
extensions. (such as account-service-type)


Cheers,
Bruno