Hey,

This looks great Peter, awesome job :) I've made some notes about
potential improvements inline below.

I've succeeded in running the system test locally, but there was some
suspicious output in the log:

exception: bind to '0.0.0.0:6600' failed (continuing anyway, because
binding to '[::]:6600' succeeded): Failed to bind socket: Address
already in use exception: Failed to access /root/.mpd/playlists: No
such file or directory


On Sat, 12 Aug 2017 03:52:11 +0200
Peter Mikkelsen <petermikkelsen10@gmail.com> wrote:
> * doc/guix.text: Add documentation.

Typo above, text rather than texi.

> * gnu/services/music.scm (<mpd-configuration>): New record type.
>   (mpd-service): New service extension.
>   (mpd-service-type): New service type.
> * gnu/tests/music.scm: New file.
> * gnu/local.mk: Add new files.
> 
> ---
>  doc/guix.texi          | 53 +++++++++++++++++++++++++++++++
>  gnu/local.mk           |  2 ++
>  gnu/services/music.scm | 84
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> gnu/tests/music.scm    | 83
> +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed,
> 222 insertions(+) create mode 100644 gnu/services/music.scm create
> mode 100644 gnu/tests/music.scm
> 
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 8f14ddd50..e565dfdc9 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -227,6 +227,7 @@ Services
>  * Network File System::         NFS related services.
>  * Continuous Integration::      The Cuirass service.
>  * Power management Services::   The TLP tool.
> +* Music Services::              The MPD.
>  * Miscellaneous Services::      Other services.
>  
>  Defining Services
> @@ -9035,6 +9036,7 @@ declaration.
>  * Network File System::         NFS related services.
>  * Continuous Integration::      The Cuirass service.
>  * Power management Services::   The TLP tool.
> +* Music Services::              The MPD.
>  * Miscellaneous Services::      Other services.
>  @end menu
>  
> @@ -15635,6 +15637,57 @@ Package object of thermald.
>  @end table
>  @end deftp
>  
> +@node Music Services
> +@subsubsection Music Services

I'm wondering if Audio services, rather than Music services might be
better? Maybe this would fit in other services related to audio, e.g.
Jack, MIDI stuff, etc...

> +@cindex mpd
> +@subsubheading Music Player Daemon
> +
> +The @code{(gnu services music)} provides a service to start MPD (the
> Music +Player Daemon). It uses pulseaudio for output.
> +
> +@defvr {Scheme Variable} mpd-service-type
> +The service type for @command{mpd}
> +@end defvr
> +
> +@deftp {Data Type} mpd-configuration
> +Data type representing the configuration of @command{mpd}.
> +
> +@table @asis
> +@item @code{user} (default: @code{"mpd"})
> +The user to run mpd as.
> +
> +@item @code{music-dir} (default: @code{"~/Music"})
> +The directory to scan for music files.
> +
> +@item @code{playlist-dir} (default: @code{"~/.mpd/playlists"})
> +The directory to store playlists.
> +
> +@item @code{pid-file} (default: @code{"~/.mpd-pid"})
> +The file mpd wil store its PID.
> +
> +@item @code{port} (default: @code{"6600"})
> +The port to run mpd on.
> +
> +@item @code{address} (default: @code{"any"})
> +The address that mpd will bind to. To use a Unix domain socket,
> +an absolute path can be specified here.

The style for Guix is to use two spaces between sentences, I always
forget about this too.

> +
> +@end table
> +@end deftp
> +
> +@deffn {Scheme Procedure} mpd-service [#:config (mpd-configuration)]
> +Return a service that runs @code{mpd} using @var{configuration},
> +a @code{<mpd-configuration>} object.
> +
> +The following example shows how one might run @code{mpd} as user
> +@code{"bob"} on port @code{6666}.
> +@example
> +(mpd-service (mpd-configuration
> +              (user "bob")
> +              (port "6666")))
> +@end example
> +@end deffn
>  
>  @node Miscellaneous Services
>  @subsubsection Miscellaneous Services
> diff --git a/gnu/local.mk b/gnu/local.mk
> index b1ff72d6a..cad0ba38d 100644
> --- a/gnu/local.mk
> +++ b/gnu/local.mk
> @@ -441,6 +441,7 @@ GNU_SYSTEM_MODULES
> =				\
> %D%/services/mail.scm				\
> %D%/services/mcron.scm			\
> %D%/services/messaging.scm			\
> +  %D%/services/music.scm                        \
>    %D%/services/networking.scm			\
>    %D%/services/nfs.scm			\
>    %D%/services/shepherd.scm			\
> @@ -488,6 +489,7 @@ GNU_SYSTEM_MODULES
> =				\
> %D%/tests/install.scm				\
> %D%/tests/mail.scm				\
> %D%/tests/messaging.scm			\
> +  %D%/tests/music.scm				\
>    %D%/tests/networking.scm			\
>    %D%/tests/ssh.scm				\
>    %D%/tests/web.scm
> diff --git a/gnu/services/music.scm b/gnu/services/music.scm
> new file mode 100644
> index 000000000..77912d5c6
> --- /dev/null
> +++ b/gnu/services/music.scm
> @@ -0,0 +1,84 @@
> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2017 Peter Mikkelsen <petermikkelsen10@gmail.com>
> +;;;
> +;;; This file is part of GNU Guix.
> +;;;
> +;;; GNU Guix is free software; you can redistribute it and/or modify
> it +;;; under the terms of the GNU General Public License as
> published by +;;; the Free Software Foundation; either version 3 of
> the License, or (at +;;; your option) any later version.
> +;;;
> +;;; GNU Guix is distributed in the hope that it will be useful, but
> +;;; WITHOUT ANY WARRANTY; without even the implied warranty of
> +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;;; GNU General Public License for more details.
> +;;;
> +;;; You should have received a copy of the GNU General Public License
> +;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
> +
> +(define-module (gnu services music)
> +  #:use-module (guix gexp)
> +  #:use-module (gnu services)
> +  #:use-module (gnu services shepherd)
> +  #:use-module (gnu packages mpd)
> +  #:use-module (guix records)
> +  #:export (mpd-configuration
> +            mpd-configuration?
> +            mpd-service
> +            mpd-service-type))
> +
> +;;; Commentary:
> +;;;
> +;;; Music related services
> +;;;
> +;;; Code:
> +
> +(define-record-type* <mpd-configuration>
> +  mpd-configuration make-mpd-configuration
> +  mpd-configuration?
> +  (user         mpd-configuration-user
> +                (default "mpd"))
> +  (music-dir    mpd-configuration-music-dir
> +                (default "~/Music"))
> +  (playlist-dir mpd-configuration-playlist-dir
> +                (default "~/.mpd/playlists"))
> +  (port         mpd-configuration-port
> +                (default "6600"))
> +  (address      mpd-configuration-address
> +                (default "any"))
> +  (pid-file     mpd-configuration-pid-file
> +                (default "~/.mpd-pid")))
> +
> +(define (mpd-config->file config)
> +  (apply
> +   mixed-text-file "mpd.conf"
> +   "audio_output {\n"
> +   "  type \"pulse\"\n"
> +   "  name \"MPD\"\n"
> +   "}\n"
> +   (map (lambda (config-line)
> +          (let ((config-name (car config-line))
> +                (config-val (cadr config-line)))
> +            (string-append config-name " \"" (config-val config)
> "\"\n")))
> +        `(("user" ,mpd-configuration-user)
> +          ("music_directory" ,mpd-configuration-music-dir)
> +          ("playlist_directory" ,mpd-configuration-playlist-dir)
> +          ("port" ,mpd-configuration-port)
> +          ("bind_to_address" ,mpd-configuration-address)
> +          ("pid_file" ,mpd-configuration-pid-file)))))
> +
> +(define mpd-service-type
> +  (shepherd-service-type
> +   'mpd
> +   (lambda (config)
> +     (shepherd-service
> +      (documentation "Run the MPD (Music Player Daemon)")
> +      (provision '(mpd))
> +      (start #~(make-forkexec-constructor
> +                (list #$(file-append mpd "/bin/mpd")
> +                      "--no-daemon"
> +                      #$(mpd-config->file config))))
> +      (stop  #~(make-kill-destructor))))))
> +
> +(define* (mpd-service #:optional (config (mpd-configuration)))
> +  (service mpd-service-type config))

I've been trying a slightly different style for this recently. At the
moment, if you had a configuration file for MPD, you couldn't use this
with the service here. One way of addressing this is to do something
like the Tailon service, and define a gexp compiler for the
configuration file (e.g. [1]). For the Tailon service, this means that
you should be able to pass your own configuration file to the service.

Also, now that a <service-type> can have a default-value, I think its
easier to just have the mpd-service-type, without the mpd-service
procedure. If you add a default value, you should be able to do
(service mpd-service-type).

1:
https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/services/admin.scm#n255

> diff --git a/gnu/tests/music.scm b/gnu/tests/music.scm
> new file mode 100644
> index 000000000..158513098
> --- /dev/null
> +++ b/gnu/tests/music.scm
> @@ -0,0 +1,83 @@
> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2017 Peter Mikkelsen <petermikkelsen10@gmail.com>
> +;;;
> +;;; This file is part of GNU Guix.
> +;;;
> +;;; GNU Guix is free software; you can redistribute it and/or modify
> it +;;; under the terms of the GNU General Public License as
> published by +;;; the Free Software Foundation; either version 3 of
> the License, or (at +;;; your option) any later version.
> +;;;
> +;;; GNU Guix is distributed in the hope that it will be useful, but
> +;;; WITHOUT ANY WARRANTY; without even the implied warranty of
> +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;;; GNU General Public License for more details.
> +;;;
> +;;; You should have received a copy of the GNU General Public License
> +;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
> +
> +(define-module (gnu tests music)
> +  #:use-module (gnu tests)
> +  #:use-module (gnu system)
> +  #:use-module (gnu system vm)
> +  #:use-module (gnu services)
> +  #:use-module (gnu services music)
> +  #:use-module (gnu packages mpd)
> +  #:use-module (guix gexp)
> +  #:export (%test-mpd))
> +
> +(define %mpd-os
> +  (simple-operating-system
> +   (mpd-service (mpd-configuration
> +                 (user "root")))))
> +
> +(define (run-mpd-test)
> +  "Run tests in %mpd-os, which has mpd running."
> +  (define os
> +    (marionette-operating-system
> +     %mpd-os
> +     #:imported-modules '((gnu services herd))))
> +
> +  (define vm
> +    (virtual-machine os))
> +
> +  (define test
> +    (with-imported-modules '((gnu build marionette))
> +      #~(begin
> +          (use-modules (srfi srfi-64)
> +                       (gnu build marionette))
> +          (define marionette
> +            (make-marionette (list #$vm)))
> +
> +          (mkdir #$output)
> +          (chdir #$output)
> +
> +          (test-begin "mpd")
> +
> +          (test-eq "service is running"
> +            'running!
> +            (marionette-eval
> +             '(begin
> +                (use-modules (gnu services herd))
> +                (start-service 'mpd)
> +                'running!)
> +             marionette))

Recently, the start-service procedure was changed to return the
information from the shepherd, and this can be used to make this check
a bit more rigorous.

I've got an patch for the Memcached service which demonstrates this
here [2], as with the test above, it will not always fail, even if the
service fails to create the PID file.

2: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28021

> +          (test-assert "pid file"
> +            (wait-for-file "/root/.mpd-pid"
> +             marionette))

If this is useful when using MPD, then I think it would be valuable to
get the shepherd to wait for the PID file. I think you can do this by
adding #:pid-file to the make-forkexec-constructor call.

If you do this, them I'm not sure this test adds anything, as I think
the start-service call would only return successfully when the service
has started, and created the PID file.

> +          (test-assert "mpc connect"
> +            (marionette-eval
> +             '(zero? (system #$(file-append mpd-mpc "/bin/mpc")))
> +             marionette))
> +
> +          (test-end)
> +          (exit (= (test-runner-fail-count (test-runner-current))
> 0)))))
> +  (gexp->derivation "mpd-test" test))
> +
> +(define %test-mpd
> +  (system-test
> +   (name "mpd")
> +   (description "Test that the mpd can run and be connected to.")
> +   (value (run-mpd-test))))