bug#63082: [PATCH 04/17] services: mpd: Obsolete the 'group' field.

[Bruno Victal]

On 2023-04-28 15:26, Maxim Cournoyer wrote:
> Prior to this change, there was a discrepancy where a user could have
> disagreeing groups between the group and user fields (the user field being a
> <user-account> record, which includes its primary group as a string).  This
> could have caused problems because the USER's group was being used to set the
> file permissions, while the GROUP name was serialized to MPD's configuration,
> and MPD would use it to set the group of its running process.  Synchronizing
> both is not practical, as it can easily lead to slightly different
> <user-account> objects conflicting, again causing problems.
> 
> The compromise is to obsolete the 'group' field.  A group can still be
> configured via the 'user' field, which accepts a <user-account> object, with
> the limitation that the group should already exist.
> 
> * gnu/services/audio.scm (%lazy-group): Delete variable.
> (%set-user-group): Delete procedure.
> (mpd-serialize-user-group): Likewise.
> (%mpd-user) [group]: Default to "audio".
> (%mpd-group): Delete variable.
> (mpd-group-sanitizer, mympd-group-sanitizer): Adjust sanitizers.
> (mpd-configuration, mympd-configuration) [group]: Default to #f.  Update doc.
> (mpd-accounts, mympd-accounts): Remove group.
> (%mympd-user) [group]: Default to "nogroup".
> * doc/guix.texi: Regenerate doc.
> ---
>  doc/guix.texi          | 15 ++++----
>  gnu/services/audio.scm | 80 ++++++++++++------------------------------
>  2 files changed, 28 insertions(+), 67 deletions(-)
> 
> diff --git a/doc/guix.texi b/doc/guix.texi
> index f8acdbd6b5..34703b1698 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -33571,8 +33571,8 @@ Audio Services
>  @item @code{user} (type: user-account)
>  The user to run mpd as.
>  
> -@item @code{group} (type: user-group)
> -The group to run mpd as.
> +@item @code{group} (default: @code{#f}) (type: boolean)
> +Obsolete.  Do not use.

[...]


>  
>  @item @code{shepherd-requirement} (default: @code{()}) (type: list-of-symbol)
>  This is a list of symbols naming Shepherd services that this service
> @@ -33824,15 +33824,12 @@ Audio Services
>  This is a list of symbols naming Shepherd services that this service
>  will depend on.
>  
> -@item @code{user} (default: @code{%mympd-user}) (type: user-account)
> +@item @code{user} (type: user-account)
>  Owner of the @command{mympd} process.
>  
> -The default @code{%mympd-user} is a system user with the name ``mympd'',
> -who is a part of the group @var{group} (see below).
> -@item @code{group} (default: @code{%mympd-group}) (type: user-group)
> -Owner group of the @command{mympd} process.
> +@item @code{group} (default: @code{#f}) (type: boolean)
> +Obsolete.  Do not use.

I'd skip documenting obsolete fields.

>  
> -The default @code{%mympd-group} is a system group with name ``mympd''.
>  @item @code{work-directory} (default: @code{"/var/lib/mympd"}) (type: string)
>  Where myMPD will store its data.
>  
> @@ -33872,7 +33869,7 @@ Audio Services
>  Override URI to myMPD.  See
>  @uref{https://github.com/jcorporation/myMPD/issues/950}.
>  
> -@item @code{script-acl} (default: @code{(mympd-ip-acl (allow 
> '("127.0.0.1")))}) (type: maybe-mympd-ip-acl)
> +@item @code{script-acl} (type: maybe-mympd-ip-acl)
>  ACL to access the myMPD script backend.

Unrelated change?

>  (define mpd-deprecated-fields '((music-dir . music-directory)
> @@ -242,15 +224,9 @@ (define (mpd-user-sanitizer value)
>           (configuration-field-error #f 'user value))))
>  
>  (define (mpd-group-sanitizer value)
> -  (cond ((user-group? value) value)
> -        ((string? value)
> -         (warning (G_ "string value for 'group' is deprecated, use \
> -user-group instead~%"))
> -         (user-group
> -          (inherit %mpd-group)
> -          (name value)))
> -        (else
> -         (configuration-field-error #f 'group value))))
> +  (when value
> +    (warning (G_ "'group' in <mpd-configuration> is obsolete; ignoring~%")))
> +  #f)

You can drop the trailing #f I think.

>  
>  ;;;
>  
> @@ -407,9 +383,10 @@ (define-configuration mpd-configuration
>     (sanitizer mpd-user-sanitizer))
>  
>    (group
> -   (user-group %mpd-group)
> -   "The group to run mpd as."
> -   (sanitizer mpd-group-sanitizer))
> +   (boolean #f)
> +   "Obsolete.  Do not use."
> +   (sanitizer mpd-group-sanitizer)
> +   (serializer empty-serializer))

You can simply use empty-serializer after (or before) sanitizer, it is a 
recognized literal for define-configuration.


>  
>  (define (mympd-group-sanitizer value)
> -  (cond ((user-group? value) value)
> -        ((string? value)
> -         (warning (G_ "string value for 'group' is not supported, use \
> -user-group instead~%"))
> -         (user-group
> -          (inherit %mympd-group)
> -          (name value)))
> -        (else
> -         (configuration-field-error #f 'group value))))
> +  (when value
> +    (warning (G_ "'group' in <mympd-configuration> is obsolete; 
> ignoring~%")))
> +  #f)

Trailing #f as mentioned above.


> @@ -737,10 +704,10 @@ (define-configuration/no-serialization 
> mympd-configuration
>     empty-serializer)
>  
>    (group
> -   (user-group %mympd-group)
> -   "Owner group of the @command{mympd} process."
> +   (boolean #f)
> +   "Obsolete.  Do not use."
>     (sanitizer mympd-group-sanitizer)
> -   empty-serializer)
> +   (serializer empty-serializer))

empty-serializer is a literal here. (although it's simply being used for 
indication per the comment above this record-type)

>  
>    (work-directory
>     (string "/var/lib/mympd")
> @@ -904,12 +871,9 @@ (define (mympd-shepherd-service config)
>         (stop #~(make-kill-destructor))))))
>  
>  (define (mympd-accounts config)
> -  (match-record config <mympd-configuration> (user group)
> -    ;; TODO: Deprecation code, to be removed.
> -    (let ((user (if (eq? (user-account-group user) %lazy-group)
> -                    (set-user-group user group)
> -                    user)))
> -      (list user group))))
> +  (match-record config <mympd-configuration>
> +    (user)
> +    (list user)))

Nitpick, personally I'm a fan of styling this part as:

--8<---------------cut here---------------start------------->8---
(match-record config <mympd-configuration> (user)
  (list user))
--8<---------------cut here---------------end--------------->8---