[bug#28059] [PATCH] gnu: Add mpd service.

[ng0 <ng0@infotropique.org>]

[-- Attachment #1: Type: text/plain, Size: 13880 bytes --]

Christopher Baines transcribed 13K bytes:
> 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

Isn't this normal for MPD? I haven't used any system service for it
in a very long time and starting mpd as a user process always gives
me a message like this.

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



-- 
ng0
GnuPG: A88C8ADD129828D7EAC02E52E22F9BBFEE348588
GnuPG: https://n0is.noblogs.org/my-keys
https://www.infotropique.org https://krosos.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]