| Message ID | cover.1670368385.git.mirai@makinata.eu |
|---|---|
| Headers | show |
| Series | services: mpd: Refactor MPD service | expand |
On Tue Dec 6, 2022 at 11:22 PM GMT, wrote: > Modernizes and expands 'mpd-service-type'. > Performs "pretty-formatting" to the generated configuration file > at the cost of a few extra procedures. > It also deprecates some of the fields (abbreviated forms). > > Bruno Victal (2): > services: mpd: use 'define-configuration'. > services: mpd: Refactor MPD service. > > doc/guix.texi | 172 +++++++++++-- > gnu/services/audio.scm | 549 ++++++++++++++++++++++++++++++----------- > 2 files changed, 556 insertions(+), 165 deletions(-) > > > base-commit: b94724e8b2102be0fe9d19e9dfe44d6f7101bd4b > -- > 2.38.1 Could you also add support for ``herd configuration'' (added in #59197)? -- (
It already does! The second commit introduces the bulk of the changes :) On 2022-12-07 08:59, ( wrote: > On Tue Dec 6, 2022 at 11:22 PM GMT, wrote: >> Modernizes and expands 'mpd-service-type'. >> Performs "pretty-formatting" to the generated configuration file >> at the cost of a few extra procedures. >> It also deprecates some of the fields (abbreviated forms). >> >> Bruno Victal (2): >> services: mpd: use 'define-configuration'. >> services: mpd: Refactor MPD service. >> >> doc/guix.texi | 172 +++++++++++-- >> gnu/services/audio.scm | 549 ++++++++++++++++++++++++++++++----------- >> 2 files changed, 556 insertions(+), 165 deletions(-) >> >> >> base-commit: b94724e8b2102be0fe9d19e9dfe44d6f7101bd4b >> -- >> 2.38.1 > > Could you also add support for ``herd configuration'' (added in #59197)? > -- (
Heya,
On Wed Dec 7, 2022 at 1:42 PM GMT, mirai wrote:
> It already does! The second commit introduces the bulk of the changes :)
Oops :) Reviewing now...
-- (
Am Dienstag, dem 06.12.2022 um 23:22 +0000 schrieb mirai@makinata.eu: > Modernizes and expands 'mpd-service-type'. > Performs "pretty-formatting" to the generated configuration file > at the cost of a few extra procedures. > It also deprecates some of the fields (abbreviated forms). > > Bruno Victal (2): > services: mpd: use 'define-configuration'. > services: mpd: Refactor MPD service. Note that there is [1], which attempts to make it so that shepherd endpoints can be specified in lieu of MPD endpoints. You don't need to implement this logic – you are free to do so if you want to – but could you make it so that there is an explicit endpoint abstraction that would allow for extension later on? Cheers [1] https://issues.guix.gnu.org/54986
On 2022-12-07 18:27, Liliana Marie Prikler wrote: > Note that there is [1], which attempts to make it so that shepherd > endpoints can be specified in lieu of MPD endpoints. You don't need to > implement this logic – you are free to do so if you want to – but could > you make it so that there is an explicit endpoint abstraction that > would allow for extension later on? > > Cheers > > [1] https://issues.guix.gnu.org/54986 Hi, After reading issue #54986, regarding mpd escaping shepherd management, my guess is that there could be two issues at play here: * mpd.conf was serialized with pid_file [1]. The safest is to actually not serialize any unused directives and let MPD decide based on what's actually present. pid_file should only be set if MPD is to be launched as a "daemon process". [2] * But the service definition for MPD is launched with '--no-daemon' option as seen in [3], which could be triggering a bug in MPD. It's probably worth testing the combinations of having pid_file set and invoking --no-daemon. > but could > you make it so that there is an explicit endpoint abstraction that > would allow for extension later on? If I'm understanding correctly what [1] is about, I think this can be implemented through the 'extend' interface. See how certbot adds a nginx-server-configuration to a nginx-service-type through the 'extend' interface. For MPD, this would amount to appending the 'adresses' field with "adequately formatted" values. [1]: https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/services/audio.scm?id=1b6172d5f6ca22f0fa02cd1335b1b90e9501b731#n116 [2]: https://mpd.readthedocs.io/en/latest/user.html#starting-and-stopping-mpd [3]: https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/services/audio.scm?id=1b6172d5f6ca22f0fa02cd1335b1b90e9501b731#n145
I've found out that this service (even before this patch-set)
is biased towards being used as a home service rather than a system service.
The reason is that it sets the XDG_RUNTIME_DIR environment variable through shepherd
to:
--8<---------------cut here---------------start------------->8---
(list (string-append
"XDG_RUNTIME_DIR=/run/user/"
(number->string (passwd:uid (getpwnam #$user)))))
--8<---------------cut here---------------end--------------->8---
This directory does not exist if this is launched as a system-wide service.
I presume that this went unnoticed because most of the times you'd want to
use this service system-wide you also configure this either as a MPD
'satellite instance' or the audio-outputs are always network-streaming ones.
This falls apart if you configure a system-wide mpd-service with a pulseaudio
output as it will try to access XDG_RUNTIME_DIR which is not created for the
system? #t 'mpd' user.
Now, pulseaudio is usually launched as a 'per-user' daemon although it can
be used in a system-pulse configuration though this is strongly discouraged.
Under most conditions (pulseaudio config mostly unchanged),
a system-wide mpd-service-type is also able to launch its own pulse instance
but due to the inherent assumptions present in the original service we have that:
- The XDG_RUNTIME_DIR env var is set when it should only be set for 'system? #f' users.
- PULSE_CLIENTCONFIG and PULSE_CONFIG are not set by shepherd.
These usually correspond to:
PULSE_CONFIG =/etc/pulse/client.conf
PULSE_CONFIG=/etc/pulse/daemon.conf
But if you use mpd-service-type as a home service, you could also have
these set to a user-specific pulse config, which will be overridden by shepherd
if these are set in mpd-service-type.
But again, if you don't set these env vars and use it as a systemwide service
with a pulseaudio output and you workaround the XDG_RUNTIME_DIR by unsetting it,
your pulse daemon won't read its config and there will be no output.
This is not just a theoretical issue, consider this snippet:
--8<---------------cut here---------------start------------->8---
;; Set a systemwide mpd service that streams with pulseaudio RTP output.
(service pulseaudio-service-type (pulseaudio-configuration
(extra-script-files
(list
(plain-file
"rtp.pa"
(string-join (list "load-module module-null-sink sink_name=rtp"
"load-module module-rtp-send source=rtp.monitor"
"set-default-sink rtp") "\n" 'suffix))))))
(service mpd-service-type
(mpd-configuration
(outputs (list (mpd-output (name "Pulseaudio over RTP")
(type "pulse")
(extra-options '((sink . "rtp")))))))
--8<---------------cut here---------------end--------------->8---
The problem here is how to conditionally select which environment variables should be set based
on whether this is being launched as a system-wide or home-instance service. I'm all ears for
suggestions.
Regarding the current patch-set, I'm thinking that some of the non-public procedures could
be defined with 'define-deprecated' instead which should (?) be cleaner than the current
comments that are demarcating deprecated code.
Regards,
Bruno
Am Dienstag, dem 03.01.2023 um 14:43 +0000 schrieb mirai: > I've found out that this service (even before this patch-set) > is biased towards being used as a home service rather than a system > service. > > The reason is that it sets the XDG_RUNTIME_DIR environment variable > through shepherd > to: > > --8<---------------cut here---------------start------------->8--- > (list (string-append > "XDG_RUNTIME_DIR=/run/user/" > (number->string (passwd:uid (getpwnam #$user))))) > --8<---------------cut here---------------end--------------->8--- > > This directory does not exist if this is launched as a system-wide > service. > I presume that this went unnoticed because most of the times you'd > want to use this service system-wide you also configure this either > as a MPD 'satellite instance' or the audio-outputs are always > network-streaming ones. > > This falls apart if you configure a system-wide mpd-service with a > pulseaudio output as it will try to access XDG_RUNTIME_DIR which is > not created for the system? #t 'mpd' user. As far as I'm aware, you can instantiate the MPD service system-wide while also pointing it to a non-system user. Admittedly, this is a somewhat degenerate use case, but that's how things were handled before home services were a thing, and there might still be valid reasons to support it. > Now, pulseaudio is usually launched as a 'per-user' daemon although > it can be used in a system-pulse configuration though this is > strongly discouraged. If it is, then XDG_RUNTIME_DIR doesn't matter. > Under most conditions (pulseaudio config mostly unchanged), > a system-wide mpd-service-type is also able to launch its own pulse > instance but due to the inherent assumptions present in the original > service we have that: > - The XDG_RUNTIME_DIR env var is set when it should only be set for > 'system? #f' users. > - PULSE_CLIENTCONFIG and PULSE_CONFIG are not set by shepherd. > These usually correspond to: > PULSE_CONFIG =/etc/pulse/client.conf > PULSE_CONFIG=/etc/pulse/daemon.conf > But if you use mpd-service-type as a home service, you could > also have > these set to a user-specific pulse config, which will be > overridden by shepherd > if these are set in mpd-service-type. > But again, if you don't set these env vars and use it as a > systemwide service > with a pulseaudio output and you workaround the XDG_RUNTIME_DIR > by unsetting it, > your pulse daemon won't read its config and there will be no > output. 何？ > This is not just a theoretical issue, consider this snippet: > > --8<---------------cut here---------------start------------->8--- > ;; Set a systemwide mpd service that streams with pulseaudio RTP > output. > > (service pulseaudio-service-type (pulseaudio-configuration > (extra-script-files > (list > (plain-file > "rtp.pa" > (string-join (list "load- > module module-null-sink sink_name=rtp" > "load- > module module-rtp-send source=rtp.monitor" > "set- > default-sink rtp") "\n" 'suffix)))))) > (service mpd-service-type > (mpd-configuration > (outputs (list (mpd-output (name "Pulseaudio over > RTP") > (type "pulse") > (extra-options '((sink . > "rtp"))))))) > --8<---------------cut here---------------end--------------->8--- > > The problem here is how to conditionally select which environment > variables should be set based on whether this is being launched as a > system-wide or home-instance service. I'm all ears for > suggestions. I don't think you need to think about the home service too hard here. If you can't get the two use cases to work together, you can define a home-mpd-service-type which takes a regular mpd-configuration or a particularly crafted home-mpd-configuration, whichever makes more sense. Not saying that this would be a good design idea, just that the option exists. I do think trying to figure out what you're going to do with your environment variables is a more worthwhile exercise at the moment. > Regarding the current patch-set, I'm thinking that some of the non- > public procedures could be defined with 'define-deprecated' instead > which should (?) be cleaner than the current > comments that are demarcating deprecated code. If they aren't public, what is the point of define-deprecated? You will only trip over your own uses. Cheers