[PATCH] gnu: mpd: Add support for socket activation.

OpenSubmitted by Liliana Marie Prikler.
Details
3 participants
  • Liliana Marie Prikler
  • Ludovic Courtès
  • Maxime Devos
Owner
unassigned
Severity
normal
L
L
Liliana Marie Prikler wrote on 17 Apr 12:01 +0200
(address . guix-patches@gnu.org)
9d4cc9d3ebb05d2aabf8f06e1890efe9b0b9a849.camel@gmail.com
* gnu/packages/mpd.scm (mpd)[#:configure-flags]: Convert to G-Expression.
Add “-Dsystemd=enabled”.
[#:phases]: New argument.
[inputs]: Add elogind.
---
gnu/packages/mpd.scm | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)

Toggle diff (44 lines)
diff --git a/gnu/packages/mpd.scm b/gnu/packages/mpd.scm
index 1ee6806735..40e6a99ad7 100644
--- a/gnu/packages/mpd.scm
+++ b/gnu/packages/mpd.scm
@@ -47,6 +47,7 @@ (define-module (gnu packages mpd)
   #:use-module (gnu packages boost)
   #:use-module (gnu packages cdrom)
   #:use-module (gnu packages cmake) ;for MPD
+  #:use-module (gnu packages freedesktop) ;elogind
   #:use-module (gnu packages gettext)
   #:use-module (gnu packages gnome)
   #:use-module (gnu packages gnupg)
@@ -119,12 +120,28 @@ (define-public mpd
                 "1v969w7h3660ph3h2bdlkrzc05pfz95bmxjqdbzzf7pfwf795ifb"))))
     (build-system meson-build-system)
     (arguments
-     `(#:configure-flags '("-Ddocumentation=enabled")))
+     (list
+      #:configure-flags #~(list "-Ddocumentation=enabled"
+                                "-Dsystemd=enabled")
+      #:phases
+      #~(modify-phases %standard-phases
+          (add-after 'unpack 'enable-elogind
+            (lambda _
+              (substitute* "src/lib/systemd/meson.build"
+                (("libsystemd") "libelogind"))
+              ;; XXX: systemd dependency overwritten internally, leads to bad
+              ;;      errors
+              (substitute* "src/lib/systemd/meson.build"
+                (("systemd_dep = declare_dependency" all)
+                 (string-append "_" all)))
+              (substitute* "meson.build"
+                (("systemd_dep,") "systemd_dep, _systemd_dep,")))))))
     (inputs (list ao
                   alsa-lib
                   avahi
                   boost
                   curl
+                  elogind
                   ffmpeg
                   flac
                   fmt
-- 
2.35.1
L
L
Ludovic Courtès wrote on 17 Apr 23:06 +0200
(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)(address . 54986@debbugs.gnu.org)
87o80zzagn.fsf@gnu.org
Hi,

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

Toggle quote (5 lines)
> * gnu/packages/mpd.scm (mpd)[#:configure-flags]: Convert to G-Expression.
> Add “-Dsystemd=enabled”.
> [#:phases]: New argument.
> [inputs]: Add elogind.

LGTM!

Are you planning to update the service as well?

Thanks,
Ludo’.
L
L
Liliana Marie Prikler wrote on 17 Apr 23:57 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 54986@debbugs.gnu.org)
a710e56204436941d99c663d76cf7e51c6a2ed60.camel@gmail.com
Am Sonntag, dem 17.04.2022 um 23:06 +0200 schrieb Ludovic Courtès:
Toggle quote (13 lines)
> Hi,
>
> Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:
>
> > * gnu/packages/mpd.scm (mpd)[#:configure-flags]: Convert to G-
> > Expression.
> > Add “-Dsystemd=enabled”.
> > [#:phases]: New argument.
> > [inputs]: Add elogind.
>
> LGTM!
>
> Are you planning to update the service as well?
I'm not sure if updating the service is a good idea. On the upside,
people who report mpd starting too early will be alleviated of one
issue. On the downside, it appears as though mpd too easily escapes
shepherd's management. My current observations are more or less
consistent with what I saw with Emacs: killing the mpd service won't
stop playing music, and shepherd won't restart a killed MPD without
asked to. IMHO shepherd's socket activation still needs some work
before we can make it the default for every service.

Cheers
L
L
Ludovic Courtès wrote on 18 Apr 23:05 +0200
(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)(address . 54986@debbugs.gnu.org)
87fsmaumok.fsf@gnu.org
Hi,

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

Toggle quote (7 lines)
> I'm not sure if updating the service is a good idea. On the upside,
> people who report mpd starting too early will be alleviated of one
> issue. On the downside, it appears as though mpd too easily escapes
> shepherd's management. My current observations are more or less
> consistent with what I saw with Emacs: killing the mpd service won't
> stop playing music,

Do you mean that ‘herd stop mpd’ doesn’t stop the mpd process?

What does /var/log/messages say?

Toggle quote (2 lines)
> and shepherd won't restart a killed MPD without asked to.

Weird. What you describe sounds as if shepherd is not looking at the
right process or something.

If you have a service definition to reproduce this, I’d be happy to take
a look!

Thanks,
Ludo’.
L
L
Liliana Marie Prikler wrote on 18 Apr 23:19 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 54986@debbugs.gnu.org)
413952d60ee9444bff89412751b8c7c97a0102a1.camel@gmail.com
Am Montag, dem 18.04.2022 um 23:05 +0200 schrieb Ludovic Courtès:
Toggle quote (12 lines)
> Hi,
>
> Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:
>
> > I'm not sure if updating the service is a good idea.  On the
> > upside, people who report mpd starting too early will be alleviated
> > of one issue.  On the downside, it appears as though mpd too easily
> > escapes shepherd's management.  My current observations are more or
> > less consistent with what I saw with Emacs: killing the mpd service
> > won't stop playing music,
>
> Do you mean that ‘herd stop mpd’ doesn’t stop the mpd process?
Yep.
Toggle quote (1 lines)
> What does /var/log/messages say?
I don't think there's anything meaningful there to inspect, I'm running
this as a user service. Shepherd's own logs are rather empty.

Interestingly, the running value for the mpd service remains
(("unknown" . "#<input-output: socket 17>")) even after MPD started.
Should that be the case?

Toggle quote (7 lines)
> > and shepherd won't restart a killed MPD without asked to.
>
> Weird.  What you describe sounds as if shepherd is not looking at the
> right process or something.
>
> If you have a service definition to reproduce this, I’d be happy to
> take a look!
The second is probably just me forgetting to set #:respawn? #t. One
"bug" down, one more to go.
L
L
Liliana Marie Prikler wrote on 17 Apr 12:01 +0200
[PATCH v2 1/3] gnu: mpd: Add support for socket activation.
(address . 54986@debbugs.gnu.org)(address . ludo@gnu.org)
8c12d29e4f7ec86ca04136ba0c62bb2947f4f190.camel@gmail.com
* gnu/packages/mpd.scm (mpd)[#:configure-flags]: Convert to G-Expression.
Add “-Dsystemd=enabled”.
[#:phases]: New argument.
[inputs]: Add elogind.
---
gnu/packages/mpd.scm | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)

Toggle diff (44 lines)
diff --git a/gnu/packages/mpd.scm b/gnu/packages/mpd.scm
index 1ee6806735..40e6a99ad7 100644
--- a/gnu/packages/mpd.scm
+++ b/gnu/packages/mpd.scm
@@ -47,6 +47,7 @@ (define-module (gnu packages mpd)
   #:use-module (gnu packages boost)
   #:use-module (gnu packages cdrom)
   #:use-module (gnu packages cmake) ;for MPD
+  #:use-module (gnu packages freedesktop) ;elogind
   #:use-module (gnu packages gettext)
   #:use-module (gnu packages gnome)
   #:use-module (gnu packages gnupg)
@@ -119,12 +120,28 @@ (define-public mpd
                 "1v969w7h3660ph3h2bdlkrzc05pfz95bmxjqdbzzf7pfwf795ifb"))))
     (build-system meson-build-system)
     (arguments
-     `(#:configure-flags '("-Ddocumentation=enabled")))
+     (list
+      #:configure-flags #~(list "-Ddocumentation=enabled"
+                                "-Dsystemd=enabled")
+      #:phases
+      #~(modify-phases %standard-phases
+          (add-after 'unpack 'enable-elogind
+            (lambda _
+              (substitute* "src/lib/systemd/meson.build"
+                (("libsystemd") "libelogind"))
+              ;; XXX: systemd dependency overwritten internally, leads to bad
+              ;;      errors
+              (substitute* "src/lib/systemd/meson.build"
+                (("systemd_dep = declare_dependency" all)
+                 (string-append "_" all)))
+              (substitute* "meson.build"
+                (("systemd_dep,") "systemd_dep, _systemd_dep,")))))))
     (inputs (list ao
                   alsa-lib
                   avahi
                   boost
                   curl
+                  elogind
                   ffmpeg
                   flac
                   fmt
-- 
2.35.1
L
L
Liliana Marie Prikler wrote on 23 Apr 16:25 +0200
[PATCH v2 2/3 WIP] services: shepherd: Add support for socket activation endpoints.
(address . 54986@debbugs.gnu.org)(address . ludo@gnu.org)
454555484d434cefead47968e938ea071fe6121a.camel@gmail.com
* gnu/services/shepherd.scm (<shepherd-endpoint>): New record type.
(shepherd-endpoint->sexp): New variable.
* doc/guix.texi (Shepherd Services): Document it.
---
doc/guix.texi | 29 +++++++++++++++++++
gnu/services/shepherd.scm | 60 +++++++++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+)

Toggle diff (120 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 5399584cb0..38aeda1d2d 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -37675,6 +37675,35 @@ This, as you can see, is a fairly sophisticated way to say hello.
 info on actions.
 @end deftp
 
+@deftp {Data Type} shepherd-endpoint
+This is an endpoint that a systemd-style shepherd service listens on.
+
+@table @code
+@item address
+This is the socket address that shepherd binds and forwards to the service.
+
+@item name
+‾\_(ツ)_/‾
+
+@item style
+‾\_(ツ)_/‾
+
+@item backlog
+‾\_(ツ)_/‾
+
+@item socket-owner
+This is the user ID owning the socket.
+
+@item socket-group
+This is the group ID owning the socket.
+
+@item socket-directory-permissions
+These are the UNIX permissions to set for the directory the socket
+resides in.
+
+@end table
+@end deftp
+
 @defvr {Scheme Variable} shepherd-root-service-type
 The service type for the Shepherd ``root service''---i.e., PID@tie{}1.
 
diff --git a/gnu/services/shepherd.scm b/gnu/services/shepherd.scm
index 4fd4b2a497..3ef0023d7f 100644
--- a/gnu/services/shepherd.scm
+++ b/gnu/services/shepherd.scm
@@ -66,6 +66,16 @@ (define-module (gnu services shepherd)
             shepherd-action-documentation
             shepherd-action-procedure
 
+            shepherd-endpoint
+            shepherd-endpoint-address
+            shepherd-endpoint-name
+            shepherd-endpoint-style
+            shepherd-endpoint-backlog
+            shepherd-endpoint-socket-owner
+            shepherd-endpoint-socket-group
+            shepherd-endpoint-socket-directory-permissions
+            shepherd-endpoint->sexp
+
             %default-modules
 
             shepherd-service-file
@@ -183,6 +193,56 @@ (define %default-modules
     ((guix build utils) #:hide (delete))
     (guix build syscalls)))
 
+(define-record-type* <shepherd-endpoint>
+  shepherd-endpoint make-shepherd-endpoint
+  shepherd-endpoint?
+  (address                      shepherd-endpoint-address)      ; sockaddr
+  (name                         shepherd-endpoint-name          ; string | #f
+                                (default #f))
+  (style                        shepherd-endpoint-style         ; int | #f
+                                (default #f))
+  (backlog                      shepherd-endpoint-backlog       ; int | #f
+                                (default #f))
+  (socket-owner                 shepherd-endpoint-socket-owner  ; uid | #f
+                                (default #f))
+  (socket-group                 shepherd-endpoint-socket-group  ; gid | #f
+                                (default #f))
+  (socket-directory-permissions
+   shepherd-endpoint-socket-directory-permissions ; chmod pattern (int) | #f
+   (default #f)))
+
+(define (shepherd-endpoint->sexp endpoint)
+  (match endpoint
+    (($ <shepherd-endpoint> address
+                            name style backlog socket-owner socket-group
+                            socket-directory-permissions)
+     `(endpoint
+       ,(match (sockaddr:fam address)
+          ((? (cute = <> AF_INET) _)
+           `(make-socket-addr AF_INET
+                              ,(sockaddr:addr address)
+                              ,(sockaddr:port address)))
+          ((? (cute = <> AF_INET6) _)
+           `(make-socket-addr AF_INET6
+                              ,(sockaddr:addr address)
+                              ,(sockaddr:port address)
+                              ,(sockaddr:flowinfo address)
+                              ,(sockaddr:scopeid address)))
+          ((? (cute = <> AF_UNIX) _)
+           `(make-socket-addr AF_UNIX
+                              ,(sockaddr:path address))))
+       ,@(fold
+          (match-lambda*
+            (((key value) seed)
+             (if value (cons* key value seed) seed)))
+          '()
+          `((#:name ,name)
+            (#:style ,style)
+            (#:backlog ,backlog)
+            (#:socket-owner ,socket-owner)
+            (#:socket-group ,socket-group)
+            (#:socket-directory-permissions ,socket-directory-permissions)))))))
+
 (define-record-type* <shepherd-service>
   shepherd-service make-shepherd-service
   shepherd-service?
-- 
2.35.1
L
L
Liliana Marie Prikler wrote on 23 Apr 16:39 +0200
[PATCH v2 3/3 WIP] services: mpd: Support socket activation.
(address . 54986@debbugs.gnu.org)(address . ludo@gnu.org)
155723efbcf08c6e0bb6552b8f6341d4a1f20ecb.camel@gmail.com
* gnu/services/audio.scm (<mpd-configuration>)[shepherd-endpoints]: New field.
(mpd-shepherd-service): Use it.
* doc/guix.texi (Music Player Daemon): Document it.
---
doc/guix.texi | 7 +++++++
gnu/services/audio.scm | 45 ++++++++++++++++++++++++++++++------------
2 files changed, 39 insertions(+), 13 deletions(-)

Toggle diff (83 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 38aeda1d2d..6bfa854b1d 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -30883,6 +30883,13 @@ The port to run mpd on.
 The address that mpd will bind to.  To use a Unix domain socket,
 an absolute path can be specified here.
 
+@item @code{shepherd-endpoints} (default: @code{'()})
+The endpoints shepherd shall bind and spawn MPD from.  If this field is
+not empty (checked via @code{null?}), a systemd-style service is used
+rather than a forkexec-service.  This delays the start of MPD until the
+first client connects.  As a side effect @code{port} and @code{address}
+may be ignored.
+
 @item @code{outputs} (default: @code{"(list (mpd-output))"})
 The audio outputs that MPD can use.  By default this is a single output using pulseaudio.
 
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index c60053f33c..b184ac596a 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -78,6 +78,8 @@ (define-record-type* <mpd-configuration>
                 (default "6600"))
   (address      mpd-configuration-address
                 (default "any"))
+  (shepherd-endpoints mpd-configuration-shepherd-endpoints
+                      (default '())) ; list of <shepherd-endpoint>
   (outputs      mpd-configuration-outputs
                 (default (list (mpd-output)))))
 
@@ -140,19 +142,36 @@ (define (mpd-shepherd-service config)
    (documentation "Run the MPD (Music Player Daemon)")
    (requirement '(user-processes))
    (provision '(mpd))
-   (start #~(make-forkexec-constructor
-             (list #$(file-append mpd "/bin/mpd")
-                   "--no-daemon"
-                   #$(mpd-config->file config))
-             #:environment-variables
-             ;; Required to detect PulseAudio when run under a user account.
-             (list (string-append
-                    "XDG_RUNTIME_DIR=/run/user/"
-                    (number->string
-                     (passwd:uid
-                      (getpwnam #$(mpd-configuration-user config))))))
-             #:log-file #$(mpd-file-name config "log")))
-   (stop  #~(make-kill-destructor))))
+   (start (if (null? (mpd-configuration-shepherd-endpoints config))
+              #~(make-forkexec-constructor
+                 (list #$(file-append mpd "/bin/mpd")
+                       "--no-daemon"
+                       #$(mpd-config->file config))
+                 #:environment-variables
+                 ;; Required to detect PulseAudio when run under a user account.
+                 (list (string-append
+                        "XDG_RUNTIME_DIR=/run/user/"
+                        (number->string
+                         (passwd:uid
+                          (getpwnam #$(mpd-configuration-user config))))))
+                 #:log-file #$(mpd-file-name config "log"))
+              #~(make-systemd-constructor
+                 (list #$(file-append mpd "/bin/mpd")
+                       "--systemd"
+                       #$(mpd-config->file config))
+                 (list #$@(map shepherd-endpoint->sexp
+                               (mpd-configuration-shepherd-endpoints config)))
+                 #:environment-variables
+                 ;; Required to detect PulseAudio when run under a user account.
+                 (list (string-append
+                        "XDG_RUNTIME_DIR=/run/user/"
+                        (number->string
+                         (passwd:uid
+                          (getpwnam #$(mpd-configuration-user config))))))
+                 #:log-file #$(mpd-file-name config "log"))))
+   (stop  (if (null? (mpd-configuration-shepherd-endpoints config))
+              #~(make-kill-destructor)
+              #~(make-systemd-destructor)))))
 
 (define (mpd-service-activation config)
   (with-imported-modules '((guix build utils))
-- 
2.35.1
M
M
Maxime Devos wrote on 23 Apr 19:31 +0200
(address . ludo@gnu.org)
ec82acd564e34a73dd5d359788f292672079505a.camel@telenet.be
Toggle quote (4 lines)
> +(define (shepherd-endpoint->sexp endpoint)
> + (match endpoint
> + (($ <shepherd-endpoint> address
> + name style backlog socket-owner socket-
group
Toggle quote (8 lines)
> + socket-directory-permissions)
> + `(endpoint
> + ,(match (sockaddr:fam address)
> + ((? (cute = <> AF_INET) _)
> + `(make-socket-addr AF_INET
> + ,(sockaddr:addr address)
> + ,(sockaddr:port address)))

Liliana Marie Prikler schreef op za 23-04-2022 om 16:39 [+0200]:
Toggle quote (3 lines)
> + (list #$@(map shepherd-endpoint->sexp
> + (mpd-configuration-shepherd-endpoints config))) 

For hygiene reasons, should 'shepherd-endpoint->sexp' use @?

`((@ (the module) endpoint)
,(match [...]
((@ [...] make-socket-addr (@ [...] AF_UNIX) ...)
...))

That way, no assumptions are made on what modules are imported and it
avoids hygiene problems like in

;; There are two ‘endpoints’ here: the ‘API endpoint’,
;; and Shepherd endpoints.
#~(let ((endpoint "http://localhost:1234/api"))
(make-systemd-constructor
(list #$(file-append soft "/bin/ware")
"--endpoint" #$endpoint)
(list (shepherd-endpoint->sexp ...))))

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYmQ39hccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7sQ1AQDwdd0W78+WuBLbBsCj42QqTl67
0wKGnWz8fetQ4c3KtQD/TeTUBQBNWZ9lDKgKs2tFiDV45g8d1/OFPn4LLaDxGAo=
=E7SQ
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 23 Apr 19:35 +0200
(address . ludo@gnu.org)
118400e2373bae7c4147e51ed2eb26eec27e3a05.camel@telenet.be
Liliana Marie Prikler schreef op za 23-04-2022 om 16:39 [+0200]:
Toggle quote (5 lines)
> +@item @code{shepherd-endpoints} (default: @code{'()})
> +The endpoints shepherd shall bind and spawn MPD from.  If this field is
> +not empty (checked via @code{null?}), a systemd-style service is used
> +rather than a forkexec-service.

systemd-style / forkexec-service looks like an implementation detail to
me, not something useful to the user.

Toggle quote (4 lines)
>  This delays the start of MPD until the
> +first client connects.
>

Likewise.

Toggle quote (3 lines)
>   As a side effect @code{port} and @code{address}
> +may be ignored.

Could these three fields be unified, keeping 'port' and 'address' only
for some backwards compatibility? E.g.:

(mpd-configuration
(endpoints (list (shepherd-endpoint [a AF_INET6 address])
(shepherd-endpoint [a AF_UNIX address]))))

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYmQ48hccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7lyVAQC1CAeKjbGP1evIu5nuk+hU4Cj4
dofW6SrBqZ+othRhmAD/W/AzXs6WzBFnMb7nilaQPTC6cW1lnH2Jvvr+ILQgTAA=
=RhFJ
-----END PGP SIGNATURE-----


L
L
Liliana Marie Prikler wrote on 23 Apr 20:28 +0200
(address . ludo@gnu.org)
ba08b708ecb96e4e52d698d82616cee13e900e35.camel@gmail.com
Am Samstag, dem 23.04.2022 um 19:35 +0200 schrieb Maxime Devos:
Toggle quote (15 lines)
> Liliana Marie Prikler schreef op za 23-04-2022 om 16:39 [+0200]:
> > +@item @code{shepherd-endpoints} (default: @code{'()})
> > +The endpoints shepherd shall bind and spawn MPD from.  If this
> > field is
> > +not empty (checked via @code{null?}), a systemd-style service is
> > used
> > +rather than a forkexec-service.
>
> systemd-style / forkexec-service looks like an implementation detail
> to me, not something useful to the user.
>
> >  This delays the start of MPD until the
> > +first client connects.
>
> Likewise.
Even if this was just an implementation detail, I shall point to
Hyrum's law.

Toggle quote (9 lines)
> >   As a side effect @code{port} and @code{address}
> > +may be ignored.
>
> Could these three fields be unified, keeping 'port' and 'address'
> only for some backwards compatibility?  E.g.:
>
>   (mpd-configuration
>     (endpoints (list (shepherd-endpoint [a AF_INET6 address])
>                      (shepherd-endpoint [a AF_UNIX address]))))
Not AFAIK. MPD is less strict than Guile when it comes to addresses,
so converting from address+port to socket addresses is nontrivial.
That being said, I'm not sure if they are even ignored. MPD only warns
about pid_file, so they might also be used.

Cheers
L
L
Liliana Marie Prikler wrote on 23 Apr 20:36 +0200
(address . ludo@gnu.org)
9d93799fedd4f40748669af04473198092c77173.camel@gmail.com
Am Samstag, dem 23.04.2022 um 19:31 +0200 schrieb Maxime Devos:
Toggle quote (36 lines)
> > +(define (shepherd-endpoint->sexp endpoint)
> > +  (match endpoint
> > +    (($ <shepherd-endpoint> address
> > +                            name style backlog socket-owner
> > socket-
> group
> > +                            socket-directory-permissions)
> > +     `(endpoint
> > +       ,(match (sockaddr:fam address)
> > +          ((? (cute = <> AF_INET) _)
> > +           `(make-socket-addr AF_INET
> > +                              ,(sockaddr:addr address)
> > +                              ,(sockaddr:port address)))
>
> Liliana Marie Prikler schreef op za 23-04-2022 om 16:39 [+0200]:
> > +                 (list #$@(map shepherd-endpoint->sexp
> > +                               (mpd-configuration-shepherd-
> > endpoints config))) 
>
> For hygiene reasons, should 'shepherd-endpoint->sexp' use @?
>
>   `((@ (the module) endpoint)
>     ,(match [...]
>         ((@ [...] make-socket-addr (@ [...] AF_UNIX) ...)
>         ...))
>
> That way, no assumptions are made on what modules are imported and it
> avoids hygiene problems like in
>
>   ;; There are two ‘endpoints’ here: the ‘API endpoint’,
>   ;; and Shepherd endpoints.
>   #~(let ((endpoint "http://localhost:1234/api"))
>       (make-systemd-constructor
>          (list #$(file-append soft "/bin/ware")
>                "--endpoint" #$endpoint)
>          (list (shepherd-endpoint->sexp ...))))
Good point, that's should probably done for defensive programming.
Note also that I'm missing half of the documentation still, so it won't
be pushed too soon, though.
L
L
Ludovic Courtès wrote on 27 Apr 22:56 +0200
Re: bug#54986: [PATCH] gnu: mpd: Add support for socket activation.
(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)(address . 54986@debbugs.gnu.org)
87v8uui6st.fsf@gnu.org
Hi,

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

[...]

Toggle quote (6 lines)
>> Do you mean that ‘herd stop mpd’ doesn’t stop the mpd process?
> Yep.
>> What does /var/log/messages say?
> I don't think there's anything meaningful there to inspect, I'm running
> this as a user service. Shepherd's own logs are rather empty.

Then it’s in ~/.local/state (by default) or
~/.local/var/log/shepherd.log if you’re using Guix Home.

Toggle quote (4 lines)
> Interestingly, the running value for the mpd service remains
> (("unknown" . "#<input-output: socket 17>")) even after MPD started.
> Should that be the case?

No.

Toggle quote (10 lines)
>> > and shepherd won't restart a killed MPD without asked to.
>>
>> Weird.  What you describe sounds as if shepherd is not looking at the
>> right process or something.
>>
>> If you have a service definition to reproduce this, I’d be happy to
>> take a look!
> The second is probably just me forgetting to set #:respawn? #t. One
> "bug" down, one more to go.

:-)

Ludo’.
L
L
Ludovic Courtès wrote on 28 Apr 00:05 +0200
Re: [PATCH v2 2/3 WIP] services: shepherd: Add support for socket activation endpoints.
(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)(address . 54986@debbugs.gnu.org)
874k2egp28.fsf@gnu.org
Hi,

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

Toggle quote (4 lines)
> * gnu/services/shepherd.scm (<shepherd-endpoint>): New record type.
> (shepherd-endpoint->sexp): New variable.
> * doc/guix.texi (Shepherd Services): Document it.

[...]

Toggle quote (24 lines)
> +++ b/gnu/services/shepherd.scm
> @@ -66,6 +66,16 @@ (define-module (gnu services shepherd)
> shepherd-action-documentation
> shepherd-action-procedure
>
> + shepherd-endpoint
> + shepherd-endpoint-address
> + shepherd-endpoint-name
> + shepherd-endpoint-style
> + shepherd-endpoint-backlog
> + shepherd-endpoint-socket-owner
> + shepherd-endpoint-socket-group
> + shepherd-endpoint-socket-directory-permissions
> + shepherd-endpoint->sexp
> +
> %default-modules
>
> shepherd-service-file
> @@ -183,6 +193,56 @@ (define %default-modules
> ((guix build utils) #:hide (delete))
> (guix build syscalls)))
>
> +(define-record-type* <shepherd-endpoint>

I don’t think this is necessary: Shepherd endpoints are created in a
#~(make-systemd-constructor …) gexp, it’s OK to use the Shepherd
endpoint API there.

Ludo’.
L
L
Ludovic Courtès wrote on 28 Apr 00:08 +0200
Re: [PATCH v2 3/3 WIP] services: mpd: Support socket activation.
(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)
87y1zqfabs.fsf@gnu.org
Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:

Toggle quote (4 lines)
> * gnu/services/audio.scm (<mpd-configuration>)[shepherd-endpoints]: New field.
> (mpd-shepherd-service): Use it.
> * doc/guix.texi (Music Player Daemon): Document it.

[...]

Toggle quote (8 lines)
> +++ b/gnu/services/audio.scm
> @@ -78,6 +78,8 @@ (define-record-type* <mpd-configuration>
> (default "6600"))
> (address mpd-configuration-address
> (default "any"))
> + (shepherd-endpoints mpd-configuration-shepherd-endpoints
> + (default '())) ; list of <shepherd-endpoint>

The way I see it, service configuration should be oblivious to whether
it’s started as “forkexec”, systemd, or inetd.

There’s already an ‘address’ field above, so my suggestion would be to
reuse it. This is what I did for example for the openssh service, and
also for bitlbee and dicod here:


WDYT?

Thanks,
Ludo’.
L
L
Liliana Marie Prikler wrote on 28 Apr 18:45 +0200
Re: bug#54986: [PATCH] gnu: mpd: Add support for socket activation.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 54986@debbugs.gnu.org)
aa80e431411c64da21253501eeaf3e8e722a4d49.camel@gmail.com
Am Mittwoch, dem 27.04.2022 um 22:56 +0200 schrieb Ludovic Courtès:
Toggle quote (6 lines)
>
> > Interestingly, the running value for the mpd service remains
> > (("unknown" . "#<input-output: socket 17>")) even after MPD
> > started. Should that be the case?
>
> No.
It turns out the issue here was that I rewrote the service to put its
log into XDG_STATE_HOME as well, but under a directory that did not yet
exist. #55080 addresses this.

Cheers
L
L
Ludovic Courtès wrote on 13 May 17:55 +0200
(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)
87ee0xfmvb.fsf_-_@gnu.org
Hi Liliana,

What’s the status of this patch series? Would be nice to have it in!

Ludo’.

Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (29 lines)
> Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:
>
>> * gnu/services/audio.scm (<mpd-configuration>)[shepherd-endpoints]: New field.
>> (mpd-shepherd-service): Use it.
>> * doc/guix.texi (Music Player Daemon): Document it.
>
> [...]
>
>> +++ b/gnu/services/audio.scm
>> @@ -78,6 +78,8 @@ (define-record-type* <mpd-configuration>
>> (default "6600"))
>> (address mpd-configuration-address
>> (default "any"))
>> + (shepherd-endpoints mpd-configuration-shepherd-endpoints
>> + (default '())) ; list of <shepherd-endpoint>
>
> The way I see it, service configuration should be oblivious to whether
> it’s started as “forkexec”, systemd, or inetd.
>
> There’s already an ‘address’ field above, so my suggestion would be to
> reuse it. This is what I did for example for the openssh service, and
> also for bitlbee and dicod here:
>
> https://issues.guix.gnu.org/54997#5
>
> WDYT?
>
> Thanks,
> Ludo’.
L
L
Liliana Marie Prikler wrote on 13 May 19:00 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)
d3bb074bff53a0503742b953762ac46de5f0b9cc.camel@gmail.com
Am Freitag, dem 13.05.2022 um 17:55 +0200 schrieb Ludovic Courtès:
Toggle quote (3 lines)
> Hi Liliana,
>
> What’s the status of this patch series?  Would be nice to have it in!
The patch for MPD itself is good to go as far as I'm aware, the patch
for the service type is work in progress.

This question
Toggle quote (3 lines)
> Ludovic Courtès <ludo@gnu.org> skribis:
> > The way I see it, service configuration should be oblivious to
> > whether it’s started as “forkexec”, systemd, or inetd.
has me in a bit of a bind in multiple ways. For one, I don't see a
direct translation from MPD's configuration scheme to shepherd's. For
another, the distinct semantics between forkexec and lazy loading cause
observable differences as in “What the fuck, I only ran mpc status, why
is the music now playing?” – this can be avoided if the user knows that
shepherd will be lazy-loading mpd and that it might as a result to
starting start playing on the first received command.

Moreover, I think the patch I added to make endpoint records
configurable from Guix could also serve to solve other bugs, e.g. SSH
only listening on IPv4 addresses, which would require us to be able to
specify whether to listen on IPv4, IPv6 or both through Guix. Long
term, I think we should only keep the distinction between forkexec and
inetd/systemd for those services where it makes an observable
difference, like mpd. For SSH, apart from bugs and perhaps people
using old shepherd, I don't think there'd be any reason to keep
forkexec beyond a certain point (in a future as distant as you would
want it to be).

Maxime also raised hygiene concerns that would be comparatively easy to
solve.

TL;DR: v2 [1/3] is good, [2/3 WIP] and [3/3 WIP] should be kept back
for now.

Cheers
?