Christopher Baines writes: > On Sat, 12 Aug 2017 19:10:08 +0200 > Peter Mikkelsen wrote: > >> Hi, thanks for the quick review! >> >> Christopher Baines writes: >> >> > 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 >> > >> >> This is pretty normal for mpd, and I believe happens because it first >> binds on IPv6 and then fails for IPv4. The error about the playlist >> dir happens because it does not exist, but AFAIK it is no problem >> unless you want to save playlists. The user can just create the dir. > > Ok, good to know :) > >> > On Sat, 12 Aug 2017 03:52:11 +0200 >> > Peter Mikkelsen wrote: >> >> * doc/guix.text: Add documentation. >> > >> > Typo above, text rather than texi. >> > >> >> Ups, my mistake. >> >> >> * gnu/services/music.scm (): 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... >> > >> >> That sounds like a good idea. >> >> >> +@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. >> > >> >> Oh yes, me 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{} 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 >> >> +;;; >> >> +;;; 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 >> >> . + >> >> +(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 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. >> >> I am not sure I get how this works, and I would probs just make a big >> mess. What about we take my approach first, and then I can come back >> to it when I learn some more about guix? > > No problem. Looking at it again, I think it might be a bit trickier > than I initially assumed, as the pid-file value is used. > Oh i see. >> > Also, now that a 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). >> > >> Ok with me. >> >> > 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 >> >> +;;; >> >> +;;; 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 >> >> . + >> >> +(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. >> > >> >> Right, I have removed this test. > > Great. Just to check I wasn't wrong, I've just tested what happens if > you break the service by getting mpd to create the PID file, and > shepherd to look for it in different places, and the previous test > about starting the service does indeed fail, which is what we want :D > Great! >> >> + (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)))) >> >> I think I have fixed all of your suggestions (apart from the >> gexp-compiler one). Please see my new patch. > > Awesome, I've put a couple more suggestions below, but just on the docs > and a bit of code style. Regardless of these, I think this is good to > go. > > >> From 419a8df59bc958ee87ece5519393b32cfbef609c Mon Sep 17 00:00:00 2001 >> From: Peter Mikkelsen >> Date: Sat, 12 Aug 2017 03:40:25 +0200 >> Subject: [PATCH] gnu: Add mpd service. >> >> * doc/guix.texi: Add documentation. >> * gnu/services/audio.scm (): New record type. >> (mpd-service-type): New service type. >> * gnu/tests/audio.scm: New file. >> * gnu/local.mk: Add new files. >> --- >> doc/guix.texi | 49 ++++++++++++++++++++++++++++ >> gnu/local.mk | 2 ++ >> gnu/services/audio.scm | 86 >> ++++++++++++++++++++++++++++++++++++++++++++++++++ >> gnu/tests/audio.scm | 78 >> +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 215 >> insertions(+) create mode 100644 gnu/services/audio.scm create mode >> 100644 gnu/tests/audio.scm >> >> diff --git a/doc/guix.texi b/doc/guix.texi >> index 8f14ddd50..754408ade 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. >> +* Audio 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. >> +* Audio Services:: The MPD. >> * Miscellaneous Services:: Other services. >> @end menu >> >> @@ -15635,6 +15637,53 @@ Package object of thermald. >> @end table >> @end deftp >> >> +@node Audio Services >> +@subsubsection Audio Services >> + >> +@cindex mpd >> +@subsubheading Music Player Daemon >> + >> +The @code{(gnu services audio)} 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{"/var/run/mpd.pid"}) >> +The file mpd wil store its PID. This must be an absolute path. >> + >> +@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. >> + >> +@end table >> +@end deftp >> + >> +The following example shows how one might run @code{mpd} as user >> +@code{"bob"} on port @code{6666}. >> +@example >> +(service mpd-service-type >> + (mpd-configuration >> + (user "bob") >> + (port "6666"))) >> +@end example > > I've got a few suggestions for the docs. Nothing too important, and I'm > fine if you still prefer docs without the suggestions below, but feel > free to pick and choose any changes that you think are good, I've put > my reasoning inline in round brackets. > > > @node Audio Services > @subsubsection Audio Services > > The @code{(gnu services audio)} module provides a service to > start MPD (the Music Player Daemon). > > ( > Having the introduction to the module above the Music Player Daemon > subsubheading seems neater. Also, I think adding "module" after > @code{(gnu services audio)} helps with readability. > ) > > @cindex mpd > @subsubheading Music Player Daemon > > The Music Player Daemon (MPD) is a service that can play music while > being controlled from the local machine or over the network by a > variety of clients. > > ( > An introductory paragraph about what the service does might be > useful, so I've written one above. > ) > > The following example shows how one might run @code{mpd} as user > @code{"bob"} on port @code{6666}. It uses pulseaudio for output. > > @example > (service mpd-service-type > (mpd-configuration > (user "bob") > (port "6666"))) > @end example > > ( > Moving the example here might be more visible, rather than below the > reference documentation. > ) > > @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}. > > ... > > @end table > @end deftp > > I like all your suggestions and they are all part of the new patch, thanks! >> @node Miscellaneous Services >> @subsubsection Miscellaneous Services >> diff --git a/gnu/local.mk b/gnu/local.mk >> index cffb18d3a..c12fd8559 100644 >> --- a/gnu/local.mk >> +++ b/gnu/local.mk >> @@ -426,6 +426,7 @@ GNU_SYSTEM_MODULES >> = \ \ >> %D%/services.scm \ >> %D%/services/admin.scm \ >> + %D%/services/audio.scm \ >> %D%/services/avahi.scm \ >> %D%/services/base.scm \ >> %D%/services/configuration.scm \ >> @@ -481,6 +482,7 @@ GNU_SYSTEM_MODULES >> = \ \ >> %D%/tests.scm \ >> %D%/tests/admin.scm \ >> + %D%/tests/audio.scm \ >> %D%/tests/base.scm \ >> %D%/tests/databases.scm \ >> %D%/tests/dict.scm \ >> diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm >> new file mode 100644 >> index 000000000..f5c465341 >> --- /dev/null >> +++ b/gnu/services/audio.scm >> @@ -0,0 +1,86 @@ >> +;;; GNU Guix --- Functional package management for GNU >> +;;; Copyright © 2017 Peter Mikkelsen >> +;;; >> +;;; 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 . >> + >> +(define-module (gnu services audio) >> + #: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-type)) >> + >> +;;; Commentary: >> +;;; >> +;;; Audio related services >> +;;; >> +;;; Code: >> + >> +(define-record-type* >> + 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 "/var/run/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"))) > > One way of making this a bit more concise and remove the need for car > and cadr is to use the match module (ice-9 match). > > (map (match-lambda > ((config-name config-val) > (string-append config-name " \"" (config-val config) "\"\n"))) > ... > > I didn't know about match-lambda, but it seems pretty neat, so I used your example here, thanks. >> + `(("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))))) >> + > > ...