bug#63082: mpd defaul configuration does not work ('No database' error)

bug#63082: mpd defaul configuration does not work ('No database' error)

[Maxim Cournoyer]

Hello,

Our MPD service does not work out of the box.  Running under its default
'mpd' user, clients would get the following error attempting to update
its database, e.g. by running 'mpc update':

--8<---------------cut here---------------start------------->8---
MPD error: database
--8<---------------cut here---------------end--------------->8---

This particular problem was solved by adding the following field to my
mpd-configuration record (thanks to Bruno):

--8<---------------cut here---------------start------------->8---
                (database (mpd-plugin
                           (plugin "simple")
                           (extra-options
                            '((path . "/var/lib/mpd/database")
                              (cache-directory . "/var/cache/mpd")))))
--8<---------------cut here---------------end--------------->8---

and running

--8<---------------cut here---------------start------------->8---
mkdir /var/cache/mpd && chown mpd:users /var/cache/mpd
--8<---------------cut here---------------end--------------->8---

Now the above error is gone, but I do not hear any audio coming out, so
there seems to be another problem.

I'm opening this ticket so that we can document and resolve all these
issues in the configuration, so that hopefully our mpd system service
can work out of the box.

-- 
Thanks,
Maxim

bug#63082: mpd defaul configuration does not work ('No database' error)

[Maxim Cournoyer]

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hello,
>
> Our MPD service does not work out of the box.  Running under its default
> 'mpd' user, clients would get the following error attempting to update
> its database, e.g. by running 'mpc update':
>
> MPD error: database
>
>
> This particular problem was solved by adding the following field to my
> mpd-configuration record (thanks to Bruno):
>
>                 (database (mpd-plugin
>                            (plugin "simple")
>                            (extra-options
>                             '((path . "/var/lib/mpd/database")
>                               (cache-directory . "/var/cache/mpd")))))
>
>
> and running
>
> mkdir /var/cache/mpd && chown mpd:users /var/cache/mpd

The cache-directory configuration doesn't seem to be useful so far;
nothing was being populated under /var/cache/mpd.

in /var/log/messages, pulseaudio throws a couple errors:

--8<---------------cut here---------------start------------->8---
Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] module-jackdbus-detect.c: Unable to contact D-Bus session bus: org.freedesktop.DBus.Error.NotSupported: Unable to autolaunch a dbus-daemon without a $DISPLAY for X11
Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] module.c: Failed to load module "module-jackdbus-detect" (argument: "channels=2"): initialization failed.
Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] cli-command.c: stat('/gnu/store/5ahapvp7rnd2ymakyjv1pwwdav7w9wdc-pulseaudio-16.1/etc/pulse/default.pa.d'): No such file or directory
Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] main.c: No card found by this name or index.
Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] main.c: Source alsa_input.pci-0000_01_01.0.analog-mono does not exist.
Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] main.c: Sink alsa_output.pci-0000_01_01.0.analog-surround-40 does not exist.
Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] server-lookup.c: Unable to contact D-Bus: org.freedesktop.DBus.Error.NotSupported: Unable to autolaunch a dbus-daemon without a $DISPLAY for X11
Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] main.c: Unable to contact D-Bus: org.freedesktop.DBus.Error.NotSupported: Unable to autolaunch a dbus-daemon without a $DISPLAY for X11
--8<---------------cut here---------------end--------------->8---

-- 
Thanks,
Maxim

bug#63082: mpd defaul configuration does not work ('No database' error)

[Liliana Marie Prikler]

Hi Maxim,

Am Dienstag, dem 25.04.2023 um 23:11 -0400 schrieb Maxim Cournoyer:
> The cache-directory configuration doesn't seem to be useful so far;
> nothing was being populated under /var/cache/mpd.
> 
> in /var/log/messages, pulseaudio throws a couple errors:
> 
> --8<---------------cut here---------------start------------->8---
> Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] module-
> jackdbus-detect.c: Unable to contact D-Bus session bus:
> org.freedesktop.DBus.Error.NotSupported: Unable to autolaunch a dbus-
> daemon without a $DISPLAY for X11
> Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] module.c:
> Failed to load module "module-jackdbus-detect" (argument:
> "channels=2"): initialization failed.
> Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] cli-
> command.c: stat('/gnu/store/5ahapvp7rnd2ymakyjv1pwwdav7w9wdc-
> pulseaudio-16.1/etc/pulse/default.pa.d'): No such file or directory
> Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] main.c: No
> card found by this name or index.
> Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] main.c:
> Source alsa_input.pci-0000_01_01.0.analog-mono does not exist.
> Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] main.c:
> Sink alsa_output.pci-0000_01_01.0.analog-surround-40 does not exist.
> Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] server-
> lookup.c: Unable to contact D-Bus:
> org.freedesktop.DBus.Error.NotSupported: Unable to autolaunch a dbus-
> daemon without a $DISPLAY for X11
> Apr 25 23:06:52 localhost pulseaudio[10356]: [pulseaudio] main.c:
> Unable to contact D-Bus: org.freedesktop.DBus.Error.NotSupported:
> Unable to autolaunch a dbus-daemon without a $DISPLAY for X11
> --8<---------------cut here---------------end--------------->8---
Configuring MPD in a vacuum meaningfully is a little difficult. 
Usually, you don't want it to spawn its own pulse, but rather to
communicate with an already existing server that has to allow network
access.  Rather than supposing this, we could make the default a null
sink or similar.

Cheers

bug#63082: [PATCH 00/17] Improve out-of-the-box experience with mpd-service-type

[Maxim Cournoyer]

Hi,

I've recently had enough issues trying to setup a very basic, minimal MTP
server that I've found an itch to improve a few things, leading to the patches below.

It obsoletes a few things to make the configuration less error prone or easier
to reason with, and focus on making things "just work" out of the box, using
something as minimal as:

    (service mpd-service-type
             (mpd-configuration
              (music-directory "/srv/music")))

Previously this would fail due to requiring the user to provide a database
file themselves in the configuration, and the default mpd user would also fail
to access the /dev/snd/* devices due to the mpd user not deing in the audio
group.  Having 'group' in mpd.conf would also cause mpd to strip the user's
supplementary groups, making this issue more likely.

Other note-worthy changes:

* Log to syslog by default (to match upstream behavior)
* Auto-detect the mixer type (to match upstream behavior)

I hope the series helps you to setup MPD too.

Maxim Cournoyer (17):
  services: mpd: Add an 'update' action to trigger a database update.
  services: mpd: Streamline mpd-user-sanitizer and mympd-user-sanitizer.
  services: mpd: Rename %set-user-group to set-user-group.
  services: mpd: Obsolete the 'group' field.
  services: mpd: List log-level in decreasing verbosity order in doc.
  services: mympd: Fix log file name.
  services: mpd: Log to syslog by default.
  services: mpd: Only rotate log when a log file is specified.
  services: mpd: Let Shepherd effect the user/group change.
  system: accounts: Export <user-account>.
  services: mpd: Warn when the MPD user is not in the "audio" group.
  services: mpd: Auto-detect mpd-output mixer type by default.
  services: mpd: Fix indentation.
  services: mpd: Obsolete 'environment-variables' field.
  services: mpd: Provision a default cache directory and set HOME.
  services: mpd: Update basic example.
  services: Avoid 'delete' overrides warning in audio module.

 doc/guix.texi           |  79 ++++++----
 gnu/services/audio.scm  | 311 +++++++++++++++++++++++-----------------
 gnu/system/accounts.scm |   3 +-
 3 files changed, 228 insertions(+), 165 deletions(-)


base-commit: ccf64b6a8b8718a8bb69719cf9ed2873464e3850
prerequisite-patch-id: eace011dd080f709a8eeb77c7a739f87079dbb81
prerequisite-patch-id: fd596ecff861483a486910ca0feecded27f6a4a2
prerequisite-patch-id: 948c73edc0a8a0a21b1d4f6878d3f09158059f38
prerequisite-patch-id: becfd217e53934fe9ef16939ff433e5ed00a4b1e
prerequisite-patch-id: 20f12e01af25b881e362ea7dc837a07aeec8a489
prerequisite-patch-id: 7534d08dbc06589ce0fe7bd306585321cb5385d1
-- 
2.39.2

bug#63082: [PATCH 01/17] services: mpd: Add an 'update' action to trigger a database update.

[Maxim Cournoyer]

* 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(+)

diff --git a/doc/guix.texi b/doc/guix.texi
index bdacb56af5..f8acdbd6b5 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33546,6 +33546,16 @@ Audio Services
           (port "6666")))
 @end lisp
 
+Most MPD clients will trigger a database update upon connecting, but you
+can also use the @code{update} action do to so:
+
+@example
+herd update mpd
+@end example
+
+All the MPD configuration fields are documented below, and a more
+complex example follows.
+
 @defvar mpd-service-type
 The service type for @command{mpd}
 @end defvar
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 8c061da47f..6e4ce3f9fb 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -620,6 +620,17 @@ (define (mpd-shepherd-service config)
                           (format #t
                                   "Issued SIGHUP to Service MPD (PID ~a)."
                                   pid))
+                        (format #t "Service MPD is not running.")))))
+              (shepherd-action
+               (name 'update)
+               (documentation "Request MPD to update its music database.")
+               (procedure
+                #~(lambda (pid)
+                    (if pid
+                        (begin
+                          (invoke #$(file-append mpd-mpc "/bin/mpc") "update")
+                          (format #t "Database update requested for service \
+MPD (PID ~a)." pid))
                         (format #t "Service MPD is not running.")))))))))))
 
 (define (mpd-accounts config)
-- 
2.39.2

bug#63082: [PATCH 02/17] services: mpd: Streamline mpd-user-sanitizer and mympd-user-sanitizer.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-user-sanitizer, %mympd-user): Remove extraneous
group field, already inherited.
(%mpd-user, %mympd-user): Clarify %lazy-group explanatory comment.  Fix
indentation.
---
 gnu/services/audio.scm | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 6e4ce3f9fb..dc83479e40 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -184,13 +184,15 @@ (define-maybe boolean (prefix mpd-))
 
 (define %mpd-user
   (user-account
-      (name "mpd")
-      (group %lazy-group)
-      (system? #t)
-      (comment "Music Player Daemon (MPD) user")
-      ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data
-      (home-directory "/var/lib/mpd")
-      (shell (file-append shadow "/sbin/nologin"))))
+   (name "mpd")
+   ;; XXX: This is a place-holder to be lazily substituted in (…-accounts)
+   ;; with the value from the 'group' field of <mpd-configuration>.
+   (group %lazy-group)
+   (system? #t)
+   (comment "Music Player Daemon (MPD) user")
+   ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data.
+   (home-directory "/var/lib/mpd")
+   (shell (file-append shadow "/sbin/nologin"))))
 
 (define %mpd-group
   (user-group
@@ -235,10 +237,7 @@ (define (mpd-user-sanitizer value)
 user-account instead~%"))
          (user-account
           (inherit %mpd-user)
-          (name value)
-          ;; XXX: This is to be lazily substituted in (…-accounts)
-          ;; with the value from 'group'.
-          (group %lazy-group)))
+          (name value)))
         (else
          (configuration-field-error #f 'user value))))
 
@@ -676,12 +675,14 @@ (define-maybe/no-serialization mympd-ip-acl)
 
 (define %mympd-user
   (user-account
-      (name "mympd")
-      (group %lazy-group)
-      (system? #t)
-      (comment "myMPD user")
-      (home-directory "/var/empty")
-      (shell (file-append shadow "/sbin/nologin"))))
+   (name "mympd")
+   ;; XXX: This is a place-holder to be lazily substituted in 'mympd-accounts'
+   ;; with the value from the 'group' field of <mympd-configuration>.
+   (group %lazy-group)
+   (system? #t)
+   (comment "myMPD user")
+   (home-directory "/var/empty")
+   (shell (file-append shadow "/sbin/nologin"))))
 
 (define %mympd-group
   (user-group
@@ -696,10 +697,7 @@ (define (mympd-user-sanitizer value)
 user-account instead~%"))
          (user-account
           (inherit %mympd-user)
-          (name value)
-          ;; XXX: this is to be lazily substituted in (…-accounts)
-          ;; with the value from 'group'.
-          (group %lazy-group)))
+          (name value)))
         (else
          (configuration-field-error #f 'user value))))
 
-- 
2.39.2

bug#63082: [PATCH 03/17] services: mpd: Rename %set-user-group to set-user-group.

[Maxim Cournoyer]

The convention to use % as a prefix is for "special" variables rather than
procedures.

* gnu/services/audio.scm ((%set-user-group): Rename to...
(set-user-group): ... this.
---
 gnu/services/audio.scm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index dc83479e40..7874539810 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -143,7 +143,7 @@ (define list-of-symbol?
 ;; Helpers for deprecated field types, to be removed later.
 (define %lazy-group (make-symbol "%lazy-group"))
 
-(define (%set-user-group user group)
+(define (set-user-group user group)
   (user-account
    (inherit user)
    (group (user-group-name group))))
@@ -636,7 +636,7 @@ (define (mpd-accounts config)
   (match-record config <mpd-configuration> (user group)
     ;; TODO: Deprecation code, to be removed.
     (let ((user (if (eq? (user-account-group user) %lazy-group)
-                    (%set-user-group user group)
+                    (set-user-group user group)
                     user)))
       (list user group))))
 
@@ -907,7 +907,7 @@ (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)
+                    (set-user-group user group)
                     user)))
       (list user group))))
 
-- 
2.39.2

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

[Maxim Cournoyer]

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.
 
-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.
 
 @item @code{ssl?} (default: @code{#f}) (type: boolean)
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 7874539810..58262f7842 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -140,14 +140,6 @@ (define (uglify-field-name field-name)
 (define list-of-symbol?
   (list-of symbol?))
 
-;; Helpers for deprecated field types, to be removed later.
-(define %lazy-group (make-symbol "%lazy-group"))
-
-(define (set-user-group user group)
-  (user-account
-   (inherit user)
-   (group (user-group-name group))))
-
 \f
 ;;;
 ;;; MPD
@@ -175,9 +167,6 @@ (define (mpd-serialize-list-of-strings field-name value)
 (define (mpd-serialize-user-account field-name value)
   (mpd-serialize-string field-name (user-account-name value)))
 
-(define (mpd-serialize-user-group field-name value)
-  (mpd-serialize-string field-name (user-group-name value)))
-
 (define-maybe string (prefix mpd-))
 (define-maybe list-of-strings (prefix mpd-))
 (define-maybe boolean (prefix mpd-))
@@ -185,20 +174,13 @@ (define-maybe boolean (prefix mpd-))
 (define %mpd-user
   (user-account
    (name "mpd")
-   ;; XXX: This is a place-holder to be lazily substituted in (…-accounts)
-   ;; with the value from the 'group' field of <mpd-configuration>.
-   (group %lazy-group)
+   (group "audio")
    (system? #t)
    (comment "Music Player Daemon (MPD) user")
    ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data.
    (home-directory "/var/lib/mpd")
    (shell (file-append shadow "/sbin/nologin"))))
 
-(define %mpd-group
-  (user-group
-   (name "mpd")
-   (system? #t)))
-
 ;;; TODO: Procedures for deprecated fields, to be removed.
 
 (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)
 
 ;;;
 
@@ -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))
 
   (shepherd-requirement
    (list-of-symbol '())
@@ -633,12 +610,9 @@ (define (mpd-shepherd-service config)
                         (format #t "Service MPD is not running.")))))))))))
 
 (define (mpd-accounts config)
-  (match-record config <mpd-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 <mpd-configuration>
+    (user)
+    (list user)))
 
 (define mpd-service-type
   (service-type
@@ -676,9 +650,7 @@ (define-maybe/no-serialization mympd-ip-acl)
 (define %mympd-user
   (user-account
    (name "mympd")
-   ;; XXX: This is a place-holder to be lazily substituted in 'mympd-accounts'
-   ;; with the value from the 'group' field of <mympd-configuration>.
-   (group %lazy-group)
+   (group "nogroup")
    (system? #t)
    (comment "myMPD user")
    (home-directory "/var/empty")
@@ -702,15 +674,10 @@ (define (mympd-user-sanitizer value)
          (configuration-field-error #f 'user value))))
 
 (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)
+
 ;;;
 
 
@@ -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))
 
   (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)))
 
 (define (mympd-log-rotation config)
   (match-record config <mympd-configuration> (log-to)
-- 
2.39.2

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---

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

[Maxim Cournoyer]

Hi,

Bruno Victal <mirai@makinata.eu> writes:

[...]

>>  @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.

'define-configuration' doesn't allow me to skip documenting a field, I
think :-).

>>
>> -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?

From the generated doc, which was probably lagging behind the source.

>>  (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.

You are right that I could, but I prefer to leave it, as sanitizer
should return a value compatible with their field declared type.

>>
>>  ;;;
>>
>> @@ -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.

I was under the impression that form had been deprecated; per (gnu
services configuration), around line 217:

--8<---------------cut here---------------start------------->8---
(warning #f (G_ "specifying serializers after documentation is \
deprecated, use (serializer ~a) instead~%") (syntax->datum #'proc))
--8<---------------cut here---------------end--------------->8---

[...]

>>    (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:
>
> (match-record config <mympd-configuration> (user)
>   (list user))
>

Can't; it'd format this way in Emacs:

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

-- 
Thanks,
Maxim

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

[Bruno Victal]

On 2023-04-29 02:15, Maxim Cournoyer wrote:
> Hi,
> 
> Bruno Victal <mirai@makinata.eu> writes:
> 
> [...]
> 
>>>  @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.
> 
> 'define-configuration' doesn't allow me to skip documenting a field, I
> think :-).

Right, but it can be manually removed from doc/guix.texi for now.

>>>
>>> -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?
> > From the generated doc, which was probably lagging behind the source.

It's unfortunate but iirc this had to be manually edited in since generate-documentation
will skip “serializing” values it doesn't know how to represent.

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

[Maxim Cournoyer]

Hi Bruno,

Bruno Victal <mirai@makinata.eu> writes:

[...]

>>>> -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?
>> > From the generated doc, which was probably lagging behind the source.
>
> It's unfortunate but iirc this had to be manually edited in since generate-documentation
> will skip “serializing” values it doesn't know how to represent.

OK.  I'm not sure it's worth manually fiddling with the generated code
instead of keeping it as a reminder to fix it at the source :-).  It's
already a bit of a pain to copy-paste the snippets already.

-- 
Thanks,
Maxim

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

[Liliana Marie Prikler]

Am Freitag, dem 28.04.2023 um 10:26 -0400 schrieb Maxim Cournoyer:
> 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.
Most services generate both an account and a group, whereas MPD would
be the odd one out here.  Defaulting to mpd:audio also has some minor
consequences when group permissions entail semantics, as this would
allow everyone in the audio group group access to mpd's stuff, which
seems needlessly permissive.  For this reason I think it makes sense to
allow users to specify a group, though it need not necessarily be via
the group field – for instance, we could make the user-accounts visible
to allow both specification of (list user group) and user alone,
deprecating the user and group fields in the process.  (Though we could
still provide read accessors to those.)

This still leaves us with the question of how to make audio work out of
the box.  IIRC using supplementary groups does not suffice, because the
service won't work then; do I actually recall that correctly?  


Cheers

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

[Liliana Marie Prikler]

Am Samstag, dem 29.04.2023 um 08:26 +0200 schrieb Liliana Marie
Prikler:
> IIRC using supplementary groups does not suffice, because the
> service won't work then; do I actually recall that correctly?  
Reading the rest of the series it turns out that worry was unwarranted.
Supplementary groups do work as they should.

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

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Samstag, dem 29.04.2023 um 08:26 +0200 schrieb Liliana Marie
> Prikler:
>> IIRC using supplementary groups does not suffice, because the
>> service won't work then; do I actually recall that correctly?  
> Reading the rest of the series it turns out that worry was unwarranted.
> Supplementary groups do work as they should.

Correct, we can use a supplementary group for "audio".  This still
leaves the question as to what primary group to use though.  I think
I'll answer to this in your other comment.

-- 
Thanks,
Maxim

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

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Samstag, dem 29.04.2023 um 08:26 +0200 schrieb Liliana Marie
> Prikler:
>> IIRC using supplementary groups does not suffice, because the
>> service won't work then; do I actually recall that correctly?  
> Reading the rest of the series it turns out that worry was unwarranted.
> Supplementary groups do work as they should.

Perhaps the simplest, more natural way to fix this would be to do the
following:

1. Revert to use simple strings for the user/group, with their types to
maybe-string.

2. When the fields are specified, it's assumed that their corresponding
user/group already exist, as done for other services.  When unspecified,
then the service takes care to extend the user-accounts service with the
mpd user/group.

This means we do not have to expose user-accounts at the
<mpd-configuration>, which is not the right place for that anyway; it's
already exposed via the users/groups fields of the <operating-system>
record.

That's much easier to reason with, for both humans and the code, in my
opinion.

-- 
Thanks,
Maxim

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

[Liliana Marie Prikler]

Am Samstag, dem 29.04.2023 um 13:16 -0400 schrieb Maxim Cournoyer:
> This means we do not have to expose user-accounts at the
> <mpd-configuration>, which is not the right place for that anyway;
> it's already exposed via the users/groups fields of the <operating-
> system> record.
> 
> That's much easier to reason with, for both humans and the code, in
> my opinion.
We have the tools to make this meaningful with user-accounts already –
we've had them for some while in fact.

(operating-system
  ...
  (users (cons* alice bob mpd %base-user-accounts))
  (services (cons* (mpd-service (user (find mpd-user? users)))
                   %base-services))
  ...)

is a perfectly fine configuration given concrete values for alice, bob,
and mpd with mpd-user? matching the MPD user account by name.  The
point in providing user-accounts in the MPD service is so that the
right thing is done w.r.t. account creation regardless of whether the
user exists in users or not.  In the former case, it is checked that
definitions match (using pointer identity), in the latter the user is
automatically added.  "I will assume that a user by this name exists
even if it does not" is a recipe for shooting oneself in the knee.

Cheers

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

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Samstag, dem 29.04.2023 um 13:16 -0400 schrieb Maxim Cournoyer:
>> This means we do not have to expose user-accounts at the
>> <mpd-configuration>, which is not the right place for that anyway;
>> it's already exposed via the users/groups fields of the <operating-
>> system> record.
>>
>> That's much easier to reason with, for both humans and the code, in
>> my opinion.
> We have the tools to make this meaningful with user-accounts already –
> we've had them for some while in fact.
>
> (operating-system
>   ...
>   (users (cons* alice bob mpd %base-user-accounts))
>   (services (cons* (mpd-service (user (find mpd-user? users)))
>                    %base-services))
>   ...)
>
> is a perfectly fine configuration given concrete values for alice, bob,
> and mpd with mpd-user? matching the MPD user account by name.  The
> point in providing user-accounts in the MPD service is so that the
> right thing is done w.r.t. account creation regardless of whether the
> user exists in users or not.  In the former case, it is checked that
> definitions match (using pointer identity), in the latter the user is
> automatically added.  "I will assume that a user by this name exists
> even if it does not" is a recipe for shooting oneself in the knee.

I agree looks nice "on paper", but in practice, I was confronted with
the following problem, which is enough annoying to make me want to go
back to the simpler string contract:

A <user-account> record encodes a lossy version of a <user-group> as a
string, as its 'group' field.  This way of specifying a group implies it
already exist, since it doesn't capture all the <user-group> details,
e.g.: is it a system group or not?

In the current mpd-configuration, to use my own user, I must also
provide the matching group as a <user-group> record, even if
e.g. 'users' is something I've never created myself and don't really
have a clue as to how it was defined without looking at the source, yet
it's important that it matches the original definition otherwise I'd
have two same-named groups differing only subtly, which would introduce
issues probably harder to diagnose than "sorry, no such group!"

One way that seems like it'd solve it is to make the group field of a
<user-account> accept a <user-group>; then the user object would be
self-contained as far as extending user-accounts goes; the API becomes a
bit more obtuse though, especially when you simply want to specify a
group known to exist ('users', 'audio', 'netdev', etc.).  We'd probably
now need to export %users-group, %audio-group, etc. to make this API a
bit more manageable/convenient.

-- 
Thanks,
Maxim

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

[Liliana Marie Prikler]

Hi Maxim,

Am Sonntag, dem 30.04.2023 um 21:07 -0400 schrieb Maxim Cournoyer:
> > In the current mpd-configuration, to use my own user, I must also
> > provide the matching group as a <user-group> record, even if
> > e.g. 'users' is something I've never created myself and don't
> > really
> > have a clue as to how it was defined without looking at the source,
> > yet it's important that it matches the original definition
> > otherwise
> > I'd have two same-named groups differing only subtly, which would
> > introduce issues probably harder to diagnose than "sorry, no such
> > group!"
The "find by name" pattern applies here as well. We could extend the
semantics of group field so that if a string is passed, %base-groups is
searched for a matching name first instead of constructing a new group.
This would allow you to more easily specify (group "users") for
example.

> > One way that seems like it'd solve it is to make the group field of
> > a
> > <user-account> accept a <user-group>; then the user object would be
> > self-contained as far as extending user-accounts goes; the API
> > becomes a bit more obtuse though, especially when you simply want
> > to
> > specify a group known to exist ('users', 'audio', 'netdev', etc.).
Not really. If the <user-account> allowed for a <user-group> group, we
could drop the group field in the MPD specification. The semantics for
string groups would remain unchanged, whereas <user-group> groups would
get added to the accounts service as though they had been specified via
the groups mechanic. We'd only have to twiddle with account service
there and the change would probably benefit every service that requires
an account:group pair.

Cheers

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

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Hi Maxim,
>
> Am Sonntag, dem 30.04.2023 um 21:07 -0400 schrieb Maxim Cournoyer:
>> > In the current mpd-configuration, to use my own user, I must also
>> > provide the matching group as a <user-group> record, even if
>> > e.g. 'users' is something I've never created myself and don't
>> > really
>> > have a clue as to how it was defined without looking at the source,
>> > yet it's important that it matches the original definition
>> > otherwise
>> > I'd have two same-named groups differing only subtly, which would
>> > introduce issues probably harder to diagnose than "sorry, no such
>> > group!"
> The "find by name" pattern applies here as well. We could extend the
> semantics of group field so that if a string is passed, %base-groups is
> searched for a matching name first instead of constructing a new group.
> This would allow you to more easily specify (group "users") for
> example.
>
>> > One way that seems like it'd solve it is to make the group field of
>> > a
>> > <user-account> accept a <user-group>; then the user object would be
>> > self-contained as far as extending user-accounts goes; the API
>> > becomes a bit more obtuse though, especially when you simply want
>> > to
>> > specify a group known to exist ('users', 'audio', 'netdev', etc.).
> Not really. If the <user-account> allowed for a <user-group> group, we
> could drop the group field in the MPD specification. The semantics for
> string groups would remain unchanged, whereas <user-group> groups would
> get added to the accounts service as though they had been specified via
> the groups mechanic. We'd only have to twiddle with account service
> there and the change would probably benefit every service that requires
> an account:group pair.

I think I'd favor the second option then (migrating <user-account> to
support both <user-group> and strings), so that every service could
benefit from the change, as you've noted.  I don't intend to work on it
in the scope of this series, but I think it'd be a welcome improvement!

-- 
Thanks,
Maxim

bug#63082: [PATCH 05/17] services: mpd: List log-level in decreasing verbosity order in doc.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-configuration) [log-level]: List log-level in
decreasing verbosity order in doc.
* doc/guix.texi (Audio Services): Update doc.
---
 doc/guix.texi          | 6 +++---
 gnu/services/audio.scm | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 34703b1698..1aa8dc2809 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33587,9 +33587,9 @@ Audio Services
 configuration file.
 
 @item @code{log-level} (type: maybe-string)
-Supress any messages below this threshold.  Available values:
-@code{notice}, @code{info}, @code{verbose}, @code{warning} and
-@code{error}.
+Supress any messages below this threshold.  The available values, in
+decreasing order of verbosity, are: @code{verbose}, @code{info},
+@code{notice}, @code{warning} and @code{error}.
 
 @item @code{music-directory} (type: maybe-string)
 The directory to scan for music files.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 58262f7842..1dc3204fc0 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -409,8 +409,8 @@ (define-configuration mpd-configuration
   (log-level
    maybe-string
    "Supress any messages below this threshold.
-Available values: @code{notice}, @code{info}, @code{verbose},
-@code{warning} and @code{error}.")
+The available values, in decreasing order of verbosity, are: @code{verbose},
+@code{info}, @code{notice}, @code{warning} and @code{error}.")
 
   (music-directory
    maybe-string
-- 
2.39.2

bug#63082: [PATCH 06/17] services: mympd: Fix log file name.

[Maxim Cournoyer]

* gnu/services/audio.scm (mympd-configuration): Adjust log file name to match
documentation.
---
 gnu/services/audio.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 1dc3204fc0..0b7a25d9ef 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -744,7 +744,7 @@ (define-configuration/no-serialization mympd-configuration
    "How much detail to include in logs, possible values: @code{0} to @code{7}.")
 
   (log-to
-   (string-or-symbol "/var/log/mympd/log")
+   (string-or-symbol "/var/log/mympd.log")
    "Where to send logs. By default, the service logs to
 @file{/var/log/mympd.log}. The alternative is @code{'syslog}, which
 sends output to the running syslog service under the @samp{daemon} facility."
-- 
2.39.2

bug#63082: [PATCH 06/17] services: mympd: Fix log file name.

[Bruno Victal]

On 2023-04-28 15:26, Maxim Cournoyer wrote:
> * gnu/services/audio.scm (mympd-configuration): Adjust log file name to match
> documentation.
> ---
>  gnu/services/audio.scm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
> index 1dc3204fc0..0b7a25d9ef 100644
> --- a/gnu/services/audio.scm
> +++ b/gnu/services/audio.scm
> @@ -744,7 +744,7 @@ (define-configuration/no-serialization mympd-configuration
>     "How much detail to include in logs, possible values: @code{0} to @code{7}.")
>  
>    (log-to
> -   (string-or-symbol "/var/log/mympd/log")
> +   (string-or-symbol "/var/log/mympd.log")
>     "Where to send logs. By default, the service logs to
>  @file{/var/log/mympd.log}. The alternative is @code{'syslog}, which
>  sends output to the running syslog service under the @samp{daemon} facility."

No, this is correct although perhaps /var/log/mympd/mympd.log would have been less error prone?
IIRC mympd is supposed to write to a subdirectory within /var/log and this change would break the service.

bug#63082: [PATCH 06/17] services: mympd: Fix log file name.

[Maxim Cournoyer]

Hi,

Bruno Victal <mirai@makinata.eu> writes:

> On 2023-04-28 15:26, Maxim Cournoyer wrote:
>> * gnu/services/audio.scm (mympd-configuration): Adjust log file name to match
>> documentation.
>> ---
>>  gnu/services/audio.scm | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
>> index 1dc3204fc0..0b7a25d9ef 100644
>> --- a/gnu/services/audio.scm
>> +++ b/gnu/services/audio.scm
>> @@ -744,7 +744,7 @@ (define-configuration/no-serialization mympd-configuration
>>     "How much detail to include in logs, possible values: @code{0} to @code{7}.")
>>  
>>    (log-to
>> -   (string-or-symbol "/var/log/mympd/log")
>> +   (string-or-symbol "/var/log/mympd.log")
>>     "Where to send logs. By default, the service logs to
>>  @file{/var/log/mympd.log}. The alternative is @code{'syslog}, which
>>  sends output to the running syslog service under the @samp{daemon} facility."
>
> No, this is correct although perhaps /var/log/mympd/mympd.log would have been less error prone?
> IIRC mympd is supposed to write to a subdirectory within /var/log and
> this change would break the service.

The only consumer of a 'log-to' is Shepherd, as far as I can see; so
mympd doesn't get to know where the log is going (its output goes to
stdout/stderr, captured by Shepherd), so shouldn't be writing anything
under /var/log/mympd.  Looking at upstream, they ship a fancy systemd
service which wouldn't log to text files but to the journal, closer in
spirit to syslog.

For symmetry with mpd, I've now adjusted logging of mympd to default to
syslog (and found it had an erroneous requirement on 'syslog rather than
syslogd with the test :-)).

-- 
Thanks,
Maxim

bug#63082: [PATCH 06/17] services: mympd: Fix log file name.

[Bruno Victal]

On 2023-04-29 02:49, Maxim Cournoyer wrote:
>> No, this is correct although perhaps /var/log/mympd/mympd.log would have been less error prone?
>> IIRC mympd is supposed to write to a subdirectory within /var/log and
>> this change would break the service.
> 
> The only consumer of a 'log-to' is Shepherd, as far as I can see; so
> mympd doesn't get to know where the log is going (its output goes to
> stdout/stderr, captured by Shepherd), so shouldn't be writing anything
> under /var/log/mympd.  Looking at upstream, they ship a fancy systemd
> service which wouldn't log to text files but to the journal, closer in
> spirit to syslog.

Oh you're right, I must have been misremembering this.

> For symmetry with mpd, I've now adjusted logging of mympd to default to
> syslog (and found it had an erroneous requirement on 'syslog rather than
> syslogd with the test :-)).
> 

Nice!

bug#63082: [PATCH 07/17] services: mpd: Log to syslog by default.

[Maxim Cournoyer]

Rationale: the tristate value was awkward to deal with, the default log file
name was odd (/var/log/mpd/log) and it required special attention to create
the 'mpd' parent directory as root and chowning it to the MPD user.  It also
didn't match the default behavior of MPD, which is to log to systemd or syslog
unless a log file is specified.

* gnu/services/audio.scm (mpd-log-file-sanitizer): New procedure.
(mpd-configuration) [log-file]: Remove default maybe value.  Add sanitizer.
(mpd-shepherd-service): Validate the log file parent directory exists and has
the right permissions.
* doc/guix.texi (Audio Services): Update doc.
---
 doc/guix.texi          |  7 +++----
 gnu/services/audio.scm | 29 ++++++++++++++++++++---------
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 1aa8dc2809..a71a05bcf3 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33581,10 +33581,9 @@ Audio Services
 @item @code{environment-variables} (default: @code{("PULSE_CLIENTCONFIG=/etc/pulse/client.conf" "PULSE_CONFIG=/etc/pulse/daemon.conf")}) (type: list-of-strings)
 A list of strings specifying environment variables.
 
-@item @code{log-file} (default: @code{"/var/log/mpd/log"}) (type: maybe-string)
-The location of the log file.  Set to @code{syslog} to use the local
-syslog daemon or @code{%unset-value} to omit this directive from the
-configuration file.
+@item @code{log-file} (type: maybe-string)
+The location of the log file.  Unless specified, the logs are collected
+by the local syslog daemon.
 
 @item @code{log-level} (type: maybe-string)
 Supress any messages below this threshold.  The available values, in
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 0b7a25d9ef..a1d1a3d2fe 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -228,7 +228,17 @@ (define (mpd-group-sanitizer value)
     (warning (G_ "'group' in <mpd-configuration> is obsolete; ignoring~%")))
   #f)
 
-;;;
+(define (mpd-log-file-sanitizer value)
+  (match value
+    (%unset-value
+     ;; XXX: While leaving the 'sys_log' option out of the mpd.conf file is
+     ;; supposed to cause logging to happen via systemd (elogind provides a
+     ;; compatible interface), this doesn't work (nothing gets logged); use
+     ;; syslog instead.
+     "syslog")
+    ((? string?)
+     value)
+    (_ (configuration-field-error #f 'user value))))
 
 ;; Generic MPD plugin record, lists only the most prevalent fields.
 (define-configuration mpd-plugin
@@ -401,10 +411,10 @@ (define-configuration mpd-configuration
    empty-serializer)
 
   (log-file
-   (maybe-string "/var/log/mpd/log")
-   "The location of the log file. Set to @code{syslog} to use the
-local syslog daemon or @code{%unset-value} to omit this directive
-from the configuration file.")
+   maybe-string
+   "The location of the log file.  Unless specified, the logs are collected by
+the local syslog daemon."
+   (sanitizer mpd-log-file-sanitizer))
 
   (log-level
    maybe-string
@@ -563,17 +573,18 @@ (define (mpd-shepherd-service config)
        (requirement `(user-processes loopback ,@shepherd-requirement))
        (provision '(mpd))
        (start #~(begin
-                  (and=> #$(maybe-value log-file)
-                         (compose mkdir-p dirname))
-
                   (let ((user (getpw #$username)))
                     (for-each
                      (lambda (x)
-                       (when (and x (not (file-exists? x)))
+                       ;; Take action on absolute file names, to filter out
+                       ;; the 'syslog' special value.
+                       (when (and x (string-prefix? "/" x)
+                                  (not (file-exists? x)))
                          (mkdir-p x)
                          (chown x (passwd:uid user) (passwd:gid user))))
                      (list #$(maybe-value playlist-directory)
                            (and=> #$(maybe-value db-file) dirname)
+                           (and=> #$(maybe-value log-file) dirname)
                            (and=> #$(maybe-value state-file) dirname)
                            (and=> #$(maybe-value sticker-file) dirname))))
 
-- 
2.39.2

bug#63082: [PATCH 07/17] services: mpd: Log to syslog by default.

[Bruno Victal]

On 2023-04-28 15:27, Maxim Cournoyer wrote:
> Rationale: the tristate value was awkward to deal with, the default log file
> name was odd (/var/log/mpd/log) and it required special attention to create
> the 'mpd' parent directory as root and chowning it to the MPD user.  It also
> didn't match the default behavior of MPD, which is to log to systemd or syslog
> unless a log file is specified.
> 
> * gnu/services/audio.scm (mpd-log-file-sanitizer): New procedure.
> (mpd-configuration) [log-file]: Remove default maybe value.  Add sanitizer.
> (mpd-shepherd-service): Validate the log file parent directory exists and has
> the right permissions.
> * doc/guix.texi (Audio Services): Update doc.

How about a similar approach taken in mympd for handling the logging parameter?
In any case, I'd like to remind you that mpd-service-type also has a rottlog service extension
so that also needs to be taken into account.

>    (log-level
>     maybe-string
> @@ -563,17 +573,18 @@ (define (mpd-shepherd-service config)
>         (requirement `(user-processes loopback ,@shepherd-requirement))
>         (provision '(mpd))
>         (start #~(begin
> -                  (and=> #$(maybe-value log-file)
> -                         (compose mkdir-p dirname))
> -
>                    (let ((user (getpw #$username)))
>                      (for-each
>                       (lambda (x)
> -                       (when (and x (not (file-exists? x)))
> +                       ;; Take action on absolute file names, to filter out
> +                       ;; the 'syslog' special value.
> +                       (when (and x (string-prefix? "/" x)
> +                                  (not (file-exists? x)))
>                           (mkdir-p x)
>                           (chown x (passwd:uid user) (passwd:gid user))))

I'd use mkdir-p/perms from (gnu build activation) instead.

bug#63082: [PATCH 07/17] services: mpd: Log to syslog by default.

[Maxim Cournoyer]

Hi,

Bruno Victal <mirai@makinata.eu> writes:

> On 2023-04-28 15:27, Maxim Cournoyer wrote:
>> Rationale: the tristate value was awkward to deal with, the default log file
>> name was odd (/var/log/mpd/log) and it required special attention to create
>> the 'mpd' parent directory as root and chowning it to the MPD user.  It also
>> didn't match the default behavior of MPD, which is to log to systemd or syslog
>> unless a log file is specified.
>> 
>> * gnu/services/audio.scm (mpd-log-file-sanitizer): New procedure.
>> (mpd-configuration) [log-file]: Remove default maybe value.  Add sanitizer.
>> (mpd-shepherd-service): Validate the log file parent directory exists and has
>> the right permissions.
>> * doc/guix.texi (Audio Services): Update doc.
>
> How about a similar approach taken in mympd for handling the logging parameter?
> In any case, I'd like to remind you that mpd-service-type also has a rottlog service extension
> so that also needs to be taken into account.

Done, in yet to be sent v2.

-- 
Thanks,
Maxim

bug#63082: [PATCH 08/17] services: mpd: Only rotate log when a log file is specified.

[Maxim Cournoyer]

This is to avoid adding a bogus log rotation job when syslog is used (which
already has its own log rotation job).

* gnu/services/audio.scm (mpd-log-rotation): Add conditional to avoid
producing a log-rotation object when no log-file was provided.
(mpd-shepherd-service): Do not manage log file parent directory creation.
Assume it already exists.  Adjust the rottlog-service-type extension.
---
 gnu/services/audio.scm | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index a1d1a3d2fe..cccf5c2693 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -554,12 +554,15 @@ (define (mpd-serialize-configuration configuration)
    (serialize-configuration configuration mpd-configuration-fields)))
 
 (define (mpd-log-rotation config)
-  (match-record config <mpd-configuration> (log-file)
-    (log-rotation
-     (files (list log-file))
-     (post-rotate #~(begin
-                      (use-modules (gnu services herd))
-                      (with-shepherd-action 'mpd ('reopen) #f))))))
+  (match-record config <mpd-configuration>
+    (log-file)
+    (if (maybe-value log-file)
+        (list (log-rotation
+               (files (list log-file))
+               (post-rotate #~(begin
+                                (use-modules (gnu services herd))
+                                (with-shepherd-action 'mpd ('reopen) #f)))))
+        '())))
 
 (define (mpd-shepherd-service config)
   (match-record config <mpd-configuration> (user package shepherd-requirement
@@ -635,7 +638,7 @@ (define mpd-service-type
           (service-extension account-service-type
                              mpd-accounts)
           (service-extension rottlog-service-type
-                             (compose list mpd-log-rotation))))
+                             mpd-log-rotation)))
    (default-value (mpd-configuration))))
 
 \f
-- 
2.39.2

bug#63082: [PATCH 09/17] services: mpd: Let Shepherd effect the user/group change.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

Quoting a MPD developer, regarding MPD's feature to switch user itself:
"that's legacy for the dark ages when proper service managers did not exist"
:-).

* gnu/services/audio.scm (mpd-serialize-user-account)
(mpd-serialize-user-group): Delete procedures.
* gnu/services/audio.scm (mpd-configuration) [user]: Do not serialize.
[group]: Likewise.
(mpd-shepherd-service): Provide the #:user, #:group and #:supplementary-groups
arguments.
(mympd-shepherd-service): Likewise, and remove the '--user' argument.
* doc/guix.texi (Audio Services): Decorate mpd with @command.
---
 doc/guix.texi          |  4 ++--
 gnu/services/audio.scm | 31 ++++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index a71a05bcf3..19320c2185 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33569,7 +33569,7 @@ Audio Services
 The MPD package.
 
 @item @code{user} (type: user-account)
-The user to run mpd as.
+The user to run @command{mpd} as.
 
 @item @code{group} (default: @code{#f}) (type: boolean)
 Obsolete.  Do not use.
@@ -33612,7 +33612,7 @@ Audio Services
 The location of the sticker database.
 
 @item @code{default-port} (default: @code{6600}) (type: maybe-port)
-The default port to run mpd on.
+The default port to run @command{mpd} on.
 
 @item @code{endpoints} (type: maybe-list-of-strings)
 The addresses that mpd will bind to.  A port different from
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index cccf5c2693..550ccc542c 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2019 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2022⁠–⁠2023 Bruno Victal <mirai@makinata.eu>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -164,9 +165,6 @@ (define mpd-serialize-boolean mpd-serialize-field)
 (define (mpd-serialize-list-of-strings field-name value)
   #~(string-append #$@(map (cut mpd-serialize-string field-name <>) value)))
 
-(define (mpd-serialize-user-account field-name value)
-  (mpd-serialize-string field-name (user-account-name value)))
-
 (define-maybe string (prefix mpd-))
 (define-maybe list-of-strings (prefix mpd-))
 (define-maybe boolean (prefix mpd-))
@@ -387,10 +385,14 @@ (define-configuration mpd-configuration
    "The MPD package."
    empty-serializer)
 
+  ;; Note: The user and its group are not serialized, otherwise MPD would
+  ;; attempt to switch the user/group itself.  The task of switching the
+  ;; user/group is left to Shepherd instead.
   (user
    (user-account %mpd-user)
-   "The user to run mpd as."
-   (sanitizer mpd-user-sanitizer))
+   "The user to run @command{mpd} as."
+   (sanitizer mpd-user-sanitizer)
+   (serializer empty-serializer))
 
   (group
    (boolean #f)
@@ -454,7 +456,7 @@ (define-configuration mpd-configuration
 
   (default-port
    (maybe-port 6600)
-   "The default port to run mpd on.")
+   "The default port to run @command{mpd} on.")
 
   (endpoints
    maybe-list-of-strings
@@ -595,7 +597,11 @@ (define (mpd-shepherd-service config)
                    (list #$(file-append package "/bin/mpd")
                          "--no-daemon"
                          #$config-file)
-                   #:environment-variables '#$environment-variables)))
+                   #:environment-variables '#$environment-variables
+                   #:user #$username
+                   #:group #$(user-account-group user)
+                   #:supplementary-groups
+                   '#$(user-account-supplementary-groups user))))
        (stop  #~(make-kill-destructor))
        (actions
         (list (shepherd-configuration-action config-file)
@@ -876,12 +882,19 @@ (define (mympd-shepherd-service config)
 
                   (make-forkexec-constructor
                    `(#$(file-append package "/bin/mympd")
-                     "--user" #$username
                      #$@(if (eq? log-to 'syslog) '("--syslog") '())
                      "--workdir" #$work-directory
                      "--cachedir" #$cache-directory)
                    #:environment-variables (list #$log-level*)
-                   #:log-file #$(if (string? log-to) log-to #f))))
+                   #:log-file #$(if (string? log-to) log-to #f)
+                   #:user #$username
+                   ;; Note: the group of the <user-account> record or that of
+                   ;; the <user-group> record can be used interchangeably
+                   ;; here, since they've been synced in the 'mympd-accounts'
+                   ;; procedure.
+                   #:group #$(user-account-group user)
+                   #:supplementary-groups
+                   '#$(user-account-supplementary-groups user))))
        (stop #~(make-kill-destructor))))))
 
 (define (mympd-accounts config)
-- 
2.39.2

bug#63082: [PATCH 09/17] services: mpd: Let Shepherd effect the user/group change.

[Bruno Victal]

On 2023-04-28 15:27, Maxim Cournoyer wrote:
> Relates to <https://issues.guix.gnu.org/63082>.
> 
> Quoting a MPD developer, regarding MPD's feature to switch user itself:
> "that's legacy for the dark ages when proper service managers did not exist"
> :-).
> 
> * gnu/services/audio.scm (mpd-serialize-user-account)
> (mpd-serialize-user-group): Delete procedures.
> * gnu/services/audio.scm (mpd-configuration) [user]: Do not serialize.
> [group]: Likewise.
> (mpd-shepherd-service): Provide the #:user, #:group and #:supplementary-groups
> arguments.
> (mympd-shepherd-service): Likewise, and remove the '--user' argument.
> * doc/guix.texi (Audio Services): Decorate mpd with @command.
> ---
>  doc/guix.texi          |  4 ++--
>  gnu/services/audio.scm | 31 ++++++++++++++++++++++---------
>  2 files changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/doc/guix.texi b/doc/guix.texi
> index a71a05bcf3..19320c2185 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -33569,7 +33569,7 @@ Audio Services
>  The MPD package.
>  
>  @item @code{user} (type: user-account)
> -The user to run mpd as.
> +The user to run @command{mpd} as.
>  
>  @item @code{group} (default: @code{#f}) (type: boolean)
>  Obsolete.  Do not use.
> @@ -33612,7 +33612,7 @@ Audio Services
>  The location of the sticker database.
>  
>  @item @code{default-port} (default: @code{6600}) (type: maybe-port)
> -The default port to run mpd on.
> +The default port to run @command{mpd} on.
>  
>  @item @code{endpoints} (type: maybe-list-of-strings)
>  The addresses that mpd will bind to.  A port different from
> diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
> index cccf5c2693..550ccc542c 100644
> --- a/gnu/services/audio.scm
> +++ b/gnu/services/audio.scm
> @@ -3,6 +3,7 @@
>  ;;; Copyright © 2019 Ricardo Wurmus <rekado@elephly.net>
>  ;;; Copyright © 2020 Ludovic Courtès <ludo@gnu.org>
>  ;;; Copyright © 2022⁠–⁠2023 Bruno Victal <mirai@makinata.eu>
> +;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -164,9 +165,6 @@ (define mpd-serialize-boolean mpd-serialize-field)
>  (define (mpd-serialize-list-of-strings field-name value)
>    #~(string-append #$@(map (cut mpd-serialize-string field-name <>) value)))
>  
> -(define (mpd-serialize-user-account field-name value)
> -  (mpd-serialize-string field-name (user-account-name value)))
> -
>  (define-maybe string (prefix mpd-))
>  (define-maybe list-of-strings (prefix mpd-))
>  (define-maybe boolean (prefix mpd-))
> @@ -387,10 +385,14 @@ (define-configuration mpd-configuration
>     "The MPD package."
>     empty-serializer)
>  
> +  ;; Note: The user and its group are not serialized, otherwise MPD would
> +  ;; attempt to switch the user/group itself.  The task of switching the
> +  ;; user/group is left to Shepherd instead.
>    (user
>     (user-account %mpd-user)
> -   "The user to run mpd as."
> -   (sanitizer mpd-user-sanitizer))
> +   "The user to run @command{mpd} as."
> +   (sanitizer mpd-user-sanitizer)
> +   (serializer empty-serializer))

Simply write empty-serializer after sanitizer instead.

>                       "--cachedir" #$cache-directory)
>                     #:environment-variables (list #$log-level*)
> -                   #:log-file #$(if (string? log-to) log-to #f))))
> +                   #:log-file #$(if (string? log-to) log-to #f)

Generic advice but how about this instead?

--8<---------------cut here---------------start------------->8---
#$@(if (string? log-to) `(#:log-file ,log-to) '())
--8<---------------cut here---------------end--------------->8---

It's cleaner to not explicitly set the keyword argument values when they're not used.

bug#63082: [PATCH 09/17] services: mpd: Let Shepherd effect the user/group change.

[Maxim Cournoyer]

Hi Bruno,

Bruno Victal <mirai@makinata.eu> writes:

[...]

>> +  ;; Note: The user and its group are not serialized, otherwise MPD would
>> +  ;; attempt to switch the user/group itself.  The task of switching the
>> +  ;; user/group is left to Shepherd instead.
>>    (user
>>     (user-account %mpd-user)
>> -   "The user to run mpd as."
>> -   (sanitizer mpd-user-sanitizer))
>> +   "The user to run @command{mpd} as."
>> +   (sanitizer mpd-user-sanitizer)
>> +   (serializer empty-serializer))
>
> Simply write empty-serializer after sanitizer instead.

Done, and for other commits too, though both works and the procedure
version appeared more consistent/readable to my eyes.

>>                       "--cachedir" #$cache-directory)
>>                     #:environment-variables (list #$log-level*)
>> -                   #:log-file #$(if (string? log-to) log-to #f))))
>> +                   #:log-file #$(if (string? log-to) log-to #f)
>
> Generic advice but how about this instead?
>
> #$@(if (string? log-to) `(#:log-file ,log-to) '())
>
> It's cleaner to not explicitly set the keyword argument values when they're not used.

They are used, but set to #f :-).  I have a slight preference for the
current version, which I find more readable.

-- 
Thanks,
Maxim

bug#63082: [PATCH 10/17] system: accounts: Export <user-account>.

[Maxim Cournoyer]

---
 gnu/system/accounts.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gnu/system/accounts.scm b/gnu/system/accounts.scm
index 586cff1842..e37b733c6d 100644
--- a/gnu/system/accounts.scm
+++ b/gnu/system/accounts.scm
@@ -19,7 +19,8 @@
 (define-module (gnu system accounts)
   #:use-module (guix records)
   #:use-module (ice-9 match)
-  #:export (user-account
+  #:export (<user-account>
+            user-account
             user-account?
             user-account-name
             user-account-password
-- 
2.39.2

bug#63082: [PATCH 11/17] services: mpd: Warn when the MPD user is not in the "audio" group.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org>.

* gnu/services/audio.scm (%mpd-user) [group]: Add comment.
(mpd-user-sanitizer): Warn if the MPD user is not in the audio group.
---
 gnu/services/audio.scm | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 550ccc542c..9579432ea3 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -30,6 +30,7 @@ (define-module (gnu services audio)
   #:use-module (gnu services configuration)
   #:use-module (gnu services shepherd)
   #:use-module (gnu services admin)
+  #:use-module (gnu system accounts)
   #:use-module (gnu system shadow)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages mpd)
@@ -172,6 +173,8 @@ (define-maybe boolean (prefix mpd-))
 (define %mpd-user
   (user-account
    (name "mpd")
+   ;; Being in the audio group ensures that PulseAudio can access sound
+   ;; devices.
    (group "audio")
    (system? #t)
    (comment "Music Player Daemon (MPD) user")
@@ -208,10 +211,17 @@ (define (mpd-serialize-port field-name value)
 
 (define-maybe port (prefix mpd-))
 
-;;; Procedures for unsupported value types, to be removed.
-
+;;; Sanitizer procedures.
 (define (mpd-user-sanitizer value)
-  (cond ((user-account? value) value)
+  (cond ((user-account? value)
+         (match-record value <user-account>
+           (group supplementary-groups)
+           (unless (or (string=? "audio" group)
+                       (member "audio" supplementary-groups))
+             ;; Being in the "audio" group is necessary for access to the
+             ;; sound devices.
+             (warning (G_ "mpd user not member of \"audio\" group~%"))))
+         value)
         ((string? value)
          (warning (G_ "string value for 'user' is deprecated, use \
 user-account instead~%"))
-- 
2.39.2

bug#63082: [PATCH 11/17] services: mpd: Warn when the MPD user is not in the "audio" group.

[Liliana Marie Prikler]

Am Freitag, dem 28.04.2023 um 10:27 -0400 schrieb Maxim Cournoyer:
> Relates to <https://issues.guix.gnu.org>.
> 
> * gnu/services/audio.scm (%mpd-user) [group]: Add comment.
> (mpd-user-sanitizer): Warn if the MPD user is not in the audio group.
> ---
>  gnu/services/audio.scm | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
> index 550ccc542c..9579432ea3 100644
> --- a/gnu/services/audio.scm
> +++ b/gnu/services/audio.scm
> @@ -30,6 +30,7 @@ (define-module (gnu services audio)
>    #:use-module (gnu services configuration)
>    #:use-module (gnu services shepherd)
>    #:use-module (gnu services admin)
> +  #:use-module (gnu system accounts)
>    #:use-module (gnu system shadow)
>    #:use-module (gnu packages admin)
>    #:use-module (gnu packages mpd)
> @@ -172,6 +173,8 @@ (define-maybe boolean (prefix mpd-))
>  (define %mpd-user
>    (user-account
>     (name "mpd")
> +   ;; Being in the audio group ensures that PulseAudio can access
> sound
> +   ;; devices.
>     (group "audio")
>     (system? #t)
>     (comment "Music Player Daemon (MPD) user")
> @@ -208,10 +211,17 @@ (define (mpd-serialize-port field-name value)
>  
>  (define-maybe port (prefix mpd-))
>  
> -;;; Procedures for unsupported value types, to be removed.
> -
> +;;; Sanitizer procedures.
>  (define (mpd-user-sanitizer value)
> -  (cond ((user-account? value) value)
> +  (cond ((user-account? value)
> +         (match-record value <user-account>
> +           (group supplementary-groups)
> +           (unless (or (string=? "audio" group)
> +                       (member "audio" supplementary-groups))
> +             ;; Being in the "audio" group is necessary for access
> to the
> +             ;; sound devices.
> +             (warning (G_ "mpd user not member of \"audio\"
> group~%"))))
> +         value)
>          ((string? value)
>           (warning (G_ "string value for 'user' is deprecated, use \
>  user-account instead~%"))
I think this check is "only" required when using alsa/pulseaudio for
outputs and should be a hard error then.  When configured to write to
httpd or null outputs, other checks are needed.

Cheers

bug#63082: [PATCH 11/17] services: mpd: Warn when the MPD user is not in the "audio" group.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Freitag, dem 28.04.2023 um 10:27 -0400 schrieb Maxim Cournoyer:
>> Relates to <https://issues.guix.gnu.org>.
>> 
>> * gnu/services/audio.scm (%mpd-user) [group]: Add comment.
>> (mpd-user-sanitizer): Warn if the MPD user is not in the audio group.
>> ---
>>  gnu/services/audio.scm | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>> 
>> diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
>> index 550ccc542c..9579432ea3 100644
>> --- a/gnu/services/audio.scm
>> +++ b/gnu/services/audio.scm
>> @@ -30,6 +30,7 @@ (define-module (gnu services audio)
>>    #:use-module (gnu services configuration)
>>    #:use-module (gnu services shepherd)
>>    #:use-module (gnu services admin)
>> +  #:use-module (gnu system accounts)
>>    #:use-module (gnu system shadow)
>>    #:use-module (gnu packages admin)
>>    #:use-module (gnu packages mpd)
>> @@ -172,6 +173,8 @@ (define-maybe boolean (prefix mpd-))
>>  (define %mpd-user
>>    (user-account
>>     (name "mpd")
>> +   ;; Being in the audio group ensures that PulseAudio can access
>> sound
>> +   ;; devices.
>>     (group "audio")
>>     (system? #t)
>>     (comment "Music Player Daemon (MPD) user")
>> @@ -208,10 +211,17 @@ (define (mpd-serialize-port field-name value)
>>  
>>  (define-maybe port (prefix mpd-))
>>  
>> -;;; Procedures for unsupported value types, to be removed.
>> -
>> +;;; Sanitizer procedures.
>>  (define (mpd-user-sanitizer value)
>> -  (cond ((user-account? value) value)
>> +  (cond ((user-account? value)
>> +         (match-record value <user-account>
>> +           (group supplementary-groups)
>> +           (unless (or (string=? "audio" group)
>> +                       (member "audio" supplementary-groups))
>> +             ;; Being in the "audio" group is necessary for access
>> to the
>> +             ;; sound devices.
>> +             (warning (G_ "mpd user not member of \"audio\"
>> group~%"))))
>> +         value)
>>          ((string? value)
>>           (warning (G_ "string value for 'user' is deprecated, use \
>>  user-account instead~%"))
> I think this check is "only" required when using alsa/pulseaudio for
> outputs and should be a hard error then.  When configured to write to
> httpd or null outputs, other checks are needed.

I agree, but then the check couldn't be made in a sanitizer and would
need to happen much later (in the start slot to ensure it runs at the
right time?).

I figured the current behavior, while not perfect, is better than the
later.

-- 
Thanks,
Maxim

bug#63082: [PATCH 12/17] services: mpd: Auto-detect mpd-output mixer type by default.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

* gnu/services/audio.scm (mpd-output) [mixer-type]: Change default value from
"none" to unspecified.
* doc/guix.texi (Audio Services): Regenerate doc.
---
 doc/guix.texi          | 11 +++++++----
 gnu/services/audio.scm | 15 +++++++++------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 19320c2185..550e6606e5 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33709,8 +33709,9 @@ Audio Services
 @end table
 @end deftp
 
+@c %start of fragment
 @deftp {Data Type} mpd-output
-Data type representing a @command{mpd} audio output.
+Available @code{mpd-output} fields are:
 
 @table @asis
 @item @code{name} (default: @code{"MPD"}) (type: string)
@@ -33737,15 +33738,16 @@ Audio Services
 
 @item @code{always-on?} (default: @code{#f}) (type: boolean)
 If set to @code{#t}, then MPD attempts to keep this audio output always
-open.  This may be useful for streaming servers, when you don?t want to
+open.  This may be useful for streaming servers, when you don’t want to
 disconnect all listeners even when playback is accidentally stopped.
 
-@item @code{mixer-type} (default: @code{"none"}) (type: string)
+@item @code{mixer-type} (type: maybe-string)
 This field accepts a string that specifies which mixer should be used
 for this audio output: the @code{hardware} mixer, the @code{software}
 mixer, the @code{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 (@code{none}).
+External Mixer) or no mixer (@code{none}).  When left unspecified, a
+@code{hardware} mixer is used for devices that support it.
 
 @item @code{replay-gain-handler} (type: maybe-string)
 This field accepts a string that specifies how
@@ -33760,6 +33762,7 @@ Audio Services
 
 @end table
 @end deftp
+@c %end of fragment
 
 The following example shows a configuration of @command{mpd} that
 configures some of its plugins and provides a HTTP audio streaming output.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 9579432ea3..071cebcef4 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -343,15 +343,18 @@ (define-configuration mpd-output
 disconnect all listeners even when playback is accidentally stopped.")
 
   (mixer-type
-   (string "none")
-   "This field accepts a string that specifies which mixer should be used
-for this audio output: the @code{hardware} mixer, the @code{software}
-mixer, the @code{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 (@code{none})."
+   maybe-string
+   "This field accepts a string that specifies which mixer should be used for
+this audio output: the @code{hardware} mixer, the @code{software} mixer, the
+@code{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 (@code{none}).  When left unspecified, a @code{hardware} mixer is used
+for devices that support it."
    (sanitizer
     (lambda (x)  ; TODO: deprecated, remove me later.
       (cond
+       ((eq? %unset-value x)
+        x)
        ((symbol? x)
         (warning (G_ "symbol value for 'mixer-type' is deprecated, \
 use string instead~%"))
-- 
2.39.2

bug#63082: [PATCH 13/17] services: mpd: Fix indentation.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-log-rotation): Fix indentation.
---
 gnu/services/audio.scm | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 071cebcef4..c11a7cfd26 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -580,10 +580,11 @@ (define (mpd-log-rotation config)
         '())))
 
 (define (mpd-shepherd-service config)
-  (match-record config <mpd-configuration> (user package shepherd-requirement
-                                            log-file playlist-directory
-                                            db-file state-file sticker-file
-                                            environment-variables)
+  (match-record config <mpd-configuration>
+    (user package shepherd-requirement
+          log-file playlist-directory
+          db-file state-file sticker-file
+          environment-variables)
     (let ((config-file (mpd-serialize-configuration config))
           (username (user-account-name user)))
       (shepherd-service
-- 
2.39.2

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

[Maxim Cournoyer]

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.
---
 doc/guix.texi          |  4 ++--
 gnu/services/audio.scm | 19 ++++++++++++++-----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 550e6606e5..23f3070f39 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33578,8 +33578,8 @@ Audio Services
 This is a list of symbols naming Shepherd services that this service
 will depend on.
 
-@item @code{environment-variables} (default: @code{("PULSE_CLIENTCONFIG=/etc/pulse/client.conf" "PULSE_CONFIG=/etc/pulse/daemon.conf")}) (type: list-of-strings)
-A list of strings specifying environment variables.
+@item @code{environment-variables} (default: @code{#f}) (type: boolean)
+Obsolete.  Do not use.
 
 @item @code{log-file} (type: maybe-string)
 The location of the log file.  Unless specified, the logs are collected
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index c11a7cfd26..f0587b9106 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -236,6 +236,12 @@ (define (mpd-group-sanitizer value)
     (warning (G_ "'group' in <mpd-configuration> is obsolete; ignoring~%")))
   #f)
 
+(define (mpd-environment-variables-sanitizer value)
+  (when value
+    (warning (G_ "'environment-variables' in <mpd-configuration> is obsolete;\
+ ignoring~%")))
+  #f)
+
 (define (mpd-log-file-sanitizer value)
   (match value
     (%unset-value
@@ -420,10 +426,10 @@ (define-configuration mpd-configuration
    empty-serializer)
 
   (environment-variables
-   (list-of-strings '("PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
-                      "PULSE_CONFIG=/etc/pulse/daemon.conf"))
-   "A list of strings specifying environment variables."
-   empty-serializer)
+   (boolean #f)
+   "Obsolete.  Do not use."
+   (sanitizer mpd-environment-variables-sanitizer)
+   (serializer empty-serializer))
 
   (log-file
    maybe-string
@@ -611,7 +617,10 @@ (define (mpd-shepherd-service config)
                    (list #$(file-append package "/bin/mpd")
                          "--no-daemon"
                          #$config-file)
-                   #:environment-variables '#$environment-variables
+                   #:environment-variables
+                   ;; Use the system-configured pulse configuration.
+                   (list "PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
+                         "PULSE_CONFIG=/etc/pulse/daemon.conf")
                    #:user #$username
                    #:group #$(user-account-group user)
                    #:supplementary-groups
-- 
2.39.2

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

[Bruno Victal]

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.

I'd consider this field to be within the same category as 'shepherd-requirement', it's for flexibility.


>  doc/guix.texi          |  4 ++--
>  gnu/services/audio.scm | 19 ++++++++++++++-----
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 550e6606e5..23f3070f39 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -33578,8 +33578,8 @@ Audio Services
>  This is a list of symbols naming Shepherd services that this service
>  will depend on.
>  
> -@item @code{environment-variables} (default: @code{("PULSE_CLIENTCONFIG=/etc/pulse/client.conf" "PULSE_CONFIG=/etc/pulse/daemon.conf")}) (type: list-of-strings)
> -A list of strings specifying environment variables.
> +@item @code{environment-variables} (default: @code{#f}) (type: boolean)
> +Obsolete.  Do not use.
>  
>  @item @code{log-file} (type: maybe-string)
>  The location of the log file.  Unless specified, the logs are collected
> diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
> index c11a7cfd26..f0587b9106 100644
> --- a/gnu/services/audio.scm
> +++ b/gnu/services/audio.scm
> @@ -236,6 +236,12 @@ (define (mpd-group-sanitizer value)
>      (warning (G_ "'group' in <mpd-configuration> is obsolete; ignoring~%")))
>    #f)
>  
> +(define (mpd-environment-variables-sanitizer value)
> +  (when value
> +    (warning (G_ "'environment-variables' in <mpd-configuration> is obsolete;\
> + ignoring~%")))
> +  #f)
> +
>  (define (mpd-log-file-sanitizer value)
>    (match value
>      (%unset-value
> @@ -420,10 +426,10 @@ (define-configuration mpd-configuration
>     empty-serializer)
>  
>    (environment-variables
> -   (list-of-strings '("PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
> -                      "PULSE_CONFIG=/etc/pulse/daemon.conf"))
> -   "A list of strings specifying environment variables."
> -   empty-serializer)
> +   (boolean #f)
> +   "Obsolete.  Do not use."
> +   (sanitizer mpd-environment-variables-sanitizer)
> +   (serializer empty-serializer))
>  
>    (log-file
>     maybe-string
> @@ -611,7 +617,10 @@ (define (mpd-shepherd-service config)
>                     (list #$(file-append package "/bin/mpd")
>                           "--no-daemon"
>                           #$config-file)
> -                   #:environment-variables '#$environment-variables
> +                   #:environment-variables
> +                   ;; Use the system-configured pulse configuration.
> +                   (list "PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
> +                         "PULSE_CONFIG=/etc/pulse/daemon.conf")
>                     #:user #$username
>                     #:group #$(user-account-group user)
>                     #:supplementary-groups

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

[Maxim Cournoyer]

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).

> 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?

-- 
Thanks,
Maxim

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.

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

[Maxim Cournoyer]

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'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.

I've yet to look carefully into SRFI-136 would provide us with, but it
seems an interesting direction, to the sound of it.

-- 
Thanks,
Maxim

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

[Bruno Victal]

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:

* <https://www.freedesktop.org/wiki/Software/PulseAudio/FAQ/#whatenvironmentvariablesdoespulseaudiocareabout>
* <https://docs.pipewire.org/page_daemon.html>


Cheers,
Bruno

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

bug#63082: [PATCH 15/17] services: mpd: Provision a default cache directory and set HOME.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

* gnu/services/audio.scm (mpd-shepherd-service): Create a default .cache
directory.  Use mkdir-p/perms and refactor loop.  Set the HOME environment
variables.
---
 doc/guix.texi          |  3 +-
 gnu/services/audio.scm | 76 ++++++++++++++++++++++++++----------------
 2 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 23f3070f39..9be59f9f02 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33603,7 +33603,8 @@ Audio Services
 The directory to store playlists.
 
 @item @code{db-file} (type: maybe-string)
-The location of the music database.
+The location of the music database.  When left unspecified,
+@file{~/.cache/db} is used.
 
 @item @code{state-file} (type: maybe-string)
 The location of the file that stores current MPD's state.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index f0587b9106..7c577ff73b 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -25,6 +25,7 @@ (define-module (gnu services audio)
   #:use-module (guix deprecation)
   #:use-module (guix diagnostics)
   #:use-module (guix i18n)
+  #:use-module (guix modules)
   #:use-module (gnu services)
   #:use-module (gnu services admin)
   #:use-module (gnu services configuration)
@@ -463,7 +464,8 @@ (define-configuration mpd-configuration
 
   (db-file
    maybe-string
-   "The location of the music database.")
+   "The location of the music database.  When left unspecified,
+@file{~/.cache/db} is used.")
 
   (state-file
    maybe-string
@@ -597,34 +599,50 @@ (define (mpd-shepherd-service config)
        (documentation "Run the MPD (Music Player Daemon)")
        (requirement `(user-processes loopback ,@shepherd-requirement))
        (provision '(mpd))
-       (start #~(begin
-                  (let ((user (getpw #$username)))
-                    (for-each
-                     (lambda (x)
-                       ;; Take action on absolute file names, to filter out
-                       ;; the 'syslog' special value.
-                       (when (and x (string-prefix? "/" x)
-                                  (not (file-exists? x)))
-                         (mkdir-p x)
-                         (chown x (passwd:uid user) (passwd:gid user))))
-                     (list #$(maybe-value playlist-directory)
-                           (and=> #$(maybe-value db-file) dirname)
-                           (and=> #$(maybe-value log-file) dirname)
-                           (and=> #$(maybe-value state-file) dirname)
-                           (and=> #$(maybe-value sticker-file) dirname))))
-
-                  (make-forkexec-constructor
-                   (list #$(file-append package "/bin/mpd")
-                         "--no-daemon"
-                         #$config-file)
-                   #:environment-variables
-                   ;; Use the system-configured pulse configuration.
-                   (list "PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
-                         "PULSE_CONFIG=/etc/pulse/daemon.conf")
-                   #:user #$username
-                   #:group #$(user-account-group user)
-                   #:supplementary-groups
-                   '#$(user-account-supplementary-groups user))))
+       (start
+        (with-imported-modules (source-module-closure
+                                '((gnu build activation)))
+          #~(begin
+              (use-modules (gnu build activation))
+
+              (let ((home #$(user-account-home-directory user)))
+                (let ((user (getpw #$username))
+                      (default-cache-dir (string-append home "/.cache")))
+
+                  (define (init-directory directory)
+                    (unless (file-exists? directory)
+                      (mkdir-p/perms directory user #o755)))
+
+                  ;; Define a cache location that can be automatically used
+                  ;; for the database file, in case it hasn't been explicitly
+                  ;; specified.
+                  (for-each
+                   init-directory
+                   (cons default-cache-dir
+                         '#$(map dirname
+                                 ;; XXX: Delete the potential "syslog"
+                                 ;; log-file value, which is not a directory.
+                                 (delete "syslog"
+                                         (filter-map maybe-value
+                                                     (list db-file
+                                                           log-file
+                                                           state-file
+                                                           sticker-file)))))))
+
+                (make-forkexec-constructor
+                 (list #$(file-append package "/bin/mpd") "--no-daemon"
+                       #$config-file)
+                 #:environment-variables
+                 ;; Use the system-configured pulse configuration.  Set HOME
+                 ;; so MPD can infer default paths, such as for the database
+                 ;; file.
+                 (list (string-append "HOME=" home)
+                       "PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
+                       "PULSE_CONFIG=/etc/pulse/daemon.conf")
+                 #:user #$username
+                 #:group #$(user-account-group user)
+                 #:supplementary-groups
+                 '#$(user-account-supplementary-groups user))))))
        (stop  #~(make-kill-destructor))
        (actions
         (list (shepherd-configuration-action config-file)
-- 
2.39.2

bug#63082: [PATCH 15/17] services: mpd: Provision a default cache directory and set HOME.

[Bruno Victal]

On 2023-04-28 15:27, Maxim Cournoyer wrote:
> Relates to <https://issues.guix.gnu.org/63082>.
> 
> * gnu/services/audio.scm (mpd-shepherd-service): Create a default .cache
> directory.  Use mkdir-p/perms and refactor loop.  Set the HOME environment
> variables.

There's a slight problem here, you might really not want db_file to be set _at all_.
Case in point, I use a database-plugin instead which would be incongruent if db_file is forcibly serialized into the config.

Perhaps we could add some kind of warning mechanism that alerts the user that either db_file or database-plugin must be configured?

bug#63082: [PATCH 15/17] services: mpd: Provision a default cache directory and set HOME.

[Maxim Cournoyer]

Hi Bruno,

Bruno Victal <mirai@makinata.eu> writes:

> On 2023-04-28 15:27, Maxim Cournoyer wrote:
>> Relates to <https://issues.guix.gnu.org/63082>.
>> 
>> * gnu/services/audio.scm (mpd-shepherd-service): Create a default .cache
>> directory.  Use mkdir-p/perms and refactor loop.  Set the HOME environment
>> variables.
>
> There's a slight problem here, you might really not want db_file to be set _at all_.
> Case in point, I use a database-plugin instead which would be
> incongruent if db_file is forcibly serialized into the config.
>
> Perhaps we could add some kind of warning mechanism that alerts the
> user that either db_file or database-plugin must be configured?

I believe there's no problem; I don't force a value to db_file, so the
serialized configuration doesn't have any.

MPD has a mechanism that scans the MPD user directory for XDG locations
such as ~/.cache; when such directories exist it supports automatically
figuring out a location for the database file and creating it itself.
If the user has specified something precise, it'd be a bug in MPD that
it doesn't honor these settings instead.

Does it clarify the mechanism?

-- 
Thanks,
Maxim

bug#63082: [PATCH 16/17] services: mpd: Update basic example.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

* doc/guix.texi (Audio Services): Do not use a deprecated user form; keep the
default one.  Remove port.  Specify a music-directory.  Mention the importance
of permissions on the music directory.
---
 doc/guix.texi | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 9be59f9f02..467870e0c4 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33536,16 +33536,27 @@ Audio Services
 being controlled from the local machine or over the network by a variety
 of clients.
 
-The following example shows how one might run @code{mpd} as user
-@code{"bob"} on port @code{6666}.  It uses pulseaudio for output.
+The following example shows the simplest configuration to locally
+expose, via PulseAudio, a music collection kept at @file{/srv/music},
+with @command{mpd} running as the default @samp{mpd} user.  This user
+will spawn its own PulseAudio daemon, which may compete for the sound
+card access with that of your own user.  In this configuration, you may
+have to stop the playback of your user audio applications to hear MPD's
+output and vice-versa.
 
 @lisp
 (service mpd-service-type
          (mpd-configuration
-          (user "bob")
-          (port "6666")))
+          (music-directory "/srv/music")))
 @end lisp
 
+@quotation Important
+The music directory must be readable to the MPD user, by default,
+@samp{mpd}.  Permission problems will be reported via @samp{Permission
+denied} errors in the MPD logs, which appear in @file{/var/log/messages}
+by default.
+@end quotation
+
 Most MPD clients will trigger a database update upon connecting, but you
 can also use the @code{update} action do to so:
 
-- 
2.39.2

bug#63082: [PATCH 17/17] services: Avoid 'delete' overrides warning in audio module.

[Maxim Cournoyer]

* gnu/services/audio.scm: Hide 'delete' on (gnu services) import.
---
 gnu/services/audio.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 7c577ff73b..40db31335e 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -26,7 +26,7 @@ (define-module (gnu services audio)
   #:use-module (guix diagnostics)
   #:use-module (guix i18n)
   #:use-module (guix modules)
-  #:use-module (gnu services)
+  #:use-module ((gnu services) #:hide (delete))
   #:use-module (gnu services admin)
   #:use-module (gnu services configuration)
   #:use-module (gnu services shepherd)
-- 
2.39.2

bug#63082: [PATCH v2 00/16] Improve out-of-the-box experience with mpd-service-type

[Maxim Cournoyer]

This series harmonize mympd with mypd further, such as using syslog as default
for the log output, and sharing more of the logic for their start slot.
Otherwise it's the same as the previous series, with some clean ups and
changes suggested by Bruno in its review (thanks!).

Maxim Cournoyer (16):
  services: mpd: Add an 'update' action to trigger a database update.
  services: mpd: Streamline mpd-user-sanitizer and mympd-user-sanitizer.
  services: mpd: Rename %set-user-group to set-user-group.
  services: mpd: Obsolete the 'group' field.
  services: mpd: List log-level in decreasing verbosity order in doc.
  services: mpd; Refactor start slot directory initialization.
  services: mpd: Log to syslog by default.
  services: mpd: Do not rotate logs when using syslog.
  services: mpd: Let Shepherd effect the user/group change.
  system: accounts: Export <user-account>.
  services: mpd: Warn when the MPD user is not in the "audio" group.
  services: mpd: Auto-detect mpd-output mixer type by default.
  services: mpd: Obsolete 'environment-variables' field.
  services: mpd: Provision a default cache directory and set HOME.
  services: mpd: Update basic example.
  services: Avoid 'delete' overrides warning in audio module.

 doc/guix.texi           |  97 ++++++----
 gnu/services/audio.scm  | 405 +++++++++++++++++++++++-----------------
 gnu/system/accounts.scm |   3 +-
 gnu/tests/audio.scm     |   4 +-
 4 files changed, 294 insertions(+), 215 deletions(-)


base-commit: ccf64b6a8b8718a8bb69719cf9ed2873464e3850
prerequisite-patch-id: eace011dd080f709a8eeb77c7a739f87079dbb81
prerequisite-patch-id: fd596ecff861483a486910ca0feecded27f6a4a2
prerequisite-patch-id: 948c73edc0a8a0a21b1d4f6878d3f09158059f38
prerequisite-patch-id: becfd217e53934fe9ef16939ff433e5ed00a4b1e
prerequisite-patch-id: 20f12e01af25b881e362ea7dc837a07aeec8a489
prerequisite-patch-id: 7534d08dbc06589ce0fe7bd306585321cb5385d1
-- 
2.39.2

bug#63082: [PATCH v2 01/16] services: mpd: Add an 'update' action to trigger a database update.

[Maxim Cournoyer]

* 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(+)

diff --git a/doc/guix.texi b/doc/guix.texi
index bdacb56af5..f8acdbd6b5 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33546,6 +33546,16 @@ Audio Services
           (port "6666")))
 @end lisp
 
+Most MPD clients will trigger a database update upon connecting, but you
+can also use the @code{update} action do to so:
+
+@example
+herd update mpd
+@end example
+
+All the MPD configuration fields are documented below, and a more
+complex example follows.
+
 @defvar mpd-service-type
 The service type for @command{mpd}
 @end defvar
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 8c061da47f..6e4ce3f9fb 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -620,6 +620,17 @@ (define (mpd-shepherd-service config)
                           (format #t
                                   "Issued SIGHUP to Service MPD (PID ~a)."
                                   pid))
+                        (format #t "Service MPD is not running.")))))
+              (shepherd-action
+               (name 'update)
+               (documentation "Request MPD to update its music database.")
+               (procedure
+                #~(lambda (pid)
+                    (if pid
+                        (begin
+                          (invoke #$(file-append mpd-mpc "/bin/mpc") "update")
+                          (format #t "Database update requested for service \
+MPD (PID ~a)." pid))
                         (format #t "Service MPD is not running.")))))))))))
 
 (define (mpd-accounts config)
-- 
2.39.2

bug#63082: [PATCH v2 02/16] services: mpd: Streamline mpd-user-sanitizer and mympd-user-sanitizer.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-user-sanitizer, %mympd-user): Remove extraneous
group field, already inherited.
(%mpd-user, %mympd-user): Clarify %lazy-group explanatory comment.  Fix
indentation.
---
 gnu/services/audio.scm | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 6e4ce3f9fb..dc83479e40 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -184,13 +184,15 @@ (define-maybe boolean (prefix mpd-))
 
 (define %mpd-user
   (user-account
-      (name "mpd")
-      (group %lazy-group)
-      (system? #t)
-      (comment "Music Player Daemon (MPD) user")
-      ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data
-      (home-directory "/var/lib/mpd")
-      (shell (file-append shadow "/sbin/nologin"))))
+   (name "mpd")
+   ;; XXX: This is a place-holder to be lazily substituted in (…-accounts)
+   ;; with the value from the 'group' field of <mpd-configuration>.
+   (group %lazy-group)
+   (system? #t)
+   (comment "Music Player Daemon (MPD) user")
+   ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data.
+   (home-directory "/var/lib/mpd")
+   (shell (file-append shadow "/sbin/nologin"))))
 
 (define %mpd-group
   (user-group
@@ -235,10 +237,7 @@ (define (mpd-user-sanitizer value)
 user-account instead~%"))
          (user-account
           (inherit %mpd-user)
-          (name value)
-          ;; XXX: This is to be lazily substituted in (…-accounts)
-          ;; with the value from 'group'.
-          (group %lazy-group)))
+          (name value)))
         (else
          (configuration-field-error #f 'user value))))
 
@@ -676,12 +675,14 @@ (define-maybe/no-serialization mympd-ip-acl)
 
 (define %mympd-user
   (user-account
-      (name "mympd")
-      (group %lazy-group)
-      (system? #t)
-      (comment "myMPD user")
-      (home-directory "/var/empty")
-      (shell (file-append shadow "/sbin/nologin"))))
+   (name "mympd")
+   ;; XXX: This is a place-holder to be lazily substituted in 'mympd-accounts'
+   ;; with the value from the 'group' field of <mympd-configuration>.
+   (group %lazy-group)
+   (system? #t)
+   (comment "myMPD user")
+   (home-directory "/var/empty")
+   (shell (file-append shadow "/sbin/nologin"))))
 
 (define %mympd-group
   (user-group
@@ -696,10 +697,7 @@ (define (mympd-user-sanitizer value)
 user-account instead~%"))
          (user-account
           (inherit %mympd-user)
-          (name value)
-          ;; XXX: this is to be lazily substituted in (…-accounts)
-          ;; with the value from 'group'.
-          (group %lazy-group)))
+          (name value)))
         (else
          (configuration-field-error #f 'user value))))
 
-- 
2.39.2

bug#63082: [PATCH v2 03/16] services: mpd: Rename %set-user-group to set-user-group.

[Maxim Cournoyer]

The convention to use % as a prefix is for "special" variables rather than
procedures.

* gnu/services/audio.scm ((%set-user-group): Rename to...
(set-user-group): ... this.
---
 gnu/services/audio.scm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index dc83479e40..7874539810 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -143,7 +143,7 @@ (define list-of-symbol?
 ;; Helpers for deprecated field types, to be removed later.
 (define %lazy-group (make-symbol "%lazy-group"))
 
-(define (%set-user-group user group)
+(define (set-user-group user group)
   (user-account
    (inherit user)
    (group (user-group-name group))))
@@ -636,7 +636,7 @@ (define (mpd-accounts config)
   (match-record config <mpd-configuration> (user group)
     ;; TODO: Deprecation code, to be removed.
     (let ((user (if (eq? (user-account-group user) %lazy-group)
-                    (%set-user-group user group)
+                    (set-user-group user group)
                     user)))
       (list user group))))
 
@@ -907,7 +907,7 @@ (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)
+                    (set-user-group user group)
                     user)))
       (list user group))))
 
-- 
2.39.2

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

[Maxim Cournoyer]

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".
(%mympd-group): Delete variable.
* doc/guix.texi: Regenerate doc.
---
 doc/guix.texi          | 15 +++-----
 gnu/services/audio.scm | 83 +++++++++++-------------------------------
 2 files changed, 27 insertions(+), 71 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.
 
-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.
 
 @item @code{ssl?} (default: @code{#f}) (type: boolean)
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 7874539810..60387272fc 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -140,14 +140,6 @@ (define (uglify-field-name field-name)
 (define list-of-symbol?
   (list-of symbol?))
 
-;; Helpers for deprecated field types, to be removed later.
-(define %lazy-group (make-symbol "%lazy-group"))
-
-(define (set-user-group user group)
-  (user-account
-   (inherit user)
-   (group (user-group-name group))))
-
 \f
 ;;;
 ;;; MPD
@@ -175,9 +167,6 @@ (define (mpd-serialize-list-of-strings field-name value)
 (define (mpd-serialize-user-account field-name value)
   (mpd-serialize-string field-name (user-account-name value)))
 
-(define (mpd-serialize-user-group field-name value)
-  (mpd-serialize-string field-name (user-group-name value)))
-
 (define-maybe string (prefix mpd-))
 (define-maybe list-of-strings (prefix mpd-))
 (define-maybe boolean (prefix mpd-))
@@ -185,20 +174,13 @@ (define-maybe boolean (prefix mpd-))
 (define %mpd-user
   (user-account
    (name "mpd")
-   ;; XXX: This is a place-holder to be lazily substituted in (…-accounts)
-   ;; with the value from the 'group' field of <mpd-configuration>.
-   (group %lazy-group)
+   (group "audio")
    (system? #t)
    (comment "Music Player Daemon (MPD) user")
    ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data.
    (home-directory "/var/lib/mpd")
    (shell (file-append shadow "/sbin/nologin"))))
 
-(define %mpd-group
-  (user-group
-   (name "mpd")
-   (system? #t)))
-
 ;;; TODO: Procedures for deprecated fields, to be removed.
 
 (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)
 
 ;;;
 
@@ -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)
+   empty-serializer)
 
   (shepherd-requirement
    (list-of-symbol '())
@@ -633,12 +610,9 @@ (define (mpd-shepherd-service config)
                         (format #t "Service MPD is not running.")))))))))))
 
 (define (mpd-accounts config)
-  (match-record config <mpd-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 <mpd-configuration>
+    (user)
+    (list user)))
 
 (define mpd-service-type
   (service-type
@@ -676,19 +650,12 @@ (define-maybe/no-serialization mympd-ip-acl)
 (define %mympd-user
   (user-account
    (name "mympd")
-   ;; XXX: This is a place-holder to be lazily substituted in 'mympd-accounts'
-   ;; with the value from the 'group' field of <mympd-configuration>.
-   (group %lazy-group)
+   (group "nogroup")
    (system? #t)
    (comment "myMPD user")
    (home-directory "/var/empty")
    (shell (file-append shadow "/sbin/nologin"))))
 
-(define %mympd-group
-  (user-group
-   (name "mympd")
-   (system? #t)))
-
 ;;; TODO: Procedures for unsupported value types, to be removed.
 (define (mympd-user-sanitizer value)
   (cond ((user-account? value) value)
@@ -702,15 +669,10 @@ (define (mympd-user-sanitizer value)
          (configuration-field-error #f 'user value))))
 
 (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)
+
 ;;;
 
 
@@ -737,8 +699,8 @@ (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)
 
@@ -904,12 +866,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)))
 
 (define (mympd-log-rotation config)
   (match-record config <mympd-configuration> (log-to)
-- 
2.39.2

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

[Bruno Victal]

Hi Maxim,

On 2023-04-29 18:21, Maxim Cournoyer wrote:
> @@ -185,20 +174,13 @@ (define-maybe boolean (prefix mpd-))
>  (define %mpd-user
>    (user-account
>     (name "mpd")
> -   ;; XXX: This is a place-holder to be lazily substituted in (…-accounts)
> -   ;; with the value from the 'group' field of <mpd-configuration>.
> -   (group %lazy-group)
> +   (group "audio")

Perhaps the group should be set to "mpd", with "audio" added to supplementary-groups instead?


Cheers,
Bruno

bug#63082: mpd defaul configuration does not work ('No database' error)

[Maxim Cournoyer]

Hi Bruno,

Bruno Victal <mirai@makinata.eu> writes:

> Hi Maxim,
>
> On 2023-04-29 18:21, Maxim Cournoyer wrote:
>> @@ -185,20 +174,13 @@ (define-maybe boolean (prefix mpd-))
>>  (define %mpd-user
>>    (user-account
>>     (name "mpd")
>> -   ;; XXX: This is a place-holder to be lazily substituted in (…-accounts)
>> -   ;; with the value from the 'group' field of <mpd-configuration>.
>> -   (group %lazy-group)
>> +   (group "audio")
>
> Perhaps the group should be set to "mpd", with "audio" added to supplementary-groups instead?

I agree it'd be better, but I'd prefer to defer this change until we get
around to supporting <user-group> objects for the group field of
<user-account>, at which point we won't need to add fragile ad-hoc
complications here.

Does this sound reasonable?

-- 
Thanks,
Maxim

bug#63082: [PATCH v2 05/16] services: mpd: List log-level in decreasing verbosity order in doc.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-configuration) [log-level]: List log-level in
decreasing verbosity order in doc.
* doc/guix.texi (Audio Services): Update doc.
---
 doc/guix.texi          | 6 +++---
 gnu/services/audio.scm | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 34703b1698..1aa8dc2809 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33587,9 +33587,9 @@ Audio Services
 configuration file.
 
 @item @code{log-level} (type: maybe-string)
-Supress any messages below this threshold.  Available values:
-@code{notice}, @code{info}, @code{verbose}, @code{warning} and
-@code{error}.
+Supress any messages below this threshold.  The available values, in
+decreasing order of verbosity, are: @code{verbose}, @code{info},
+@code{notice}, @code{warning} and @code{error}.
 
 @item @code{music-directory} (type: maybe-string)
 The directory to scan for music files.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 60387272fc..ead4cb8d90 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -409,8 +409,8 @@ (define-configuration mpd-configuration
   (log-level
    maybe-string
    "Supress any messages below this threshold.
-Available values: @code{notice}, @code{info}, @code{verbose},
-@code{warning} and @code{error}.")
+The available values, in decreasing order of verbosity, are: @code{verbose},
+@code{info}, @code{notice}, @code{warning} and @code{error}.")
 
   (music-directory
    maybe-string
-- 
2.39.2

bug#63082: [PATCH v2 06/16] services: mpd; Refactor start slot directory initialization.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-shepherd-service): Standardize the way the log
file parent and other directories are initialized in the start slot.
(mympd-shepherd-service): Likewise.
---
 gnu/services/audio.scm | 132 ++++++++++++++++++++++++-----------------
 1 file changed, 77 insertions(+), 55 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index ead4cb8d90..6e57bf5cba 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -24,6 +24,7 @@ (define-module (gnu services audio)
   #:use-module (guix deprecation)
   #:use-module (guix diagnostics)
   #:use-module (guix i18n)
+  #:use-module (guix modules)
   #:use-module (gnu services)
   #:use-module (gnu services admin)
   #:use-module (gnu services configuration)
@@ -552,36 +553,45 @@ (define (mpd-log-rotation config)
                       (with-shepherd-action 'mpd ('reopen) #f))))))
 
 (define (mpd-shepherd-service config)
-  (match-record config <mpd-configuration> (user package shepherd-requirement
-                                            log-file playlist-directory
-                                            db-file state-file sticker-file
-                                            environment-variables)
+  (match-record config <mpd-configuration>
+    (user package shepherd-requirement
+          log-file playlist-directory
+          db-file state-file sticker-file
+          environment-variables)
     (let ((config-file (mpd-serialize-configuration config))
           (username (user-account-name user)))
       (shepherd-service
        (documentation "Run the MPD (Music Player Daemon)")
        (requirement `(user-processes loopback ,@shepherd-requirement))
        (provision '(mpd))
-       (start #~(begin
-                  (and=> #$(maybe-value log-file)
-                         (compose mkdir-p dirname))
-
-                  (let ((user (getpw #$username)))
-                    (for-each
-                     (lambda (x)
-                       (when (and x (not (file-exists? x)))
-                         (mkdir-p x)
-                         (chown x (passwd:uid user) (passwd:gid user))))
-                     (list #$(maybe-value playlist-directory)
-                           (and=> #$(maybe-value db-file) dirname)
-                           (and=> #$(maybe-value state-file) dirname)
-                           (and=> #$(maybe-value sticker-file) dirname))))
-
-                  (make-forkexec-constructor
-                   (list #$(file-append package "/bin/mpd")
-                         "--no-daemon"
-                         #$config-file)
-                   #:environment-variables '#$environment-variables)))
+       (start
+        (with-imported-modules (source-module-closure
+                                '((gnu build activation)))
+          #~(begin
+              (use-modules (gnu build activation))
+
+              (let ((user (getpw #$username)))
+
+                (define (init-directory directory)
+                  (unless (file-exists? directory)
+                    (mkdir-p/perms directory user #o755)))
+
+                (for-each
+                 init-directory
+                 '#$(map dirname
+                         ;; XXX: Delete the potential "syslog"
+                         ;; log-file value, which is not a directory.
+                         (delete "syslog"
+                                 (filter-map maybe-value
+                                             (list db-file
+                                                   log-file
+                                                   state-file
+                                                   sticker-file))))))
+
+              (make-forkexec-constructor
+               (list #$(file-append package "/bin/mpd") "--no-daemon"
+                     #$config-file)
+               #:environment-variables '#$environment-variables))))
        (stop  #~(make-kill-destructor))
        (actions
         (list (shepherd-configuration-action config-file)
@@ -833,37 +843,49 @@ (define (mympd-serialize-configuration config)
                                        filename-to-field)))))
 
 (define (mympd-shepherd-service config)
-  (match-record config <mympd-configuration> (package shepherd-requirement
-                                              user work-directory
-                                              cache-directory log-level log-to)
-    (let ((log-level* (format #f "MYMPD_LOGLEVEL=~a" log-level))
-          (username (user-account-name user)))
-      (shepherd-service
-       (documentation "Run the myMPD daemon.")
-       (requirement `(loopback user-processes
-                               ,@(if (eq? log-to 'syslog)
-                                     '(syslog)
-                                     '())
-                               ,@shepherd-requirement))
-       (provision '(mympd))
-       (start #~(begin
-                  (let* ((pw (getpwnam #$username))
-                         (uid (passwd:uid pw))
-                         (gid (passwd:gid pw)))
-                    (for-each (lambda (dir)
-                                (mkdir-p dir)
-                                (chown dir uid gid))
-                              (list #$work-directory #$cache-directory)))
-
-                  (make-forkexec-constructor
-                   `(#$(file-append package "/bin/mympd")
-                     "--user" #$username
-                     #$@(if (eq? log-to 'syslog) '("--syslog") '())
-                     "--workdir" #$work-directory
-                     "--cachedir" #$cache-directory)
-                   #:environment-variables (list #$log-level*)
-                   #:log-file #$(if (string? log-to) log-to #f))))
-       (stop #~(make-kill-destructor))))))
+  (match-record config <mympd-configuration>
+    (package shepherd-requirement user work-directory cache-directory
+             log-level log-to)
+    (shepherd-service
+     (documentation "Run the myMPD daemon.")
+     (requirement `(loopback user-processes
+                             ,@(if (eq? log-to 'syslog)
+                                   '(syslog)
+                                   '())
+                             ,@shepherd-requirement))
+     (provision '(mympd))
+     (start
+      (let ((username (user-account-name user)))
+        (with-imported-modules (source-module-closure
+                                '((gnu build activation)))
+          #~(begin
+              (use-modules (gnu build activation))
+
+              (let ((user (getpw #$username)))
+
+                (define (init-directory directory)
+                  (unless (file-exists? directory)
+                    (mkdir-p/perms directory user #o755)))
+
+                (for-each
+                 init-directory
+                 '#$(map dirname
+                         ;; XXX: Delete the potential 'syslog log-file value,
+                         ;; which is not a directory.
+                         (delete 'syslog
+                                 (filter-map maybe-value
+                                             (list log-to
+                                                   work-directory
+                                                   cache-directory))))))
+              (make-forkexec-constructor
+               `(#$(file-append package "/bin/mympd")
+                 "--user" #$username
+                 #$@(if (eq? log-to 'syslog) '("--syslog") '())
+                 "--workdir" #$work-directory
+                 "--cachedir" #$cache-directory)
+               #:environment-variables
+               (list #$(format #f "MYMPD_LOGLEVEL=~a" log-level))
+               #:log-file #$(if (string? log-to) log-to #f)))))))))
 
 (define (mympd-accounts config)
   (match-record config <mympd-configuration>
-- 
2.39.2

bug#63082: [PATCH v2 07/16] services: mpd: Log to syslog by default.

[Maxim Cournoyer]

Rationale: the tristate value was awkward to deal with, the default log file
name was odd (/var/log/mpd/log) and it required special attention to create
the 'mpd' parent directory as root and chowning it to the MPD user.  It also
didn't match the default behavior of MPD, which is to log to systemd or syslog
unless a log file is specified.

* gnu/services/audio.scm (mpd-log-file-sanitizer): New procedure.
(mpd-configuration) [log-file]: Remove default maybe value.  Add sanitizer.
(mpd-shepherd-service): Validate the log file parent directory exists and has
the right permissions.  Conditionally add syslogd to requirements.
(mympd-log-to-sanitizer): New procedure.
(mympd-configuration) [log-to]: Change type to maybe-string.  Update doc and
add sanitizer.
(mympd-shepherd-service) [requirement]: Fix to use syslogd.  Adjust
accordingly.
[start] Adjust accordingly.
(mympd-log-rotation): Check log-to via maybe-value-set?.
* doc/guix.texi (Audio Services): Update doc.
---
 doc/guix.texi          | 17 +++++-----
 gnu/services/audio.scm | 74 ++++++++++++++++++++++++++++--------------
 2 files changed, 57 insertions(+), 34 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 1aa8dc2809..e558e5bb18 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33581,10 +33581,10 @@ Audio Services
 @item @code{environment-variables} (default: @code{("PULSE_CLIENTCONFIG=/etc/pulse/client.conf" "PULSE_CONFIG=/etc/pulse/daemon.conf")}) (type: list-of-strings)
 A list of strings specifying environment variables.
 
-@item @code{log-file} (default: @code{"/var/log/mpd/log"}) (type: maybe-string)
-The location of the log file.  Set to @code{syslog} to use the local
-syslog daemon or @code{%unset-value} to omit this directive from the
-configuration file.
+@item @code{log-file} (type: maybe-string)
+The location of the log file.  Unless specified, logs are sent to the
+local syslog daemon.  Alternatively, a log file name can be specified,
+for example @file{/var/log/mpd.log}.
 
 @item @code{log-level} (type: maybe-string)
 Supress any messages below this threshold.  The available values, in
@@ -33855,11 +33855,10 @@ Audio Services
 How much detail to include in logs, possible values: @code{0} to
 @code{7}.
 
-@item @code{log-to} (default: @code{"/var/log/mympd/log"}) (type: string-or-symbol)
-Where to send logs.  By default, the service logs to
-@file{/var/log/mympd.log}.  The alternative is @code{'syslog}, which
-sends output to the running syslog service under the @samp{daemon}
-facility.
+@item @code{log-to} (type: maybe-string)
+Where to send logs.  Unless specified, the service logs to the local
+syslog service under the @samp{daemon} facility.  Alternatively, a log
+file name can be specified, for example @file{/var/log/mympd.log}.
 
 @item @code{lualibs} (default: @code{"all"}) (type: maybe-string)
 See
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 6e57bf5cba..c1295837b6 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -229,6 +229,18 @@ (define (mpd-group-sanitizer value)
     (warning (G_ "'group' in <mpd-configuration> is obsolete; ignoring~%")))
   #f)
 
+(define (mpd-log-file-sanitizer value)
+  (match value
+    (%unset-value
+     ;; XXX: While leaving the 'sys_log' option out of the mpd.conf file is
+     ;; supposed to cause logging to happen via systemd (elogind provides a
+     ;; compatible interface), this doesn't work (nothing gets logged); use
+     ;; syslog instead.
+     "syslog")
+    ((? string?)
+     value)
+    (_ (configuration-field-error #f 'log-file value))))
+
 ;;;
 
 ;; Generic MPD plugin record, lists only the most prevalent fields.
@@ -402,10 +414,11 @@ (define-configuration mpd-configuration
    empty-serializer)
 
   (log-file
-   (maybe-string "/var/log/mpd/log")
-   "The location of the log file. Set to @code{syslog} to use the
-local syslog daemon or @code{%unset-value} to omit this directive
-from the configuration file.")
+   maybe-string
+   "The location of the log file.  Unless specified, logs are sent to the
+local syslog daemon.  Alternatively, a log file name can be specified, for
+example @file{/var/log/mpd.log}."
+   (sanitizer mpd-log-file-sanitizer))
 
   (log-level
    maybe-string
@@ -562,7 +575,11 @@ (define (mpd-shepherd-service config)
           (username (user-account-name user)))
       (shepherd-service
        (documentation "Run the MPD (Music Player Daemon)")
-       (requirement `(user-processes loopback ,@shepherd-requirement))
+       (requirement `(user-processes loopback
+                                     ,@(if (string=? "syslog" log-file)
+                                           '(syslogd)
+                                           '())
+                                     ,@shepherd-requirement))
        (provision '(mpd))
        (start
         (with-imported-modules (source-module-closure
@@ -683,6 +700,15 @@ (define (mympd-group-sanitizer value)
     (warning (G_ "'group' in <mympd-configuration> is obsolete; ignoring~%")))
   #f)
 
+(define (mympd-log-to-sanitizer value)
+  (match value
+    ('syslog
+     (warning (G_ "syslog symbol value for 'log-to' is deprecated~%"))
+     %unset-value)
+    ((or %unset-value (? string?))
+     value)
+    (_ (configuration-field-error #f 'log-to value))))
+
 ;;;
 
 
@@ -749,10 +775,11 @@ (define-configuration/no-serialization mympd-configuration
    "How much detail to include in logs, possible values: @code{0} to @code{7}.")
 
   (log-to
-   (string-or-symbol "/var/log/mympd/log")
-   "Where to send logs. By default, the service logs to
-@file{/var/log/mympd.log}. The alternative is @code{'syslog}, which
-sends output to the running syslog service under the @samp{daemon} facility."
+   maybe-string
+   "Where to send logs.  Unless specified, the service logs to the local
+syslog service under the @samp{daemon} facility.  Alternatively, a log file
+name can be specified, for example @file{/var/log/mympd.log}."
+   (sanitizer mympd-log-to-sanitizer)
    empty-serializer)
 
   (lualibs
@@ -849,9 +876,9 @@ (define (mympd-shepherd-service config)
     (shepherd-service
      (documentation "Run the myMPD daemon.")
      (requirement `(loopback user-processes
-                             ,@(if (eq? log-to 'syslog)
-                                   '(syslog)
-                                   '())
+                             ,@(if (maybe-value-set? log-to)
+                                   '()
+                                   '(syslogd))
                              ,@shepherd-requirement))
      (provision '(mympd))
      (start
@@ -867,16 +894,12 @@ (define (mympd-shepherd-service config)
                   (unless (file-exists? directory)
                     (mkdir-p/perms directory user #o755)))
 
-                (for-each
-                 init-directory
-                 '#$(map dirname
-                         ;; XXX: Delete the potential 'syslog log-file value,
-                         ;; which is not a directory.
-                         (delete 'syslog
-                                 (filter-map maybe-value
-                                             (list log-to
-                                                   work-directory
-                                                   cache-directory))))))
+                (for-each init-directory
+                          '#$(map dirname (filter-map maybe-value
+                                                      (list log-to
+                                                            work-directory
+                                                            cache-directory)))))
+
               (make-forkexec-constructor
                `(#$(file-append package "/bin/mympd")
                  "--user" #$username
@@ -885,7 +908,7 @@ (define (mympd-shepherd-service config)
                  "--cachedir" #$cache-directory)
                #:environment-variables
                (list #$(format #f "MYMPD_LOGLEVEL=~a" log-level))
-               #:log-file #$(if (string? log-to) log-to #f)))))))))
+               #:log-file #$(maybe-value log-to)))))))))
 
 (define (mympd-accounts config)
   (match-record config <mympd-configuration>
@@ -893,8 +916,9 @@ (define (mympd-accounts config)
     (list user)))
 
 (define (mympd-log-rotation config)
-  (match-record config <mympd-configuration> (log-to)
-    (if (string? log-to)
+  (match-record config <mympd-configuration>
+    (log-to)
+    (if (maybe-value-set? log-to)
         (list (log-rotation
                (files (list log-to))))
         '())))
-- 
2.39.2

bug#63082: [PATCH v2 08/16] services: mpd: Do not rotate logs when using syslog.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-log-rotation): Conditionlize based on the value
of LOG-FILE.
---
 gnu/services/audio.scm | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index c1295837b6..7fb4b8ccf7 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -558,12 +558,15 @@ (define (mpd-serialize-configuration configuration)
    (serialize-configuration configuration mpd-configuration-fields)))
 
 (define (mpd-log-rotation config)
-  (match-record config <mpd-configuration> (log-file)
-    (log-rotation
-     (files (list log-file))
-     (post-rotate #~(begin
-                      (use-modules (gnu services herd))
-                      (with-shepherd-action 'mpd ('reopen) #f))))))
+  (match-record config <mpd-configuration>
+    (log-file)
+    (if (string=? "syslog" log-file)
+        '()                             ;nothing to do
+        (list (log-rotation
+               (files (list log-file))
+               (post-rotate #~(begin
+                                (use-modules (gnu services herd))
+                                (with-shepherd-action 'mpd ('reopen) #f))))))))
 
 (define (mpd-shepherd-service config)
   (match-record config <mpd-configuration>
-- 
2.39.2

bug#63082: [PATCH v2 09/16] services: mpd: Let Shepherd effect the user/group change.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

Quoting a MPD developer, regarding MPD's feature to switch user itself:
"that's legacy for the dark ages when proper service managers did not exist"
:-).

* gnu/services/audio.scm (mpd-serialize-user-account)
(mpd-serialize-user-group): Delete procedures.
* gnu/services/audio.scm (mpd-configuration) [user]: Do not serialize.
[group]: Likewise.
(mpd-shepherd-service): Provide the #:user, #:group and #:supplementary-groups
arguments.
(mympd-shepherd-service): Likewise, and remove the '--user' argument.
* doc/guix.texi (Audio Services): Update doc.
(mympd-configuration) [port]: Change default value to 8080.
[ssl-port]: Change default value to 443.
* gnu/tests/audio.scm (run-mympd-test): Adjust accordingly.
---
 doc/guix.texi          | 12 +++++-----
 gnu/services/audio.scm | 52 +++++++++++++++++++++++++-----------------
 gnu/tests/audio.scm    |  4 ++--
 3 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index e558e5bb18..e4dc4fdd17 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33569,7 +33569,7 @@ Audio Services
 The MPD package.
 
 @item @code{user} (type: user-account)
-The user to run mpd as.
+The user to run @command{mpd} as.
 
 @item @code{group} (default: @code{#f}) (type: boolean)
 Obsolete.  Do not use.
@@ -33613,7 +33613,7 @@ Audio Services
 The location of the sticker database.
 
 @item @code{default-port} (default: @code{6600}) (type: maybe-port)
-The default port to run mpd on.
+The default port to run @command{mpd} on.
 
 @item @code{endpoints} (type: maybe-list-of-strings)
 The addresses that mpd will bind to.  A port different from
@@ -33798,13 +33798,13 @@ Audio Services
 @uref{https://jcorporation.github.io/myMPD/, myMPD} is a web server
 frontend for MPD that provides a mobile friendly web client for MPD.
 
-The following example shows a myMPD instance listening on port 80,
+The following example shows a myMPD instance listening on port 8080,
 with album cover caching disabled.
 
 @lisp
 (service mympd-service-type
          (mympd-configuration
-          (port 80)
+          (port 8080)
           (covercache-ttl 0)))
 @end lisp
 
@@ -33848,7 +33848,7 @@ Audio Services
 @item @code{host} (default: @code{"[::]"}) (type: string)
 Host name to listen on.
 
-@item @code{port} (default: @code{80}) (type: maybe-port)
+@item @code{port} (default: @code{8080}) (type: maybe-port)
 HTTP port to listen on.
 
 @item @code{log-level} (default: @code{5}) (type: integer)
@@ -33874,7 +33874,7 @@ Audio Services
 @item @code{ssl?} (default: @code{#f}) (type: boolean)
 SSL/TLS support.
 
-@item @code{ssl-port} (default: @code{443}) (type: maybe-port)
+@item @code{ssl-port} (default: @code{4443}) (type: maybe-port)
 Port to listen for HTTPS.
 
 @item @code{ssl-cert} (type: maybe-string)
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 7fb4b8ccf7..f470ca20e0 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2019 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2022⁠–⁠2023 Bruno Victal <mirai@makinata.eu>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -165,9 +166,6 @@ (define mpd-serialize-boolean mpd-serialize-field)
 (define (mpd-serialize-list-of-strings field-name value)
   #~(string-append #$@(map (cut mpd-serialize-string field-name <>) value)))
 
-(define (mpd-serialize-user-account field-name value)
-  (mpd-serialize-string field-name (user-account-name value)))
-
 (define-maybe string (prefix mpd-))
 (define-maybe list-of-strings (prefix mpd-))
 (define-maybe boolean (prefix mpd-))
@@ -390,10 +388,14 @@ (define-configuration mpd-configuration
    "The MPD package."
    empty-serializer)
 
+  ;; Note: The user and its group are not serialized, otherwise MPD would
+  ;; attempt to switch the user/group itself.  The task of switching the
+  ;; user/group is left to Shepherd instead.
   (user
    (user-account %mpd-user)
-   "The user to run mpd as."
-   (sanitizer mpd-user-sanitizer))
+   "The user to run @command{mpd} as."
+   (sanitizer mpd-user-sanitizer)
+   empty-serializer)
 
   (group
    (boolean #f)
@@ -458,7 +460,7 @@ (define-configuration mpd-configuration
 
   (default-port
    (maybe-port 6600)
-   "The default port to run mpd on.")
+   "The default port to run @command{mpd} on.")
 
   (endpoints
    maybe-list-of-strings
@@ -611,7 +613,11 @@ (define (mpd-shepherd-service config)
               (make-forkexec-constructor
                (list #$(file-append package "/bin/mpd") "--no-daemon"
                      #$config-file)
-               #:environment-variables '#$environment-variables))))
+               #:environment-variables '#$environment-variables
+               #:user #$username
+               #:group #$(user-account-group user)
+               #:supplementary-groups
+               '#$(user-account-supplementary-groups user)))))
        (stop  #~(make-kill-destructor))
        (actions
         (list (shepherd-configuration-action config-file)
@@ -654,7 +660,7 @@ (define mpd-service-type
           (service-extension account-service-type
                              mpd-accounts)
           (service-extension rottlog-service-type
-                             (compose list mpd-log-rotation))))
+                             mpd-log-rotation)))
    (default-value (mpd-configuration))))
 
 \f
@@ -770,7 +776,7 @@ (define-configuration/no-serialization mympd-configuration
    "Host name to listen on.")
 
   (port
-   (maybe-port 80)
+   (maybe-port 8080)
    "HTTP port to listen on.")
 
   (log-level
@@ -805,7 +811,7 @@ (define-configuration/no-serialization mympd-configuration
    "SSL/TLS support.")
 
   (ssl-port
-   (maybe-port 443)
+   (maybe-port 4443)
    "Port to listen for HTTPS.")
 
   (ssl-cert
@@ -901,17 +907,21 @@ (define (mympd-shepherd-service config)
                           '#$(map dirname (filter-map maybe-value
                                                       (list log-to
                                                             work-directory
-                                                            cache-directory)))))
-
-              (make-forkexec-constructor
-               `(#$(file-append package "/bin/mympd")
-                 "--user" #$username
-                 #$@(if (eq? log-to 'syslog) '("--syslog") '())
-                 "--workdir" #$work-directory
-                 "--cachedir" #$cache-directory)
-               #:environment-variables
-               (list #$(format #f "MYMPD_LOGLEVEL=~a" log-level))
-               #:log-file #$(maybe-value log-to)))))))))
+                                                            cache-directory))))
+
+                (make-forkexec-constructor
+                 `(#$(file-append package "/bin/mympd")
+                   "--user" #$username
+                   #$@(if (eq? log-to 'syslog) '("--syslog") '())
+                   "--workdir" #$work-directory
+                   "--cachedir" #$cache-directory)
+                 #:environment-variables
+                 (list #$(format #f "MYMPD_LOGLEVEL=~a" log-level))
+                 #:log-file #$(maybe-value log-to)
+                 #:user #$username
+                 #:group #$(user-account-group user)
+                 #:supplementary-groups
+                 '#$(user-account-supplementary-groups user))))))))))
 
 (define (mympd-accounts config)
   (match-record config <mympd-configuration>
diff --git a/gnu/tests/audio.scm b/gnu/tests/audio.scm
index acb91293e8..efa07b5ba9 100644
--- a/gnu/tests/audio.scm
+++ b/gnu/tests/audio.scm
@@ -89,7 +89,7 @@ (define (run-mympd-test)
   (define vm
     (virtual-machine
      (operating-system os)
-     (port-forwardings '((8080 . 80)))))
+     (port-forwardings '((8080 . 8080)))))
 
   (define test
     (with-imported-modules '((gnu build marionette))
@@ -113,7 +113,7 @@ (define (run-mympd-test)
                              marionette))
 
           (test-assert "HTTP port ready"
-            (wait-for-tcp-port 80 marionette))
+            (wait-for-tcp-port 8080 marionette))
 
           (test-equal "http-head"
             200
-- 
2.39.2

bug#63082: [PATCH v2 10/16] system: accounts: Export <user-account>.

[Maxim Cournoyer]

---
 gnu/system/accounts.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gnu/system/accounts.scm b/gnu/system/accounts.scm
index 586cff1842..e37b733c6d 100644
--- a/gnu/system/accounts.scm
+++ b/gnu/system/accounts.scm
@@ -19,7 +19,8 @@
 (define-module (gnu system accounts)
   #:use-module (guix records)
   #:use-module (ice-9 match)
-  #:export (user-account
+  #:export (<user-account>
+            user-account
             user-account?
             user-account-name
             user-account-password
-- 
2.39.2

bug#63082: [PATCH v2 11/16] services: mpd: Warn when the MPD user is not in the "audio" group.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org>.

* gnu/services/audio.scm (%mpd-user) [group]: Add comment.
(mpd-user-sanitizer): Warn if the MPD user is not in the audio group.
---
 gnu/services/audio.scm | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index f470ca20e0..7040a63ecd 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -31,6 +31,7 @@ (define-module (gnu services audio)
   #:use-module (gnu services configuration)
   #:use-module (gnu services shepherd)
   #:use-module (gnu services admin)
+  #:use-module (gnu system accounts)
   #:use-module (gnu system shadow)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages mpd)
@@ -173,6 +174,8 @@ (define-maybe boolean (prefix mpd-))
 (define %mpd-user
   (user-account
    (name "mpd")
+   ;; Being in the audio group ensures that PulseAudio can access sound
+   ;; devices.
    (group "audio")
    (system? #t)
    (comment "Music Player Daemon (MPD) user")
@@ -209,10 +212,17 @@ (define (mpd-serialize-port field-name value)
 
 (define-maybe port (prefix mpd-))
 
-;;; Procedures for unsupported value types, to be removed.
-
+;;; Sanitizer procedures.
 (define (mpd-user-sanitizer value)
-  (cond ((user-account? value) value)
+  (cond ((user-account? value)
+         (match-record value <user-account>
+           (group supplementary-groups)
+           (unless (or (string=? "audio" group)
+                       (member "audio" supplementary-groups))
+             ;; Being in the "audio" group is necessary for access to the
+             ;; sound devices.
+             (warning (G_ "mpd user not member of \"audio\" group~%"))))
+         value)
         ((string? value)
          (warning (G_ "string value for 'user' is deprecated, use \
 user-account instead~%"))
-- 
2.39.2

bug#63082: [PATCH v2 12/16] services: mpd: Auto-detect mpd-output mixer type by default.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

* gnu/services/audio.scm (mpd-output) [mixer-type]: Change default value from
"none" to unspecified.
* doc/guix.texi (Audio Services): Regenerate doc.
---
 doc/guix.texi          | 11 +++++++----
 gnu/services/audio.scm | 15 +++++++++------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index e4dc4fdd17..3de7405318 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33710,8 +33710,9 @@ Audio Services
 @end table
 @end deftp
 
+@c %start of fragment
 @deftp {Data Type} mpd-output
-Data type representing a @command{mpd} audio output.
+Available @code{mpd-output} fields are:
 
 @table @asis
 @item @code{name} (default: @code{"MPD"}) (type: string)
@@ -33738,15 +33739,16 @@ Audio Services
 
 @item @code{always-on?} (default: @code{#f}) (type: boolean)
 If set to @code{#t}, then MPD attempts to keep this audio output always
-open.  This may be useful for streaming servers, when you don?t want to
+open.  This may be useful for streaming servers, when you don’t want to
 disconnect all listeners even when playback is accidentally stopped.
 
-@item @code{mixer-type} (default: @code{"none"}) (type: string)
+@item @code{mixer-type} (type: maybe-string)
 This field accepts a string that specifies which mixer should be used
 for this audio output: the @code{hardware} mixer, the @code{software}
 mixer, the @code{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 (@code{none}).
+External Mixer) or no mixer (@code{none}).  When left unspecified, a
+@code{hardware} mixer is used for devices that support it.
 
 @item @code{replay-gain-handler} (type: maybe-string)
 This field accepts a string that specifies how
@@ -33761,6 +33763,7 @@ Audio Services
 
 @end table
 @end deftp
+@c %end of fragment
 
 The following example shows a configuration of @command{mpd} that
 configures some of its plugins and provides a HTTP audio streaming output.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 7040a63ecd..1e0a8b7f9e 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -346,15 +346,18 @@ (define-configuration mpd-output
 disconnect all listeners even when playback is accidentally stopped.")
 
   (mixer-type
-   (string "none")
-   "This field accepts a string that specifies which mixer should be used
-for this audio output: the @code{hardware} mixer, the @code{software}
-mixer, the @code{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 (@code{none})."
+   maybe-string
+   "This field accepts a string that specifies which mixer should be used for
+this audio output: the @code{hardware} mixer, the @code{software} mixer, the
+@code{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 (@code{none}).  When left unspecified, a @code{hardware} mixer is used
+for devices that support it."
    (sanitizer
     (lambda (x)  ; TODO: deprecated, remove me later.
       (cond
+       ((eq? %unset-value x)
+        x)
        ((symbol? x)
         (warning (G_ "symbol value for 'mixer-type' is deprecated, \
 use string instead~%"))
-- 
2.39.2

bug#63082: [PATCH v2 13/16] services: mpd: Obsolete 'environment-variables' field.

[Maxim Cournoyer]

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.
---
 doc/guix.texi          |  4 ++--
 gnu/services/audio.scm | 17 ++++++++++++-----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 3de7405318..148ca88633 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33578,8 +33578,8 @@ Audio Services
 This is a list of symbols naming Shepherd services that this service
 will depend on.
 
-@item @code{environment-variables} (default: @code{("PULSE_CLIENTCONFIG=/etc/pulse/client.conf" "PULSE_CONFIG=/etc/pulse/daemon.conf")}) (type: list-of-strings)
-A list of strings specifying environment variables.
+@item @code{environment-variables} (default: @code{#f}) (type: boolean)
+Obsolete.  Do not use.
 
 @item @code{log-file} (type: maybe-string)
 The location of the log file.  Unless specified, logs are sent to the
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 1e0a8b7f9e..dca2e8e5f6 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -249,7 +249,11 @@ (define (mpd-log-file-sanitizer value)
      value)
     (_ (configuration-field-error #f 'log-file value))))
 
-;;;
+(define (mpd-environment-variables-sanitizer value)
+  (when value
+    (warning (G_ "'environment-variables' in <mpd-configuration> is obsolete;\
+ ignoring~%")))
+  #f)
 
 ;; Generic MPD plugin record, lists only the most prevalent fields.
 (define-configuration mpd-plugin
@@ -423,9 +427,9 @@ (define-configuration mpd-configuration
    empty-serializer)
 
   (environment-variables
-   (list-of-strings '("PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
-                      "PULSE_CONFIG=/etc/pulse/daemon.conf"))
-   "A list of strings specifying environment variables."
+   (boolean #f)
+   "Obsolete.  Do not use."
+   (sanitizer mpd-environment-variables-sanitizer)
    empty-serializer)
 
   (log-file
@@ -626,7 +630,10 @@ (define (mpd-shepherd-service config)
               (make-forkexec-constructor
                (list #$(file-append package "/bin/mpd") "--no-daemon"
                      #$config-file)
-               #:environment-variables '#$environment-variables
+               #:environment-variables
+               ;; Use the system-configured pulse configuration.
+               (list "PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
+                     "PULSE_CONFIG=/etc/pulse/daemon.conf")
                #:user #$username
                #:group #$(user-account-group user)
                #:supplementary-groups
-- 
2.39.2

bug#63082: [PATCH v2 14/16] services: mpd: Provision a default cache directory and set HOME.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

* gnu/services/audio.scm (mpd-shepherd-service): Create a default .cache
directory.  Use mkdir-p/perms and refactor loop.  Set the HOME environment
variables.
---
 doc/guix.texi          |  3 +-
 gnu/services/audio.scm | 68 ++++++++++++++++++++++++------------------
 2 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 148ca88633..abfbbdb2fc 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33604,7 +33604,8 @@ Audio Services
 The directory to store playlists.
 
 @item @code{db-file} (type: maybe-string)
-The location of the music database.
+The location of the music database.  When left unspecified,
+@file{~/.cache/db} is used.
 
 @item @code{state-file} (type: maybe-string)
 The location of the file that stores current MPD's state.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index dca2e8e5f6..0e90d72462 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -465,7 +465,8 @@ (define-configuration mpd-configuration
 
   (db-file
    maybe-string
-   "The location of the music database.")
+   "The location of the music database.  When left unspecified,
+@file{~/.cache/db} is used.")
 
   (state-file
    maybe-string
@@ -609,35 +610,44 @@ (define (mpd-shepherd-service config)
           #~(begin
               (use-modules (gnu build activation))
 
-              (let ((user (getpw #$username)))
-
-                (define (init-directory directory)
-                  (unless (file-exists? directory)
-                    (mkdir-p/perms directory user #o755)))
+              (let ((home #$(user-account-home-directory user)))
+                (let ((user (getpw #$username))
+                      (default-cache-dir (string-append home "/.cache")))
+
+                  (define (init-directory directory)
+                    (unless (file-exists? directory)
+                      (mkdir-p/perms directory user #o755)))
+
+                  ;; Define a cache location that can be automatically used
+                  ;; for the database file, in case it hasn't been explicitly
+                  ;; specified.
+                  (for-each
+                   init-directory
+                   (cons default-cache-dir
+                         '#$(map dirname
+                                 ;; XXX: Delete the potential "syslog"
+                                 ;; log-file value, which is not a directory.
+                                 (delete "syslog"
+                                         (filter-map maybe-value
+                                                     (list db-file
+                                                           log-file
+                                                           state-file
+                                                           sticker-file)))))))
 
-                (for-each
-                 init-directory
-                 '#$(map dirname
-                         ;; XXX: Delete the potential "syslog"
-                         ;; log-file value, which is not a directory.
-                         (delete "syslog"
-                                 (filter-map maybe-value
-                                             (list db-file
-                                                   log-file
-                                                   state-file
-                                                   sticker-file))))))
-
-              (make-forkexec-constructor
-               (list #$(file-append package "/bin/mpd") "--no-daemon"
-                     #$config-file)
-               #:environment-variables
-               ;; Use the system-configured pulse configuration.
-               (list "PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
-                     "PULSE_CONFIG=/etc/pulse/daemon.conf")
-               #:user #$username
-               #:group #$(user-account-group user)
-               #:supplementary-groups
-               '#$(user-account-supplementary-groups user)))))
+                (make-forkexec-constructor
+                 (list #$(file-append package "/bin/mpd") "--no-daemon"
+                       #$config-file)
+                 #:environment-variables
+                 ;; Use the system-configured pulse configuration.  Set HOME
+                 ;; so MPD can infer default paths, such as for the database
+                 ;; file.
+                 (list (string-append "HOME=" home)
+                       "PULSE_CLIENTCONFIG=/etc/pulse/client.conf"
+                       "PULSE_CONFIG=/etc/pulse/daemon.conf")
+                 #:user #$username
+                 #:group #$(user-account-group user)
+                 #:supplementary-groups
+                 '#$(user-account-supplementary-groups user))))))
        (stop  #~(make-kill-destructor))
        (actions
         (list (shepherd-configuration-action config-file)
-- 
2.39.2

bug#63082: [PATCH v2 15/16] services: mpd: Update basic example.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

* doc/guix.texi (Audio Services): Do not use a deprecated user form; keep the
default one.  Remove port.  Specify a music-directory.  Mention the importance
of permissions on the music directory.
---
 doc/guix.texi | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index abfbbdb2fc..a26c46ff61 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33536,16 +33536,27 @@ Audio Services
 being controlled from the local machine or over the network by a variety
 of clients.
 
-The following example shows how one might run @code{mpd} as user
-@code{"bob"} on port @code{6666}.  It uses pulseaudio for output.
+The following example shows the simplest configuration to locally
+expose, via PulseAudio, a music collection kept at @file{/srv/music},
+with @command{mpd} running as the default @samp{mpd} user.  This user
+will spawn its own PulseAudio daemon, which may compete for the sound
+card access with that of your own user.  In this configuration, you may
+have to stop the playback of your user audio applications to hear MPD's
+output and vice-versa.
 
 @lisp
 (service mpd-service-type
          (mpd-configuration
-          (user "bob")
-          (port "6666")))
+          (music-directory "/srv/music")))
 @end lisp
 
+@quotation Important
+The music directory must be readable to the MPD user, by default,
+@samp{mpd}.  Permission problems will be reported via @samp{Permission
+denied} errors in the MPD logs, which appear in @file{/var/log/messages}
+by default.
+@end quotation
+
 Most MPD clients will trigger a database update upon connecting, but you
 can also use the @code{update} action do to so:
 
-- 
2.39.2

bug#63082: [PATCH v2 16/16] services: Avoid 'delete' overrides warning in audio module.

[Maxim Cournoyer]

* gnu/services/audio.scm: Hide 'delete' on (gnu services) import.
---
 gnu/services/audio.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 0e90d72462..9fc113ed32 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -26,7 +26,7 @@ (define-module (gnu services audio)
   #:use-module (guix diagnostics)
   #:use-module (guix i18n)
   #:use-module (guix modules)
-  #:use-module (gnu services)
+  #:use-module ((gnu services) #:hide (delete))
   #:use-module (gnu services admin)
   #:use-module (gnu services configuration)
   #:use-module (gnu services shepherd)
-- 
2.39.2

bug#63082: [PATCH v3 00/16] Improve out-of-the-box experience with mpd-service-type

[Maxim Cournoyer]

This series reinstate the ENVIRONMENT-VARIABLES configuration option.  The
rest is unchanged.

Maxim Cournoyer (16):
  services: mpd: Add auto-update? field to mpd-configuration.
  services: mpd: Add an 'update' action to trigger a database update.
  services: mpd: Streamline mpd-user-sanitizer and mympd-user-sanitizer.
  services: mpd: Rename %set-user-group to set-user-group.
  services: mpd: Obsolete the 'group' field.
  services: mpd: List log-level in decreasing verbosity order in doc.
  services: mpd; Refactor start slot directory initialization.
  services: mpd: Log to syslog by default.
  services: mpd: Do not rotate logs when using syslog.
  services: mpd: Let Shepherd effect the user/group change.
  system: accounts: Export <user-account>.
  services: mpd: Warn when the MPD user is not in the "audio" group.
  services: mpd: Auto-detect mpd-output mixer type by default.
  services: mpd: Provision a default cache directory and set HOME.
  services: mpd: Update basic example.
  services: Avoid 'delete' overrides warning in audio module.

 doc/guix.texi           | 130 ++++++++-----
 gnu/services/audio.scm  | 394 +++++++++++++++++++++++-----------------
 gnu/system/accounts.scm |   3 +-
 gnu/tests/audio.scm     |   4 +-
 4 files changed, 311 insertions(+), 220 deletions(-)


base-commit: 6922069bcbe5c08da09c00e5aad44e390ebd1cc7
-- 
2.39.2

bug#63082: [PATCH v3 01/16] services: mpd: Add auto-update? field to mpd-configuration.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-configuration) [auto-update?]: New field.
* doc/guix.texi (Audio Services): Update doc.
---
 doc/guix.texi          | 39 ++++++++++++++++++++++++++-------------
 gnu/services/audio.scm |  5 +++++
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 55221a10c3..66eb44812d 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33579,24 +33579,22 @@ Audio Services
 The service type for @command{mpd}
 @end defvar
 
+@c %start of fragment
 @deftp {Data Type} mpd-configuration
-Data type representing the configuration of @command{mpd}.
+Available @code{mpd-configuration} fields are:
 
 @table @asis
 @item @code{package} (default: @code{mpd}) (type: file-like)
 The MPD package.
 
-@item @code{user} (default: @code{%mpd-user}) (type: user-account)
+@item @code{user} (type: user-account)
 The user to run mpd as.
 
-The default @code{%mpd-user} is a system user with the name ``mpd'',
-who is a part of the group @var{group} (see below).
-@item @code{group} (default: @code{%mpd-group}) (type: user-group)
+@item @code{group} (type: user-group)
 The group to run mpd as.
 
-The default @code{%mpd-group} is a system group with name ``mpd''.
 @item @code{shepherd-requirement} (default: @code{()}) (type: list-of-symbol)
-A list of symbols naming Shepherd services that this service
+This is a list of symbols naming Shepherd services that this service
 will depend on.
 
 @item @code{environment-variables} (default: @code{("PULSE_CLIENTCONFIG=/etc/pulse/client.conf" "PULSE_CONFIG=/etc/pulse/daemon.conf")}) (type: list-of-strings)
@@ -33615,9 +33613,15 @@ Audio Services
 @item @code{music-directory} (type: maybe-string)
 The directory to scan for music files.
 
+@item @code{music-dir} (type: maybe-string)
+The directory to scan for music files.
+
 @item @code{playlist-directory} (type: maybe-string)
 The directory to store playlists.
 
+@item @code{playlist-dir} (type: maybe-string)
+The directory to store playlists.
+
 @item @code{db-file} (type: maybe-string)
 The location of the music database.
 
@@ -33627,15 +33631,19 @@ Audio Services
 @item @code{sticker-file} (type: maybe-string)
 The location of the sticker database.
 
-@item @code{default-port} (default: @code{6600}) (type: maybe-integer)
+@item @code{default-port} (default: @code{6600}) (type: maybe-port)
 The default port to run mpd on.
 
 @item @code{endpoints} (type: maybe-list-of-strings)
-The addresses that mpd will bind to.  A port different from @var{default-port}
-may be specified, e.g. @code{localhost:6602} and IPv6 addresses must be
-enclosed in square brackets when a different port is used.
-To use a Unix domain socket, an absolute path or a path starting with @code{~}
-can be specified here.
+The addresses that mpd will bind to.  A port different from
+@var{default-port} may be specified, e.g.  @code{localhost:6602} and
+IPv6 addresses must be enclosed in square brackets when a different port
+is used.  To use a Unix domain socket, an absolute path or a path
+starting with @code{~} can be specified here.
+
+@item @code{address} (type: maybe-string)
+The address that mpd will bind to.  To use a Unix domain socket, an
+absolute path can be specified here.
 
 @item @code{database} (type: maybe-mpd-plugin)
 MPD database plugin configuration.
@@ -33652,6 +33660,10 @@ Audio Services
 @item @code{archive-plugins} (default: @code{()}) (type: list-of-mpd-plugin)
 List of MPD archive plugin configurations.
 
+@item @code{auto-update?} (type: maybe-boolean)
+Whether to automatically update the music database when files are
+changed in the @var{music-directory}.
+
 @item @code{input-cache-size} (type: maybe-string)
 MPD input cache size.
 
@@ -33677,6 +33689,7 @@ Audio Services
 
 @end table
 @end deftp
+@c %end of fragment
 
 @deftp {Data Type} mpd-plugin
 Data type representing a @command{mpd} plugin.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 690409b7a1..8c061da47f 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -514,6 +514,11 @@ (define-configuration mpd-configuration
    (serializer (lambda (_ x)
                  (mpd-serialize-list-of-mpd-plugin "archive_plugin" x))))
 
+  (auto-update?
+   maybe-boolean
+   "Whether to automatically update the music database when files are changed
+in the @var{music-directory}.")
+
   (input-cache-size
    maybe-string
    "MPD input cache size."
-- 
2.39.2

bug#63082: [PATCH v3 02/16] services: mpd: Add an 'update' action to trigger a database update.

[Maxim Cournoyer]

* 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(+)

diff --git a/doc/guix.texi b/doc/guix.texi
index 66eb44812d..d68d7dd7eb 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33575,6 +33575,16 @@ Audio Services
           (port "6666")))
 @end lisp
 
+Most MPD clients will trigger a database update upon connecting, but you
+can also use the @code{update} action do to so:
+
+@example
+herd update mpd
+@end example
+
+All the MPD configuration fields are documented below, and a more
+complex example follows.
+
 @defvar mpd-service-type
 The service type for @command{mpd}
 @end defvar
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 8c061da47f..6e4ce3f9fb 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -620,6 +620,17 @@ (define (mpd-shepherd-service config)
                           (format #t
                                   "Issued SIGHUP to Service MPD (PID ~a)."
                                   pid))
+                        (format #t "Service MPD is not running.")))))
+              (shepherd-action
+               (name 'update)
+               (documentation "Request MPD to update its music database.")
+               (procedure
+                #~(lambda (pid)
+                    (if pid
+                        (begin
+                          (invoke #$(file-append mpd-mpc "/bin/mpc") "update")
+                          (format #t "Database update requested for service \
+MPD (PID ~a)." pid))
                         (format #t "Service MPD is not running.")))))))))))
 
 (define (mpd-accounts config)
-- 
2.39.2

bug#63082: [PATCH v3 02/16] services: mpd: Add an 'update' action to trigger a database update.

[Bruno Victal]

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)


-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.

bug#63082: mpd defaul configuration does not work ('No database' error)

[Maxim Cournoyer]

Hi Bruno,

Bruno Victal <mirai@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.

-- 
Thanks,
Maxim

bug#63082: mpd defaul configuration does not work ('No database' error)

[Bruno Victal]

Hi Maxim,

On 2023-07-25 21:48, Maxim Cournoyer wrote:
> Hi Bruno,
> 
> Bruno Victal <mirai@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>


-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.

bug#63082: mpd defaul configuration does not work ('No database' error)

[Maxim Cournoyer]

Hi,

Bruno Victal <mirai@makinata.eu> writes:

> Hi Maxim,
>
> On 2023-07-25 21:48, Maxim Cournoyer wrote:
>> Hi Bruno,
>> 
>> Bruno Victal <mirai@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

bug#63082: [PATCH v3 03/16] services: mpd: Streamline mpd-user-sanitizer and mympd-user-sanitizer.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-user-sanitizer, %mympd-user): Remove extraneous
group field, already inherited.
(%mpd-user, %mympd-user): Clarify %lazy-group explanatory comment.  Fix
indentation.
---
 gnu/services/audio.scm | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 6e4ce3f9fb..dc83479e40 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -184,13 +184,15 @@ (define-maybe boolean (prefix mpd-))
 
 (define %mpd-user
   (user-account
-      (name "mpd")
-      (group %lazy-group)
-      (system? #t)
-      (comment "Music Player Daemon (MPD) user")
-      ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data
-      (home-directory "/var/lib/mpd")
-      (shell (file-append shadow "/sbin/nologin"))))
+   (name "mpd")
+   ;; XXX: This is a place-holder to be lazily substituted in (…-accounts)
+   ;; with the value from the 'group' field of <mpd-configuration>.
+   (group %lazy-group)
+   (system? #t)
+   (comment "Music Player Daemon (MPD) user")
+   ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data.
+   (home-directory "/var/lib/mpd")
+   (shell (file-append shadow "/sbin/nologin"))))
 
 (define %mpd-group
   (user-group
@@ -235,10 +237,7 @@ (define (mpd-user-sanitizer value)
 user-account instead~%"))
          (user-account
           (inherit %mpd-user)
-          (name value)
-          ;; XXX: This is to be lazily substituted in (…-accounts)
-          ;; with the value from 'group'.
-          (group %lazy-group)))
+          (name value)))
         (else
          (configuration-field-error #f 'user value))))
 
@@ -676,12 +675,14 @@ (define-maybe/no-serialization mympd-ip-acl)
 
 (define %mympd-user
   (user-account
-      (name "mympd")
-      (group %lazy-group)
-      (system? #t)
-      (comment "myMPD user")
-      (home-directory "/var/empty")
-      (shell (file-append shadow "/sbin/nologin"))))
+   (name "mympd")
+   ;; XXX: This is a place-holder to be lazily substituted in 'mympd-accounts'
+   ;; with the value from the 'group' field of <mympd-configuration>.
+   (group %lazy-group)
+   (system? #t)
+   (comment "myMPD user")
+   (home-directory "/var/empty")
+   (shell (file-append shadow "/sbin/nologin"))))
 
 (define %mympd-group
   (user-group
@@ -696,10 +697,7 @@ (define (mympd-user-sanitizer value)
 user-account instead~%"))
          (user-account
           (inherit %mympd-user)
-          (name value)
-          ;; XXX: this is to be lazily substituted in (…-accounts)
-          ;; with the value from 'group'.
-          (group %lazy-group)))
+          (name value)))
         (else
          (configuration-field-error #f 'user value))))
 
-- 
2.39.2

bug#63082: [PATCH v3 04/16] services: mpd: Rename %set-user-group to set-user-group.

[Maxim Cournoyer]

The convention to use % as a prefix is for "special" variables rather than
procedures.

* gnu/services/audio.scm ((%set-user-group): Rename to...
(set-user-group): ... this.
---
 gnu/services/audio.scm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index dc83479e40..7874539810 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -143,7 +143,7 @@ (define list-of-symbol?
 ;; Helpers for deprecated field types, to be removed later.
 (define %lazy-group (make-symbol "%lazy-group"))
 
-(define (%set-user-group user group)
+(define (set-user-group user group)
   (user-account
    (inherit user)
    (group (user-group-name group))))
@@ -636,7 +636,7 @@ (define (mpd-accounts config)
   (match-record config <mpd-configuration> (user group)
     ;; TODO: Deprecation code, to be removed.
     (let ((user (if (eq? (user-account-group user) %lazy-group)
-                    (%set-user-group user group)
+                    (set-user-group user group)
                     user)))
       (list user group))))
 
@@ -907,7 +907,7 @@ (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)
+                    (set-user-group user group)
                     user)))
       (list user group))))
 
-- 
2.39.2

bug#63082: [PATCH v3 04/16] services: mpd: Rename %set-user-group to set-user-group.

[Liliana Marie Prikler]

Am Freitag, dem 05.05.2023 um 14:28 -0400 schrieb Maxim Cournoyer:
> The convention to use % as a prefix is for "special" variables rather
> than procedures.
Variables are a superset of procedures in Scheme :)
That being said, I'm d'accord with the actual change.

> * gnu/services/audio.scm ((%set-user-group): Rename to...
One parenthesis too much.
> (set-user-group): ... this.
Cheers

bug#63082: [PATCH v3 04/16] services: mpd: Rename %set-user-group to set-user-group.

[Maxim Cournoyer]

Hi Liliana!

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Freitag, dem 05.05.2023 um 14:28 -0400 schrieb Maxim Cournoyer:
>> The convention to use % as a prefix is for "special" variables rather
>> than procedures.
> Variables are a superset of procedures in Scheme :)
> That being said, I'm d'accord with the actual change.

Uh, interesting.

>> * gnu/services/audio.scm ((%set-user-group): Rename to...
> One parenthesis too much.

Thanks, fixed locally.

-- 
Thanks,
Maxim

bug#63082: [PATCH v3 04/16] services: mpd: Rename %set-user-group to set-user-group.

[Bruno Victal]

On 2023-05-05 19:28, Maxim Cournoyer wrote:
> The convention to use % as a prefix is for "special" variables rather than
> procedures.
> 
> * gnu/services/audio.scm ((%set-user-group): Rename to...
> (set-user-group): ... this.
> ---
>  gnu/services/audio.scm | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Though that's what is stated in
<http://community.schemewiki.org/?variable-naming-convention>,
there is precedent within guix to prefix “internal” procedures with
% in certain cases like changing the default constructor for the
record-type itself.

Some commits that show this: 22dd558c70901a336de97187f0470be584571158,
2397f4768091210b0a705ef750f2f38d6946fb89.

-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.

bug#63082: [PATCH v3 05/16] services: mpd: Obsolete the 'group' field.

[Maxim Cournoyer]

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".
(%mympd-group): Delete variable.
* doc/guix.texi: Regenerate doc.
---
 doc/guix.texi          | 15 +++-----
 gnu/services/audio.scm | 83 +++++++++++-------------------------------
 2 files changed, 27 insertions(+), 71 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index d68d7dd7eb..db8f275bf2 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33600,8 +33600,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
@@ -33853,15 +33853,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.
 
-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.
 
@@ -33901,7 +33898,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.
 
 @item @code{ssl?} (default: @code{#f}) (type: boolean)
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 7874539810..60387272fc 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -140,14 +140,6 @@ (define (uglify-field-name field-name)
 (define list-of-symbol?
   (list-of symbol?))
 
-;; Helpers for deprecated field types, to be removed later.
-(define %lazy-group (make-symbol "%lazy-group"))
-
-(define (set-user-group user group)
-  (user-account
-   (inherit user)
-   (group (user-group-name group))))
-
 \f
 ;;;
 ;;; MPD
@@ -175,9 +167,6 @@ (define (mpd-serialize-list-of-strings field-name value)
 (define (mpd-serialize-user-account field-name value)
   (mpd-serialize-string field-name (user-account-name value)))
 
-(define (mpd-serialize-user-group field-name value)
-  (mpd-serialize-string field-name (user-group-name value)))
-
 (define-maybe string (prefix mpd-))
 (define-maybe list-of-strings (prefix mpd-))
 (define-maybe boolean (prefix mpd-))
@@ -185,20 +174,13 @@ (define-maybe boolean (prefix mpd-))
 (define %mpd-user
   (user-account
    (name "mpd")
-   ;; XXX: This is a place-holder to be lazily substituted in (…-accounts)
-   ;; with the value from the 'group' field of <mpd-configuration>.
-   (group %lazy-group)
+   (group "audio")
    (system? #t)
    (comment "Music Player Daemon (MPD) user")
    ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data.
    (home-directory "/var/lib/mpd")
    (shell (file-append shadow "/sbin/nologin"))))
 
-(define %mpd-group
-  (user-group
-   (name "mpd")
-   (system? #t)))
-
 ;;; TODO: Procedures for deprecated fields, to be removed.
 
 (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)
 
 ;;;
 
@@ -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)
+   empty-serializer)
 
   (shepherd-requirement
    (list-of-symbol '())
@@ -633,12 +610,9 @@ (define (mpd-shepherd-service config)
                         (format #t "Service MPD is not running.")))))))))))
 
 (define (mpd-accounts config)
-  (match-record config <mpd-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 <mpd-configuration>
+    (user)
+    (list user)))
 
 (define mpd-service-type
   (service-type
@@ -676,19 +650,12 @@ (define-maybe/no-serialization mympd-ip-acl)
 (define %mympd-user
   (user-account
    (name "mympd")
-   ;; XXX: This is a place-holder to be lazily substituted in 'mympd-accounts'
-   ;; with the value from the 'group' field of <mympd-configuration>.
-   (group %lazy-group)
+   (group "nogroup")
    (system? #t)
    (comment "myMPD user")
    (home-directory "/var/empty")
    (shell (file-append shadow "/sbin/nologin"))))
 
-(define %mympd-group
-  (user-group
-   (name "mympd")
-   (system? #t)))
-
 ;;; TODO: Procedures for unsupported value types, to be removed.
 (define (mympd-user-sanitizer value)
   (cond ((user-account? value) value)
@@ -702,15 +669,10 @@ (define (mympd-user-sanitizer value)
          (configuration-field-error #f 'user value))))
 
 (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)
+
 ;;;
 
 
@@ -737,8 +699,8 @@ (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)
 
@@ -904,12 +866,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)))
 
 (define (mympd-log-rotation config)
   (match-record config <mympd-configuration> (log-to)
-- 
2.39.2

bug#63082: [PATCH v3 05/16] services: mpd: Obsolete the 'group' field.

[Liliana Marie Prikler]

Am Freitag, dem 05.05.2023 um 14:29 -0400 schrieb Maxim Cournoyer:
> 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.  
Didn't we agree in v2 that we want to address this on the account-
service level?  Unless the rest of this series somehow depends on this
patch, I'd rather delay it until we have a proper solution.

> Synchronizing both is not practical, as it can easily lead to
> slightly different <user-account> objects conflicting, again causing
> problems.
It might not be practical to do so inside the service, but note how
this has already become an effort in defensive programming.  There are
easier ways to not make this a problem on the configuration level,
namely by specifying the same group for both user and group fields.  As
far as I see this is even the default state of being if the user is
supplied as a string.


Cheers

bug#63082: [PATCH v3 05/16] services: mpd: Obsolete the 'group' field.

[Maxim Cournoyer]

Hi!

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Freitag, dem 05.05.2023 um 14:29 -0400 schrieb Maxim Cournoyer:
>> 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.  
> Didn't we agree in v2 that we want to address this on the account-
> service level?  Unless the rest of this series somehow depends on this
> patch, I'd rather delay it until we have a proper solution.

I think we agreed the idea to have <user-account> support <user-group>
objects for its group field was a good idea that should be implemented,
but I declined doing this new work as part of this series :-).

>> Synchronizing both is not practical, as it can easily lead to
>> slightly different <user-account> objects conflicting, again causing
>> problems.
> It might not be practical to do so inside the service, but note how
> this has already become an effort in defensive programming.  There are
> easier ways to not make this a problem on the configuration level,
> namely by specifying the same group for both user and group fields.  As
> far as I see this is even the default state of being if the user is
> supplied as a string.

I really don't like the group information being duplicated in both the
user and a distinct field; it's an awkward API that raises more
questions than it provides answers, in my opinion (non-intuitive).  One
of the reasons I came think this way is because a <user-group> can
differ by being a system group or not, which would make it easy to
introduce unexpected, subtle variants.

-- 
Thanks,
Maxim

bug#63082: [PATCH v3 05/16] services: mpd: Obsolete the 'group' field.

[Liliana Marie Prikler]

Am Samstag, dem 06.05.2023 um 22:55 -0400 schrieb Maxim Cournoyer:
> Hi!
> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > Am Freitag, dem 05.05.2023 um 14:29 -0400 schrieb Maxim Cournoyer:
> > Didn't we agree in v2 that we want to address this on the account-
> > service level?  Unless the rest of this series somehow depends on
> > this patch, I'd rather delay it until we have a proper solution.
> 
> I think we agreed the idea to have <user-account> support <user-
> group> objects for its group field was a good idea that should be
> implemented, but I declined doing this new work as part of this
> series :-).
Indeed, that's how I understood it.  However, I also thought that
addressing this issue in a later series means we can keep the current
behaviour until that is done.

> > > Synchronizing both is not practical, as it can easily lead to
> > > slightly different <user-account> objects conflicting, again
> > > causing problems.
> > It might not be practical to do so inside the service, but note how
> > this has already become an effort in defensive programming.  There
> > are easier ways to not make this a problem on the configuration
> > level, namely by specifying the same group for both user and group
> > fields.  As far as I see this is even the default state of being if
> > the user is supplied as a string.
> 
> I really don't like the group information being duplicated in both
> the user and a distinct field; it's an awkward API that raises more
> questions than it provides answers, in my opinion (non-intuitive).
And I agree that it's awkward, but I don't agree that this patch solves
the underlying issue.

> One of the reasons I came think this way is because a <user-group>
> can differ by being a system group or not, which would make it easy
> to introduce unexpected, subtle variants.
Is that a serious issue, though?  Yes, two configuration files, one
with (system? #t) and one without will produce different results in
that GIDs are allocated differently, but the same applies to the user
as well.  The only real issue I can think about here goes back to the
handling of duplicate accounts and groups; and again, we both agree
that those ought to be hard errors rather than warnings.

Cheers

bug#63082: [PATCH v3 05/16] services: mpd: Obsolete the 'group' field.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Samstag, dem 06.05.2023 um 22:55 -0400 schrieb Maxim Cournoyer:
>> Hi!
>> 
>> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
>> 
>> > Am Freitag, dem 05.05.2023 um 14:29 -0400 schrieb Maxim Cournoyer:
>> > Didn't we agree in v2 that we want to address this on the account-
>> > service level?  Unless the rest of this series somehow depends on
>> > this patch, I'd rather delay it until we have a proper solution.
>> 
>> I think we agreed the idea to have <user-account> support <user-
>> group> objects for its group field was a good idea that should be
>> implemented, but I declined doing this new work as part of this
>> series :-).
> Indeed, that's how I understood it.  However, I also thought that
> addressing this issue in a later series means we can keep the current
> behaviour until that is done.

My focus on this series was making sure the configuration is easy(er) to
reason with and that it works out of the box for the most part.

>> > > Synchronizing both is not practical, as it can easily lead to
>> > > slightly different <user-account> objects conflicting, again
>> > > causing problems.
>> > It might not be practical to do so inside the service, but note how
>> > this has already become an effort in defensive programming.  There
>> > are easier ways to not make this a problem on the configuration
>> > level, namely by specifying the same group for both user and group
>> > fields.  As far as I see this is even the default state of being if
>> > the user is supplied as a string.
>> 
>> I really don't like the group information being duplicated in both
>> the user and a distinct field; it's an awkward API that raises more
>> questions than it provides answers, in my opinion (non-intuitive).
> And I agree that it's awkward, but I don't agree that this patch solves
> the underlying issue.

It puts the issue aside; if you can't configure a mismatched group, you
can't shoot yourself in the foot.

>> One of the reasons I came think this way is because a <user-group>
>> can differ by being a system group or not, which would make it easy
>> to introduce unexpected, subtle variants.
> Is that a serious issue, though?  Yes, two configuration files, one
> with (system? #t) and one without will produce different results in
> that GIDs are allocated differently, but the same applies to the user
> as well.  The only real issue I can think about here goes back to the
> handling of duplicate accounts and groups; and again, we both agree
> that those ought to be hard errors rather than warnings.

I think it's a serious issue because the permissions configured in the
start slot may be wrong, and the service could fail to run because of
it.

-- 
Thanks,
Maxim

bug#63082: [PATCH v3 05/16] services: mpd: Obsolete the 'group' field.

[Liliana Marie Prikler]

Hi Maxim,

Am Sonntag, dem 07.05.2023 um 14:12 -0400 schrieb Maxim Cournoyer:
> My focus on this series was making sure the configuration is easy(er)
> to reason with and that it works out of the box for the most part.
Obsoleting the group field does imho not significantly ease its use. 
It rather makes its non-ootb use harder, because you now have to edit
two operating-system fields, without changing anything for the ootb
use.

> > > > 
> It puts the issue aside; if you can't configure a mismatched group,
> you can't shoot yourself in the foot.
No, it doesn't: Since it pulls in the groups field into "stuff you need
to worry about when editing your MPD service", it actually exacerbates
the issue.  Yes, the API is awkward, but it does help making mpd-
service-type self-contained.

> > 
> I think it's a serious issue because the permissions configured in
> the start slot may be wrong, and the service could fail to run
> because of it.
What is "it" here: the fact that you can make a group with (system? #f)
or the error in accounts-service-type that has been demoted to a
warning?

Cheers

bug#63082: [PATCH v3 05/16] services: mpd: Obsolete the 'group' field.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Hi Maxim,
>
> Am Sonntag, dem 07.05.2023 um 14:12 -0400 schrieb Maxim Cournoyer:
>> My focus on this series was making sure the configuration is easy(er)
>> to reason with and that it works out of the box for the most part.
> Obsoleting the group field does imho not significantly ease its use.
> It rather makes its non-ootb use harder, because you now have to edit
> two operating-system fields, without changing anything for the ootb
> use.

If you haven't tried that already, I'd like to give you the following
challenge: with the current MPD service, are you able to configure it so
that it works as your user, touching the minimum amount of configuration
switches (as you'd do if you were a new MPD service user getting
started?).

With this series I opinionated on the side that less is better, coming
from the realization that configuring a working MPD was already quite
the challenge (less after this series, if it succeeds at its goal).  In
my opinion, the main two use cases for configuring the services
user/group probably are:

1. you want to run it as an existing user
2. you want it to run as its own user

The defaults cover 2, while for 1 you don't have a need to configure a
group for it, since an existing user will also already have an existing
group (and the <user-account> captures such group).

>> It puts the issue aside; if you can't configure a mismatched group,
>> you can't shoot yourself in the foot.
> No, it doesn't: Since it pulls in the groups field into "stuff you need
> to worry about when editing your MPD service", it actually exacerbates
> the issue.  Yes, the API is awkward, but it does help making mpd-
> service-type self-contained.

The thing is that the 'group' field of mpd-service-type has a default,
which is easy to forget (because it's duplicated from a <user-account>
object and you may reasonably expect the service to default to the
specified user-account's group).  But that's not easy to achieve.  I
tried.

>> I think it's a serious issue because the permissions configured in
>> the start slot may be wrong, and the service could fail to run
>> because of it.
> What is "it" here: the fact that you can make a group with (system? #f)
> or the error in accounts-service-type that has been demoted to a
> warning?

I was thinking about the first one, although 2 would have been a nice
sanity check to avoid ending in a strange state where your existing user
is now in a different group that it ought to, for example.

I hope all this text helps furthering our common understanding :-).

-- 
Thanks,
Maxim

bug#63082: [PATCH v3 05/16] services: mpd: Obsolete the 'group' field.

[Liliana Marie Prikler]

Am Sonntag, dem 07.05.2023 um 21:05 -0400 schrieb Maxim Cournoyer:
> Hi Liliana,
> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > Hi Maxim,
> > 
> > Am Sonntag, dem 07.05.2023 um 14:12 -0400 schrieb Maxim Cournoyer:
> > > My focus on this series was making sure the configuration is
> > > easy(er)
> > > to reason with and that it works out of the box for the most
> > > part.
> > Obsoleting the group field does imho not significantly ease its
> > use.
> > It rather makes its non-ootb use harder, because you now have to
> > edit
> > two operating-system fields, without changing anything for the ootb
> > use.
> 
> If you haven't tried that already, I'd like to give you the following
> challenge: with the current MPD service, are you able to configure it
> so that it works as your user, touching the minimum amount of
> configuration switches (as you'd do if you were a new MPD service
> user getting started?).
I mean, I do have an unfair advantage at this challenge, but:

  (define* (by proc value #:optional (= equal?))
    (lambda (record)
      (= (proc record) value)))

  (service mpd-service-type
           (mpd-configuration
            (user (find (by user-account-name my-user-name) users))
            (group (find (by user-group-name "users") %base-groups))))

If you want, you can also make that a match-lambda.

> With this series I opinionated on the side that less is better,
> coming from the realization that configuring a working MPD was
> already quite the challenge (less after this series, if it succeeds
> at its goal).  In my opinion, the main two use cases for configuring
> the services user/group probably are:
> 
> 1. you want to run it as an existing user
> 2. you want it to run as its own user
> 
> The defaults cover 2, while for 1 you don't have a need to configure
> a group for it, since an existing user will also already have an
> existing group (and the <user-account> captures such group).
Seeing how you write about "the main two use cases", I think either
there is a third use case not mentioned or you are underselling the
extent of the second use case.

> > > It puts the issue aside; if you can't configure a mismatched
> > > group, you can't shoot yourself in the foot.
> > No, it doesn't: Since it pulls in the groups field into "stuff you
> > need to worry about when editing your MPD service", it actually
> > exacerbates the issue.  Yes, the API is awkward, but it does help
> > making mpd-service-type self-contained.
> 
> The thing is that the 'group' field of mpd-service-type has a
> default, which is easy to forget (because it's duplicated from a
> <user-account> object and you may reasonably expect the service to
> default to the specified user-account's group).  But that's not easy
> to achieve.  I tried.
I mean, if you want to serialize the user account's group, by all means
go ahead, but the group field should stay semantically intact for the
case that user:group are provided as strings.  Well, as it turns out,
you don't need to serialize the user or the group to the config file,
so that's not even an issue holding us back.

> > 
Cheers

bug#63082: [PATCH v3 06/16] services: mpd: List log-level in decreasing verbosity order in doc.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-configuration) [log-level]: List log-level in
decreasing verbosity order in doc.
* doc/guix.texi (Audio Services): Update doc.
---
 doc/guix.texi          | 6 +++---
 gnu/services/audio.scm | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index db8f275bf2..200f6d019c 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33616,9 +33616,9 @@ Audio Services
 configuration file.
 
 @item @code{log-level} (type: maybe-string)
-Supress any messages below this threshold.  Available values:
-@code{notice}, @code{info}, @code{verbose}, @code{warning} and
-@code{error}.
+Supress any messages below this threshold.  The available values, in
+decreasing order of verbosity, are: @code{verbose}, @code{info},
+@code{notice}, @code{warning} and @code{error}.
 
 @item @code{music-directory} (type: maybe-string)
 The directory to scan for music files.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 60387272fc..ead4cb8d90 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -409,8 +409,8 @@ (define-configuration mpd-configuration
   (log-level
    maybe-string
    "Supress any messages below this threshold.
-Available values: @code{notice}, @code{info}, @code{verbose},
-@code{warning} and @code{error}.")
+The available values, in decreasing order of verbosity, are: @code{verbose},
+@code{info}, @code{notice}, @code{warning} and @code{error}.")
 
   (music-directory
    maybe-string
-- 
2.39.2

bug#63082: [PATCH v3 07/16] services: mpd; Refactor start slot directory initialization.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-shepherd-service): Standardize the way the log
file parent and other directories are initialized in the start slot.
(mympd-shepherd-service): Likewise.
---
 gnu/services/audio.scm | 132 ++++++++++++++++++++++++-----------------
 1 file changed, 77 insertions(+), 55 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index ead4cb8d90..6e57bf5cba 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -24,6 +24,7 @@ (define-module (gnu services audio)
   #:use-module (guix deprecation)
   #:use-module (guix diagnostics)
   #:use-module (guix i18n)
+  #:use-module (guix modules)
   #:use-module (gnu services)
   #:use-module (gnu services admin)
   #:use-module (gnu services configuration)
@@ -552,36 +553,45 @@ (define (mpd-log-rotation config)
                       (with-shepherd-action 'mpd ('reopen) #f))))))
 
 (define (mpd-shepherd-service config)
-  (match-record config <mpd-configuration> (user package shepherd-requirement
-                                            log-file playlist-directory
-                                            db-file state-file sticker-file
-                                            environment-variables)
+  (match-record config <mpd-configuration>
+    (user package shepherd-requirement
+          log-file playlist-directory
+          db-file state-file sticker-file
+          environment-variables)
     (let ((config-file (mpd-serialize-configuration config))
           (username (user-account-name user)))
       (shepherd-service
        (documentation "Run the MPD (Music Player Daemon)")
        (requirement `(user-processes loopback ,@shepherd-requirement))
        (provision '(mpd))
-       (start #~(begin
-                  (and=> #$(maybe-value log-file)
-                         (compose mkdir-p dirname))
-
-                  (let ((user (getpw #$username)))
-                    (for-each
-                     (lambda (x)
-                       (when (and x (not (file-exists? x)))
-                         (mkdir-p x)
-                         (chown x (passwd:uid user) (passwd:gid user))))
-                     (list #$(maybe-value playlist-directory)
-                           (and=> #$(maybe-value db-file) dirname)
-                           (and=> #$(maybe-value state-file) dirname)
-                           (and=> #$(maybe-value sticker-file) dirname))))
-
-                  (make-forkexec-constructor
-                   (list #$(file-append package "/bin/mpd")
-                         "--no-daemon"
-                         #$config-file)
-                   #:environment-variables '#$environment-variables)))
+       (start
+        (with-imported-modules (source-module-closure
+                                '((gnu build activation)))
+          #~(begin
+              (use-modules (gnu build activation))
+
+              (let ((user (getpw #$username)))
+
+                (define (init-directory directory)
+                  (unless (file-exists? directory)
+                    (mkdir-p/perms directory user #o755)))
+
+                (for-each
+                 init-directory
+                 '#$(map dirname
+                         ;; XXX: Delete the potential "syslog"
+                         ;; log-file value, which is not a directory.
+                         (delete "syslog"
+                                 (filter-map maybe-value
+                                             (list db-file
+                                                   log-file
+                                                   state-file
+                                                   sticker-file))))))
+
+              (make-forkexec-constructor
+               (list #$(file-append package "/bin/mpd") "--no-daemon"
+                     #$config-file)
+               #:environment-variables '#$environment-variables))))
        (stop  #~(make-kill-destructor))
        (actions
         (list (shepherd-configuration-action config-file)
@@ -833,37 +843,49 @@ (define (mympd-serialize-configuration config)
                                        filename-to-field)))))
 
 (define (mympd-shepherd-service config)
-  (match-record config <mympd-configuration> (package shepherd-requirement
-                                              user work-directory
-                                              cache-directory log-level log-to)
-    (let ((log-level* (format #f "MYMPD_LOGLEVEL=~a" log-level))
-          (username (user-account-name user)))
-      (shepherd-service
-       (documentation "Run the myMPD daemon.")
-       (requirement `(loopback user-processes
-                               ,@(if (eq? log-to 'syslog)
-                                     '(syslog)
-                                     '())
-                               ,@shepherd-requirement))
-       (provision '(mympd))
-       (start #~(begin
-                  (let* ((pw (getpwnam #$username))
-                         (uid (passwd:uid pw))
-                         (gid (passwd:gid pw)))
-                    (for-each (lambda (dir)
-                                (mkdir-p dir)
-                                (chown dir uid gid))
-                              (list #$work-directory #$cache-directory)))
-
-                  (make-forkexec-constructor
-                   `(#$(file-append package "/bin/mympd")
-                     "--user" #$username
-                     #$@(if (eq? log-to 'syslog) '("--syslog") '())
-                     "--workdir" #$work-directory
-                     "--cachedir" #$cache-directory)
-                   #:environment-variables (list #$log-level*)
-                   #:log-file #$(if (string? log-to) log-to #f))))
-       (stop #~(make-kill-destructor))))))
+  (match-record config <mympd-configuration>
+    (package shepherd-requirement user work-directory cache-directory
+             log-level log-to)
+    (shepherd-service
+     (documentation "Run the myMPD daemon.")
+     (requirement `(loopback user-processes
+                             ,@(if (eq? log-to 'syslog)
+                                   '(syslog)
+                                   '())
+                             ,@shepherd-requirement))
+     (provision '(mympd))
+     (start
+      (let ((username (user-account-name user)))
+        (with-imported-modules (source-module-closure
+                                '((gnu build activation)))
+          #~(begin
+              (use-modules (gnu build activation))
+
+              (let ((user (getpw #$username)))
+
+                (define (init-directory directory)
+                  (unless (file-exists? directory)
+                    (mkdir-p/perms directory user #o755)))
+
+                (for-each
+                 init-directory
+                 '#$(map dirname
+                         ;; XXX: Delete the potential 'syslog log-file value,
+                         ;; which is not a directory.
+                         (delete 'syslog
+                                 (filter-map maybe-value
+                                             (list log-to
+                                                   work-directory
+                                                   cache-directory))))))
+              (make-forkexec-constructor
+               `(#$(file-append package "/bin/mympd")
+                 "--user" #$username
+                 #$@(if (eq? log-to 'syslog) '("--syslog") '())
+                 "--workdir" #$work-directory
+                 "--cachedir" #$cache-directory)
+               #:environment-variables
+               (list #$(format #f "MYMPD_LOGLEVEL=~a" log-level))
+               #:log-file #$(if (string? log-to) log-to #f)))))))))
 
 (define (mympd-accounts config)
   (match-record config <mympd-configuration>
-- 
2.39.2

bug#63082: [PATCH v3 07/16] services: mpd; Refactor start slot directory initialization.

[Bruno Victal]

On 2023-05-05 19:29, Maxim Cournoyer wrote:
> -                  (make-forkexec-constructor
> -                   (list #$(file-append package "/bin/mpd")
> -                         "--no-daemon"
> -                         #$config-file)
> -                   #:environment-variables '#$environment-variables)))
> +       (start
> +        (with-imported-modules (source-module-closure
> +                                '((gnu build activation)))

How about adding '(gnu build activation) into %default-imported-modules
(and %default-modules) at gnu/services/shepherd.scm?
Services should be using the start field to perform these kinds of tasks
anyways. (rather than extend activation-service-type which is incorrect use)

> +          #~(begin
> +              (use-modules (gnu build activation))

In general, rather than #~(begin (use-modules ...)), it's preferred to specify
additional modules using the 'modules' field e.g.

--8<---------------cut here---------------start------------->8---
(modules (cons '(gnu build activation)
                %default-modules))
--8<---------------cut here---------------end--------------->8---

> +
> +              (let ((user (getpw #$username)))
> +
> +                (define (init-directory directory)
> +                  (unless (file-exists? directory)
> +                    (mkdir-p/perms directory user #o755)))
> +
> +                (for-each
> +                 init-directory
> +                 '#$(map dirname
> +                         ;; XXX: Delete the potential "syslog"
> +                         ;; log-file value, which is not a directory.
> +                         (delete "syslog"
> +                                 (filter-map maybe-value
> +                                             (list db-file
> +                                                   log-file
> +                                                   state-file
> +                                                   sticker-file))))))

Perhaps treat “syslog” as a symbol instead?
Strings seem more adequate when the value is a path, with a symbol
being a sign that the value is to be treated “specially”.
(this aligns with how mympd handles this)


-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.

bug#63082: mpd defaul configuration does not work ('No database' error)

[Maxim Cournoyer]

Hi,

Bruno Victal <mirai@makinata.eu> writes:

> On 2023-05-05 19:29, Maxim Cournoyer wrote:
>> -                  (make-forkexec-constructor
>> -                   (list #$(file-append package "/bin/mpd")
>> -                         "--no-daemon"
>> -                         #$config-file)
>> -                   #:environment-variables '#$environment-variables)))
>> +       (start
>> +        (with-imported-modules (source-module-closure
>> +                                '((gnu build activation)))
>
> How about adding '(gnu build activation) into %default-imported-modules
> (and %default-modules) at gnu/services/shepherd.scm?
> Services should be using the start field to perform these kinds of tasks
> anyways. (rather than extend activation-service-type which is incorrect use)

I think that's something to discuss outside the scope of this series,
which is already a bit unwieldy :-).  I think originally they were put
there because adding them to (guix build utils) would entail a world
rebuild.

>> +          #~(begin
>> +              (use-modules (gnu build activation))
>
> In general, rather than #~(begin (use-modules ...)), it's preferred to specify
> additional modules using the 'modules' field e.g.

I prefer to keep it the way it is, because the use-modules works in
tandem with the with-imported-modules.  I'd also have to wrap the stop
procedure in a second with-imported-modules if I used the 'global'
modules field, since it applies to both.

> (modules (cons '(gnu build activation)
>                 %default-modules))
>
>> +
>> +              (let ((user (getpw #$username)))
>> +
>> +                (define (init-directory directory)
>> +                  (unless (file-exists? directory)
>> +                    (mkdir-p/perms directory user #o755)))
>> +
>> +                (for-each
>> +                 init-directory
>> +                 '#$(map dirname
>> +                         ;; XXX: Delete the potential "syslog"
>> +                         ;; log-file value, which is not a directory.
>> +                         (delete "syslog"
>> +                                 (filter-map maybe-value
>> +                                             (list db-file
>> +                                                   log-file
>> +                                                   state-file
>> +                                                   sticker-file))))))
>
> Perhaps treat “syslog” as a symbol instead?
> Strings seem more adequate when the value is a path, with a symbol
> being a sign that the value is to be treated “specially”.
> (this aligns with how mympd handles this)

There no longer is a delete "syslog" in the code, reworked in "services:
mpd: Log to syslog by default."

-- 
Thanks,
Maxim

bug#63082: [PATCH v3 08/16] services: mpd: Log to syslog by default.

[Maxim Cournoyer]

Rationale: the tristate value was awkward to deal with, the default log file
name was odd (/var/log/mpd/log) and it required special attention to create
the 'mpd' parent directory as root and chowning it to the MPD user.  It also
didn't match the default behavior of MPD, which is to log to systemd or syslog
unless a log file is specified.

* gnu/services/audio.scm (mpd-log-file-sanitizer): New procedure.
(mpd-configuration) [log-file]: Remove default maybe value.  Add sanitizer.
(mpd-shepherd-service): Validate the log file parent directory exists and has
the right permissions.  Conditionally add syslogd to requirements.
(mympd-log-to-sanitizer): New procedure.
(mympd-configuration) [log-to]: Change type to maybe-string.  Update doc and
add sanitizer.
(mympd-shepherd-service) [requirement]: Fix to use syslogd.  Adjust
accordingly.
[start] Adjust accordingly.
(mympd-log-rotation): Check log-to via maybe-value-set?.
* doc/guix.texi (Audio Services): Update doc.
---
 doc/guix.texi          | 17 +++++-----
 gnu/services/audio.scm | 74 ++++++++++++++++++++++++++++--------------
 2 files changed, 57 insertions(+), 34 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 200f6d019c..253b8f113b 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33610,10 +33610,10 @@ Audio Services
 @item @code{environment-variables} (default: @code{("PULSE_CLIENTCONFIG=/etc/pulse/client.conf" "PULSE_CONFIG=/etc/pulse/daemon.conf")}) (type: list-of-strings)
 A list of strings specifying environment variables.
 
-@item @code{log-file} (default: @code{"/var/log/mpd/log"}) (type: maybe-string)
-The location of the log file.  Set to @code{syslog} to use the local
-syslog daemon or @code{%unset-value} to omit this directive from the
-configuration file.
+@item @code{log-file} (type: maybe-string)
+The location of the log file.  Unless specified, logs are sent to the
+local syslog daemon.  Alternatively, a log file name can be specified,
+for example @file{/var/log/mpd.log}.
 
 @item @code{log-level} (type: maybe-string)
 Supress any messages below this threshold.  The available values, in
@@ -33884,11 +33884,10 @@ Audio Services
 How much detail to include in logs, possible values: @code{0} to
 @code{7}.
 
-@item @code{log-to} (default: @code{"/var/log/mympd/log"}) (type: string-or-symbol)
-Where to send logs.  By default, the service logs to
-@file{/var/log/mympd.log}.  The alternative is @code{'syslog}, which
-sends output to the running syslog service under the @samp{daemon}
-facility.
+@item @code{log-to} (type: maybe-string)
+Where to send logs.  Unless specified, the service logs to the local
+syslog service under the @samp{daemon} facility.  Alternatively, a log
+file name can be specified, for example @file{/var/log/mympd.log}.
 
 @item @code{lualibs} (default: @code{"all"}) (type: maybe-string)
 See
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 6e57bf5cba..c1295837b6 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -229,6 +229,18 @@ (define (mpd-group-sanitizer value)
     (warning (G_ "'group' in <mpd-configuration> is obsolete; ignoring~%")))
   #f)
 
+(define (mpd-log-file-sanitizer value)
+  (match value
+    (%unset-value
+     ;; XXX: While leaving the 'sys_log' option out of the mpd.conf file is
+     ;; supposed to cause logging to happen via systemd (elogind provides a
+     ;; compatible interface), this doesn't work (nothing gets logged); use
+     ;; syslog instead.
+     "syslog")
+    ((? string?)
+     value)
+    (_ (configuration-field-error #f 'log-file value))))
+
 ;;;
 
 ;; Generic MPD plugin record, lists only the most prevalent fields.
@@ -402,10 +414,11 @@ (define-configuration mpd-configuration
    empty-serializer)
 
   (log-file
-   (maybe-string "/var/log/mpd/log")
-   "The location of the log file. Set to @code{syslog} to use the
-local syslog daemon or @code{%unset-value} to omit this directive
-from the configuration file.")
+   maybe-string
+   "The location of the log file.  Unless specified, logs are sent to the
+local syslog daemon.  Alternatively, a log file name can be specified, for
+example @file{/var/log/mpd.log}."
+   (sanitizer mpd-log-file-sanitizer))
 
   (log-level
    maybe-string
@@ -562,7 +575,11 @@ (define (mpd-shepherd-service config)
           (username (user-account-name user)))
       (shepherd-service
        (documentation "Run the MPD (Music Player Daemon)")
-       (requirement `(user-processes loopback ,@shepherd-requirement))
+       (requirement `(user-processes loopback
+                                     ,@(if (string=? "syslog" log-file)
+                                           '(syslogd)
+                                           '())
+                                     ,@shepherd-requirement))
        (provision '(mpd))
        (start
         (with-imported-modules (source-module-closure
@@ -683,6 +700,15 @@ (define (mympd-group-sanitizer value)
     (warning (G_ "'group' in <mympd-configuration> is obsolete; ignoring~%")))
   #f)
 
+(define (mympd-log-to-sanitizer value)
+  (match value
+    ('syslog
+     (warning (G_ "syslog symbol value for 'log-to' is deprecated~%"))
+     %unset-value)
+    ((or %unset-value (? string?))
+     value)
+    (_ (configuration-field-error #f 'log-to value))))
+
 ;;;
 
 
@@ -749,10 +775,11 @@ (define-configuration/no-serialization mympd-configuration
    "How much detail to include in logs, possible values: @code{0} to @code{7}.")
 
   (log-to
-   (string-or-symbol "/var/log/mympd/log")
-   "Where to send logs. By default, the service logs to
-@file{/var/log/mympd.log}. The alternative is @code{'syslog}, which
-sends output to the running syslog service under the @samp{daemon} facility."
+   maybe-string
+   "Where to send logs.  Unless specified, the service logs to the local
+syslog service under the @samp{daemon} facility.  Alternatively, a log file
+name can be specified, for example @file{/var/log/mympd.log}."
+   (sanitizer mympd-log-to-sanitizer)
    empty-serializer)
 
   (lualibs
@@ -849,9 +876,9 @@ (define (mympd-shepherd-service config)
     (shepherd-service
      (documentation "Run the myMPD daemon.")
      (requirement `(loopback user-processes
-                             ,@(if (eq? log-to 'syslog)
-                                   '(syslog)
-                                   '())
+                             ,@(if (maybe-value-set? log-to)
+                                   '()
+                                   '(syslogd))
                              ,@shepherd-requirement))
      (provision '(mympd))
      (start
@@ -867,16 +894,12 @@ (define (mympd-shepherd-service config)
                   (unless (file-exists? directory)
                     (mkdir-p/perms directory user #o755)))
 
-                (for-each
-                 init-directory
-                 '#$(map dirname
-                         ;; XXX: Delete the potential 'syslog log-file value,
-                         ;; which is not a directory.
-                         (delete 'syslog
-                                 (filter-map maybe-value
-                                             (list log-to
-                                                   work-directory
-                                                   cache-directory))))))
+                (for-each init-directory
+                          '#$(map dirname (filter-map maybe-value
+                                                      (list log-to
+                                                            work-directory
+                                                            cache-directory)))))
+
               (make-forkexec-constructor
                `(#$(file-append package "/bin/mympd")
                  "--user" #$username
@@ -885,7 +908,7 @@ (define (mympd-shepherd-service config)
                  "--cachedir" #$cache-directory)
                #:environment-variables
                (list #$(format #f "MYMPD_LOGLEVEL=~a" log-level))
-               #:log-file #$(if (string? log-to) log-to #f)))))))))
+               #:log-file #$(maybe-value log-to)))))))))
 
 (define (mympd-accounts config)
   (match-record config <mympd-configuration>
@@ -893,8 +916,9 @@ (define (mympd-accounts config)
     (list user)))
 
 (define (mympd-log-rotation config)
-  (match-record config <mympd-configuration> (log-to)
-    (if (string? log-to)
+  (match-record config <mympd-configuration>
+    (log-to)
+    (if (maybe-value-set? log-to)
         (list (log-rotation
                (files (list log-to))))
         '())))
-- 
2.39.2

bug#63082: [PATCH v3 09/16] services: mpd: Do not rotate logs when using syslog.

[Maxim Cournoyer]

* gnu/services/audio.scm (mpd-log-rotation): Conditionlize based on the value
of LOG-FILE.
---
 gnu/services/audio.scm | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index c1295837b6..7fb4b8ccf7 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -558,12 +558,15 @@ (define (mpd-serialize-configuration configuration)
    (serialize-configuration configuration mpd-configuration-fields)))
 
 (define (mpd-log-rotation config)
-  (match-record config <mpd-configuration> (log-file)
-    (log-rotation
-     (files (list log-file))
-     (post-rotate #~(begin
-                      (use-modules (gnu services herd))
-                      (with-shepherd-action 'mpd ('reopen) #f))))))
+  (match-record config <mpd-configuration>
+    (log-file)
+    (if (string=? "syslog" log-file)
+        '()                             ;nothing to do
+        (list (log-rotation
+               (files (list log-file))
+               (post-rotate #~(begin
+                                (use-modules (gnu services herd))
+                                (with-shepherd-action 'mpd ('reopen) #f))))))))
 
 (define (mpd-shepherd-service config)
   (match-record config <mpd-configuration>
-- 
2.39.2

bug#63082: [PATCH v3 10/16] services: mpd: Let Shepherd effect the user/group change.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

Quoting a MPD developer, regarding MPD's feature to switch user itself:
"that's legacy for the dark ages when proper service managers did not exist"
:-).

* gnu/services/audio.scm (mpd-serialize-user-account)
(mpd-serialize-user-group): Delete procedures.
* gnu/services/audio.scm (mpd-configuration) [user]: Do not serialize.
[group]: Likewise.
(mpd-shepherd-service): Provide the #:user, #:group and #:supplementary-groups
arguments.
(mympd-shepherd-service): Likewise, and remove the '--user' argument.
* doc/guix.texi (Audio Services): Update doc.
(mympd-configuration) [port]: Change default value to 8080.
[ssl-port]: Change default value to 443.
* gnu/tests/audio.scm (run-mympd-test): Adjust accordingly.
---
 doc/guix.texi          | 12 +++++-----
 gnu/services/audio.scm | 52 +++++++++++++++++++++++++-----------------
 gnu/tests/audio.scm    |  4 ++--
 3 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 253b8f113b..cdc1f4dedc 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33598,7 +33598,7 @@ Audio Services
 The MPD package.
 
 @item @code{user} (type: user-account)
-The user to run mpd as.
+The user to run @command{mpd} as.
 
 @item @code{group} (default: @code{#f}) (type: boolean)
 Obsolete.  Do not use.
@@ -33642,7 +33642,7 @@ Audio Services
 The location of the sticker database.
 
 @item @code{default-port} (default: @code{6600}) (type: maybe-port)
-The default port to run mpd on.
+The default port to run @command{mpd} on.
 
 @item @code{endpoints} (type: maybe-list-of-strings)
 The addresses that mpd will bind to.  A port different from
@@ -33827,13 +33827,13 @@ Audio Services
 @uref{https://jcorporation.github.io/myMPD/, myMPD} is a web server
 frontend for MPD that provides a mobile friendly web client for MPD.
 
-The following example shows a myMPD instance listening on port 80,
+The following example shows a myMPD instance listening on port 8080,
 with album cover caching disabled.
 
 @lisp
 (service mympd-service-type
          (mympd-configuration
-          (port 80)
+          (port 8080)
           (covercache-ttl 0)))
 @end lisp
 
@@ -33877,7 +33877,7 @@ Audio Services
 @item @code{host} (default: @code{"[::]"}) (type: string)
 Host name to listen on.
 
-@item @code{port} (default: @code{80}) (type: maybe-port)
+@item @code{port} (default: @code{8080}) (type: maybe-port)
 HTTP port to listen on.
 
 @item @code{log-level} (default: @code{5}) (type: integer)
@@ -33903,7 +33903,7 @@ Audio Services
 @item @code{ssl?} (default: @code{#f}) (type: boolean)
 SSL/TLS support.
 
-@item @code{ssl-port} (default: @code{443}) (type: maybe-port)
+@item @code{ssl-port} (default: @code{4443}) (type: maybe-port)
 Port to listen for HTTPS.
 
 @item @code{ssl-cert} (type: maybe-string)
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 7fb4b8ccf7..f470ca20e0 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2019 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2022⁠–⁠2023 Bruno Victal <mirai@makinata.eu>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -165,9 +166,6 @@ (define mpd-serialize-boolean mpd-serialize-field)
 (define (mpd-serialize-list-of-strings field-name value)
   #~(string-append #$@(map (cut mpd-serialize-string field-name <>) value)))
 
-(define (mpd-serialize-user-account field-name value)
-  (mpd-serialize-string field-name (user-account-name value)))
-
 (define-maybe string (prefix mpd-))
 (define-maybe list-of-strings (prefix mpd-))
 (define-maybe boolean (prefix mpd-))
@@ -390,10 +388,14 @@ (define-configuration mpd-configuration
    "The MPD package."
    empty-serializer)
 
+  ;; Note: The user and its group are not serialized, otherwise MPD would
+  ;; attempt to switch the user/group itself.  The task of switching the
+  ;; user/group is left to Shepherd instead.
   (user
    (user-account %mpd-user)
-   "The user to run mpd as."
-   (sanitizer mpd-user-sanitizer))
+   "The user to run @command{mpd} as."
+   (sanitizer mpd-user-sanitizer)
+   empty-serializer)
 
   (group
    (boolean #f)
@@ -458,7 +460,7 @@ (define-configuration mpd-configuration
 
   (default-port
    (maybe-port 6600)
-   "The default port to run mpd on.")
+   "The default port to run @command{mpd} on.")
 
   (endpoints
    maybe-list-of-strings
@@ -611,7 +613,11 @@ (define (mpd-shepherd-service config)
               (make-forkexec-constructor
                (list #$(file-append package "/bin/mpd") "--no-daemon"
                      #$config-file)
-               #:environment-variables '#$environment-variables))))
+               #:environment-variables '#$environment-variables
+               #:user #$username
+               #:group #$(user-account-group user)
+               #:supplementary-groups
+               '#$(user-account-supplementary-groups user)))))
        (stop  #~(make-kill-destructor))
        (actions
         (list (shepherd-configuration-action config-file)
@@ -654,7 +660,7 @@ (define mpd-service-type
           (service-extension account-service-type
                              mpd-accounts)
           (service-extension rottlog-service-type
-                             (compose list mpd-log-rotation))))
+                             mpd-log-rotation)))
    (default-value (mpd-configuration))))
 
 \f
@@ -770,7 +776,7 @@ (define-configuration/no-serialization mympd-configuration
    "Host name to listen on.")
 
   (port
-   (maybe-port 80)
+   (maybe-port 8080)
    "HTTP port to listen on.")
 
   (log-level
@@ -805,7 +811,7 @@ (define-configuration/no-serialization mympd-configuration
    "SSL/TLS support.")
 
   (ssl-port
-   (maybe-port 443)
+   (maybe-port 4443)
    "Port to listen for HTTPS.")
 
   (ssl-cert
@@ -901,17 +907,21 @@ (define (mympd-shepherd-service config)
                           '#$(map dirname (filter-map maybe-value
                                                       (list log-to
                                                             work-directory
-                                                            cache-directory)))))
-
-              (make-forkexec-constructor
-               `(#$(file-append package "/bin/mympd")
-                 "--user" #$username
-                 #$@(if (eq? log-to 'syslog) '("--syslog") '())
-                 "--workdir" #$work-directory
-                 "--cachedir" #$cache-directory)
-               #:environment-variables
-               (list #$(format #f "MYMPD_LOGLEVEL=~a" log-level))
-               #:log-file #$(maybe-value log-to)))))))))
+                                                            cache-directory))))
+
+                (make-forkexec-constructor
+                 `(#$(file-append package "/bin/mympd")
+                   "--user" #$username
+                   #$@(if (eq? log-to 'syslog) '("--syslog") '())
+                   "--workdir" #$work-directory
+                   "--cachedir" #$cache-directory)
+                 #:environment-variables
+                 (list #$(format #f "MYMPD_LOGLEVEL=~a" log-level))
+                 #:log-file #$(maybe-value log-to)
+                 #:user #$username
+                 #:group #$(user-account-group user)
+                 #:supplementary-groups
+                 '#$(user-account-supplementary-groups user))))))))))
 
 (define (mympd-accounts config)
   (match-record config <mympd-configuration>
diff --git a/gnu/tests/audio.scm b/gnu/tests/audio.scm
index acb91293e8..efa07b5ba9 100644
--- a/gnu/tests/audio.scm
+++ b/gnu/tests/audio.scm
@@ -89,7 +89,7 @@ (define (run-mympd-test)
   (define vm
     (virtual-machine
      (operating-system os)
-     (port-forwardings '((8080 . 80)))))
+     (port-forwardings '((8080 . 8080)))))
 
   (define test
     (with-imported-modules '((gnu build marionette))
@@ -113,7 +113,7 @@ (define (run-mympd-test)
                              marionette))
 
           (test-assert "HTTP port ready"
-            (wait-for-tcp-port 80 marionette))
+            (wait-for-tcp-port 8080 marionette))
 
           (test-equal "http-head"
             200
-- 
2.39.2

bug#63082: [PATCH v3 10/16] services: mpd: Let Shepherd effect the user/group change.

[Bruno Victal]

On 2023-05-05 19:29, Maxim Cournoyer wrote:
> Relates to <https://issues.guix.gnu.org/63082>.
> 
> Quoting a MPD developer, regarding MPD's feature to switch user itself:
> "that's legacy for the dark ages when proper service managers did not exist"
> :-).
> 
> * gnu/services/audio.scm (mpd-serialize-user-account)
> (mpd-serialize-user-group): Delete procedures.
> * gnu/services/audio.scm (mpd-configuration) [user]: Do not serialize.
> [group]: Likewise.
> (mpd-shepherd-service): Provide the #:user, #:group and #:supplementary-groups
> arguments.
> (mympd-shepherd-service): Likewise, and remove the '--user' argument.
> * doc/guix.texi (Audio Services): Update doc.
> (mympd-configuration) [port]: Change default value to 8080.
> [ssl-port]: Change default value to 443.
> * gnu/tests/audio.scm (run-mympd-test): Adjust accordingly.
> ---
>  doc/guix.texi          | 12 +++++-----
>  gnu/services/audio.scm | 52 +++++++++++++++++++++++++-----------------
>  gnu/tests/audio.scm    |  4 ++--
>  3 files changed, 39 insertions(+), 29 deletions(-)

This contains a submarine change that isn't easily spotted from the
commit message, that mympd is getting its default port changed and that
it can no longer bind to privileged ports, since although mympd can
start as root in order to bind to possibly privileged ports, it will
explicitly refuse to continue running as root afterwards.

I think we can have shepherd effect for mympd, but only if (and after)
shepherd gets support for POSIX capabilities (CAP_NET_BIND_SERVICE) or
a suitable way to specify that “yes, the program invoked by the service
should have CAP_NET_BIND_SERVICE” is provided.


-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.

bug#63082: mpd defaul configuration does not work ('No database' error)

[Maxim Cournoyer]

Hi Bruno,

Bruno Victal <mirai@makinata.eu> writes:

> On 2023-05-05 19:29, Maxim Cournoyer wrote:
>> Relates to <https://issues.guix.gnu.org/63082>.
>> 
>> Quoting a MPD developer, regarding MPD's feature to switch user itself:
>> "that's legacy for the dark ages when proper service managers did not exist"
>> :-).
>> 
>> * gnu/services/audio.scm (mpd-serialize-user-account)
>> (mpd-serialize-user-group): Delete procedures.
>> * gnu/services/audio.scm (mpd-configuration) [user]: Do not serialize.
>> [group]: Likewise.
>> (mpd-shepherd-service): Provide the #:user, #:group and #:supplementary-groups
>> arguments.
>> (mympd-shepherd-service): Likewise, and remove the '--user' argument.
>> * doc/guix.texi (Audio Services): Update doc.
>> (mympd-configuration) [port]: Change default value to 8080.
>> [ssl-port]: Change default value to 443.
>> * gnu/tests/audio.scm (run-mympd-test): Adjust accordingly.
>> ---
>>  doc/guix.texi          | 12 +++++-----
>>  gnu/services/audio.scm | 52 +++++++++++++++++++++++++-----------------
>>  gnu/tests/audio.scm    |  4 ++--
>>  3 files changed, 39 insertions(+), 29 deletions(-)
>
> This contains a submarine change that isn't easily spotted from the
> commit message, that mympd is getting its default port changed and that
> it can no longer bind to privileged ports, since although mympd can
> start as root in order to bind to possibly privileged ports, it will
> explicitly refuse to continue running as root afterwards.
>
> I think we can have shepherd effect for mympd, but only if (and after)
> shepherd gets support for POSIX capabilities (CAP_NET_BIND_SERVICE) or
> a suitable way to specify that “yes, the program invoked by the service
> should have CAP_NET_BIND_SERVICE” is provided.

OK.  I've dropped the change so as to not block the rest of the series.

-- 
Thanks,
Maxim

bug#63082: [PATCH v3 10/16] services: mpd: Let Shepherd effect the user/group change.

[Maxim Cournoyer]

Hello,

Bruno Victal <mirai@makinata.eu> writes:

> On 2023-05-05 19:29, Maxim Cournoyer wrote:
>> Relates to <https://issues.guix.gnu.org/63082>.
>> 
>> Quoting a MPD developer, regarding MPD's feature to switch user itself:
>> "that's legacy for the dark ages when proper service managers did not exist"
>> :-).
>> 
>> * gnu/services/audio.scm (mpd-serialize-user-account)
>> (mpd-serialize-user-group): Delete procedures.
>> * gnu/services/audio.scm (mpd-configuration) [user]: Do not serialize.
>> [group]: Likewise.
>> (mpd-shepherd-service): Provide the #:user, #:group and #:supplementary-groups
>> arguments.
>> (mympd-shepherd-service): Likewise, and remove the '--user' argument.
>> * doc/guix.texi (Audio Services): Update doc.
>> (mympd-configuration) [port]: Change default value to 8080.
>> [ssl-port]: Change default value to 443.
>> * gnu/tests/audio.scm (run-mympd-test): Adjust accordingly.
>> ---
>>  doc/guix.texi          | 12 +++++-----
>>  gnu/services/audio.scm | 52 +++++++++++++++++++++++++-----------------
>>  gnu/tests/audio.scm    |  4 ++--
>>  3 files changed, 39 insertions(+), 29 deletions(-)
>
> This contains a submarine change that isn't easily spotted from the
> commit message, that mympd is getting its default port changed and that
> it can no longer bind to privileged ports, since although mympd can
> start as root in order to bind to possibly privileged ports, it will
> explicitly refuse to continue running as root afterwards.
>
> I think we can have shepherd effect for mympd, but only if (and after)
> shepherd gets support for POSIX capabilities (CAP_NET_BIND_SERVICE) or
> a suitable way to specify that “yes, the program invoked by the service
> should have CAP_NET_BIND_SERVICE” is provided.

As mentioned before, I've let go of this commit for now (though that
means supplementary-groups on a user-account are not honored anymore)
and other commits touching the current group mechanism until we've
implemented support for POSIX capabilities as mentioned in
https://issues.guix.gnu.org/64862.

We can thus close this issue for now, keeping on mind that some bits
could be salvaged at a later time when 64862 is done.

-- 
Thanks,
Maxim

bug#63082: [PATCH v3 11/16] system: accounts: Export <user-account>.

[Maxim Cournoyer]

---
 gnu/system/accounts.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gnu/system/accounts.scm b/gnu/system/accounts.scm
index 586cff1842..e37b733c6d 100644
--- a/gnu/system/accounts.scm
+++ b/gnu/system/accounts.scm
@@ -19,7 +19,8 @@
 (define-module (gnu system accounts)
   #:use-module (guix records)
   #:use-module (ice-9 match)
-  #:export (user-account
+  #:export (<user-account>
+            user-account
             user-account?
             user-account-name
             user-account-password
-- 
2.39.2

bug#63082: [PATCH v3 12/16] services: mpd: Warn when the MPD user is not in the "audio" group.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org>.

* gnu/services/audio.scm (%mpd-user) [group]: Add comment.
(mpd-user-sanitizer): Warn if the MPD user is not in the audio group.
---
 gnu/services/audio.scm | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index f470ca20e0..7040a63ecd 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -31,6 +31,7 @@ (define-module (gnu services audio)
   #:use-module (gnu services configuration)
   #:use-module (gnu services shepherd)
   #:use-module (gnu services admin)
+  #:use-module (gnu system accounts)
   #:use-module (gnu system shadow)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages mpd)
@@ -173,6 +174,8 @@ (define-maybe boolean (prefix mpd-))
 (define %mpd-user
   (user-account
    (name "mpd")
+   ;; Being in the audio group ensures that PulseAudio can access sound
+   ;; devices.
    (group "audio")
    (system? #t)
    (comment "Music Player Daemon (MPD) user")
@@ -209,10 +212,17 @@ (define (mpd-serialize-port field-name value)
 
 (define-maybe port (prefix mpd-))
 
-;;; Procedures for unsupported value types, to be removed.
-
+;;; Sanitizer procedures.
 (define (mpd-user-sanitizer value)
-  (cond ((user-account? value) value)
+  (cond ((user-account? value)
+         (match-record value <user-account>
+           (group supplementary-groups)
+           (unless (or (string=? "audio" group)
+                       (member "audio" supplementary-groups))
+             ;; Being in the "audio" group is necessary for access to the
+             ;; sound devices.
+             (warning (G_ "mpd user not member of \"audio\" group~%"))))
+         value)
         ((string? value)
          (warning (G_ "string value for 'user' is deprecated, use \
 user-account instead~%"))
-- 
2.39.2

bug#63082: [PATCH v3 13/16] services: mpd: Auto-detect mpd-output mixer type by default.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

* gnu/services/audio.scm (mpd-output) [mixer-type]: Change default value from
"none" to unspecified.
* doc/guix.texi (Audio Services): Regenerate doc.
---
 doc/guix.texi          | 11 +++++++----
 gnu/services/audio.scm | 15 +++++++++------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index cdc1f4dedc..0981aa1568 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33739,8 +33739,9 @@ Audio Services
 @end table
 @end deftp
 
+@c %start of fragment
 @deftp {Data Type} mpd-output
-Data type representing a @command{mpd} audio output.
+Available @code{mpd-output} fields are:
 
 @table @asis
 @item @code{name} (default: @code{"MPD"}) (type: string)
@@ -33767,15 +33768,16 @@ Audio Services
 
 @item @code{always-on?} (default: @code{#f}) (type: boolean)
 If set to @code{#t}, then MPD attempts to keep this audio output always
-open.  This may be useful for streaming servers, when you don?t want to
+open.  This may be useful for streaming servers, when you don’t want to
 disconnect all listeners even when playback is accidentally stopped.
 
-@item @code{mixer-type} (default: @code{"none"}) (type: string)
+@item @code{mixer-type} (type: maybe-string)
 This field accepts a string that specifies which mixer should be used
 for this audio output: the @code{hardware} mixer, the @code{software}
 mixer, the @code{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 (@code{none}).
+External Mixer) or no mixer (@code{none}).  When left unspecified, a
+@code{hardware} mixer is used for devices that support it.
 
 @item @code{replay-gain-handler} (type: maybe-string)
 This field accepts a string that specifies how
@@ -33790,6 +33792,7 @@ Audio Services
 
 @end table
 @end deftp
+@c %end of fragment
 
 The following example shows a configuration of @command{mpd} that
 configures some of its plugins and provides a HTTP audio streaming output.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 7040a63ecd..1e0a8b7f9e 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -346,15 +346,18 @@ (define-configuration mpd-output
 disconnect all listeners even when playback is accidentally stopped.")
 
   (mixer-type
-   (string "none")
-   "This field accepts a string that specifies which mixer should be used
-for this audio output: the @code{hardware} mixer, the @code{software}
-mixer, the @code{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 (@code{none})."
+   maybe-string
+   "This field accepts a string that specifies which mixer should be used for
+this audio output: the @code{hardware} mixer, the @code{software} mixer, the
+@code{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 (@code{none}).  When left unspecified, a @code{hardware} mixer is used
+for devices that support it."
    (sanitizer
     (lambda (x)  ; TODO: deprecated, remove me later.
       (cond
+       ((eq? %unset-value x)
+        x)
        ((symbol? x)
         (warning (G_ "symbol value for 'mixer-type' is deprecated, \
 use string instead~%"))
-- 
2.39.2

bug#63082: [PATCH v3 14/16] services: mpd: Provision a default cache directory and set HOME.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

* gnu/services/audio.scm (mpd-shepherd-service): Create a default .cache
directory.  Use mkdir-p/perms and refactor loop.  Set the HOME environment
variables.
---
 doc/guix.texi          |  3 +-
 gnu/services/audio.scm | 63 +++++++++++++++++++++++++-----------------
 2 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 0981aa1568..04744498dd 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33633,7 +33633,8 @@ Audio Services
 The directory to store playlists.
 
 @item @code{db-file} (type: maybe-string)
-The location of the music database.
+The location of the music database.  When left unspecified,
+@file{~/.cache/db} is used.
 
 @item @code{state-file} (type: maybe-string)
 The location of the file that stores current MPD's state.
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index 1e0a8b7f9e..e242f48ba0 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -461,7 +461,8 @@ (define-configuration mpd-configuration
 
   (db-file
    maybe-string
-   "The location of the music database.")
+   "The location of the music database.  When left unspecified,
+@file{~/.cache/db} is used.")
 
   (state-file
    maybe-string
@@ -605,32 +606,42 @@ (define (mpd-shepherd-service config)
           #~(begin
               (use-modules (gnu build activation))
 
-              (let ((user (getpw #$username)))
-
-                (define (init-directory directory)
-                  (unless (file-exists? directory)
-                    (mkdir-p/perms directory user #o755)))
+              (let ((home #$(user-account-home-directory user)))
+                (let ((user (getpw #$username))
+                      (default-cache-dir (string-append home "/.cache")))
+
+                  (define (init-directory directory)
+                    (unless (file-exists? directory)
+                      (mkdir-p/perms directory user #o755)))
+
+                  ;; Define a cache location that can be automatically used
+                  ;; for the database file, in case it hasn't been explicitly
+                  ;; specified.
+                  (for-each
+                   init-directory
+                   (cons default-cache-dir
+                         '#$(map dirname
+                                 ;; XXX: Delete the potential "syslog"
+                                 ;; log-file value, which is not a directory.
+                                 (delete "syslog"
+                                         (filter-map maybe-value
+                                                     (list db-file
+                                                           log-file
+                                                           state-file
+                                                           sticker-file)))))))
 
-                (for-each
-                 init-directory
-                 '#$(map dirname
-                         ;; XXX: Delete the potential "syslog"
-                         ;; log-file value, which is not a directory.
-                         (delete "syslog"
-                                 (filter-map maybe-value
-                                             (list db-file
-                                                   log-file
-                                                   state-file
-                                                   sticker-file))))))
-
-              (make-forkexec-constructor
-               (list #$(file-append package "/bin/mpd") "--no-daemon"
-                     #$config-file)
-               #:environment-variables '#$environment-variables
-               #:user #$username
-               #:group #$(user-account-group user)
-               #:supplementary-groups
-               '#$(user-account-supplementary-groups user)))))
+                (make-forkexec-constructor
+                 (list #$(file-append package "/bin/mpd") "--no-daemon"
+                       #$config-file)
+                 #:environment-variables
+                 ;; Set HOME so MPD can infer default paths, such as
+                 ;; for the database file.
+                 (cons (string-append "HOME=" home)
+                       '#$environment-variables)
+                 #:user #$username
+                 #:group #$(user-account-group user)
+                 #:supplementary-groups
+                 '#$(user-account-supplementary-groups user))))))
        (stop  #~(make-kill-destructor))
        (actions
         (list (shepherd-configuration-action config-file)
-- 
2.39.2

bug#63082: [PATCH v3 15/16] services: mpd: Update basic example.

[Maxim Cournoyer]

Relates to <https://issues.guix.gnu.org/63082>.

* doc/guix.texi (Audio Services): Do not use a deprecated user form; keep the
default one.  Remove port.  Specify a music-directory.  Mention the importance
of permissions on the music directory.
---
 doc/guix.texi | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 04744498dd..207001c550 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33565,16 +33565,27 @@ Audio Services
 being controlled from the local machine or over the network by a variety
 of clients.
 
-The following example shows how one might run @code{mpd} as user
-@code{"bob"} on port @code{6666}.  It uses pulseaudio for output.
+The following example shows the simplest configuration to locally
+expose, via PulseAudio, a music collection kept at @file{/srv/music},
+with @command{mpd} running as the default @samp{mpd} user.  This user
+will spawn its own PulseAudio daemon, which may compete for the sound
+card access with that of your own user.  In this configuration, you may
+have to stop the playback of your user audio applications to hear MPD's
+output and vice-versa.
 
 @lisp
 (service mpd-service-type
          (mpd-configuration
-          (user "bob")
-          (port "6666")))
+          (music-directory "/srv/music")))
 @end lisp
 
+@quotation Important
+The music directory must be readable to the MPD user, by default,
+@samp{mpd}.  Permission problems will be reported via @samp{Permission
+denied} errors in the MPD logs, which appear in @file{/var/log/messages}
+by default.
+@end quotation
+
 Most MPD clients will trigger a database update upon connecting, but you
 can also use the @code{update} action do to so:
 
-- 
2.39.2

bug#63082: [PATCH v3 16/16] services: Avoid 'delete' overrides warning in audio module.

[Maxim Cournoyer]

* gnu/services/audio.scm: Hide 'delete' on (gnu services) import.
---
 gnu/services/audio.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index e242f48ba0..146ad89b0d 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -26,7 +26,7 @@ (define-module (gnu services audio)
   #:use-module (guix diagnostics)
   #:use-module (guix i18n)
   #:use-module (guix modules)
-  #:use-module (gnu services)
+  #:use-module ((gnu services) #:hide (delete))
   #:use-module (gnu services admin)
   #:use-module (gnu services configuration)
   #:use-module (gnu services shepherd)
-- 
2.39.2