[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#28059] [PATCH] gnu: Add mpd service.
|
From: |
Christopher Baines |
|
Subject: |
[bug#28059] [PATCH] gnu: Add mpd service. |
|
Date: |
Sat, 12 Aug 2017 11:50:11 +0100 |
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 <address@hidden> 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
>
> address@hidden Music Services
> address@hidden 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...
> address@hidden mpd
> address@hidden Music Player Daemon
> +
> +The @code{(gnu services music)} provides a service to start MPD (the
> Music +Player Daemon). It uses pulseaudio for output.
> +
> address@hidden {Scheme Variable} mpd-service-type
> +The service type for @command{mpd}
> address@hidden defvr
> +
> address@hidden {Data Type} mpd-configuration
> +Data type representing the configuration of @command{mpd}.
> +
> address@hidden @asis
> address@hidden @code{user} (default: @code{"mpd"})
> +The user to run mpd as.
> +
> address@hidden @code{music-dir} (default: @code{"~/Music"})
> +The directory to scan for music files.
> +
> address@hidden @code{playlist-dir} (default: @code{"~/.mpd/playlists"})
> +The directory to store playlists.
> +
> address@hidden @code{pid-file} (default: @code{"~/.mpd-pid"})
> +The file mpd wil store its PID.
> +
> address@hidden @code{port} (default: @code{"6600"})
> +The port to run mpd on.
> +
> address@hidden @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.
> +
> address@hidden table
> address@hidden deftp
> +
> address@hidden {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
> address@hidden"bob"} on port @code{6666}.
> address@hidden
> +(mpd-service (mpd-configuration
> + (user "bob")
> + (port "6666")))
> address@hidden example
> address@hidden 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 <address@hidden>
> +;;;
> +;;; 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 <address@hidden>
> +;;;
> +;;; 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))))
pgp3WkDs88U7t.pgp
Description: OpenPGP digital signature