Conversation
1ef056a to
605b8c4
Compare
|
When you're done making changes, you may want to consider squashing some of your commits. See https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions |
|
I know these conventions, and they are under (inactive) debate in: I intentionally separated the commits as they are separated now. If you have any specific suggestions please shared them with me :). As for the actual changes of the PR: I'm running my system with this patch for a few days now with no issues, so a review of the implementation is more then welcome. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-december/1711/34 |
emmanuelrosa
left a comment
There was a problem hiding this comment.
Looks good to me.
Awesome work, @doronbehar.
ee74995 to
7876189
Compare
nixos/modules/services/audio/mpd.nix
Outdated
| { | ||
| mpd_key = "decoder"; | ||
| plugin = "fluidsynth"; | ||
| soundfont = "${pkgs.soundfont-fluid}/share/soundfonts/FluidR3_GM2-2.sf2"; | ||
| } | ||
| ''} | ||
|
|
||
| ${lib.optionalString (cfg.credentials != [ ]) (credentialsPlaceholder cfg.credentials)} | ||
|
|
||
| ${cfg.extraConfig} | ||
| ''; | ||
| ]; | ||
| mpdConf = pkgs.writeText "mpd.conf" ( | ||
| '' | ||
| # This file was automatically generated by NixOS. Edit mpd's configuration | ||
| # via NixOS' configuration.nix, as this file will be rewritten upon mpd's | ||
| # restart. | ||
| '' | ||
| + lib.concatStringsSep "\n" ( | ||
| mkKeyValue ( | ||
| { | ||
| state_file = "${cfg.dataDir}/state"; | ||
| sticker_file = "${cfg.dataDir}/sticker.sql"; | ||
| } |
There was a problem hiding this comment.
I think the logic here would be a bit clearer if these defaults were set through config instead, and have this part only be the config generation logic.
There was a problem hiding this comment.
I think the logic here would be a bit clearer if these defaults were set through
configinstead, and have this part only be the config generation logic.
I too was thinking about this, but the problem is that the serviceConfig.StateDirectory uses this dataDir, and I'm afraid to break configs that depend on permissions there etc. If you have an idea how to do this smoothly, it'd be nice.
There was a problem hiding this comment.
I too was thinking about this, but the problem is that the serviceConfig.StateDirectory uses this dataDir, and I'm afraid to break configs that depend on permissions there etc. If you have an idea how to do this smoothly, it'd be nice.
To be honest I didn't quite understand the relation to StateDirectory here. The suggestion was to set the defaults by setting servives.mpd.settings in config, and I don't see how it would change the logic much from adding the defaults in the config file expression instead. Maybe we were talking past each other.
Posted a related patch in the other comment with the concrete suggestion :)
There was a problem hiding this comment.
To be honest I didn't quite understand the relation to
StateDirectoryhere.
We evaluate cfg.dataDir when evaluating serviceConfig.StateDirectory... We can continue discuss this downwards.
nixos/modules/services/audio/mpd.nix
Outdated
| ${cfg.extraConfig} | ||
| ''; | ||
| ]; | ||
| mpdConf = pkgs.writeText "mpd.conf" ( |
There was a problem hiding this comment.
It would be kinda cool if we could validate this file. Maybe it's possible to run mpd with the config file through gnu expect in the nix sandbox during a check phase, and see if it throws a config-related error. Can also be left for another time if you'd rather get the PR through as is.
There was a problem hiding this comment.
It would be kinda cool if we could validate this file. Maybe it's possible to run mpd with the config file through gnu expect in the nix sandbox during a check phase, and see if it throws a config-related error.
That indeed sounds cool, but I think it requires IFD. Do you have an example where this is being done in a different module?
There was a problem hiding this comment.
I played a bit around, and made a patch with a few suggested changes
- Move the defaults out of the config file generation logic
- Simplify the config file generation logic a bit
- Move the file config file derivation down to around where it is used.
- Add a checkphase that runs
mpdin expect, and fails on error messages related to the config file (but succeeds on unrelated errors) - Move the
atomTypedown to where it is used, and fix thefreeformType - Fix a typo
patch
diff --git i/nixos/modules/services/audio/mpd.nix w/nixos/modules/services/audio/mpd.nix
index 9ad1e9cc5848..b69fee1a7297 100644
--- i/nixos/modules/services/audio/mpd.nix
+++ w/nixos/modules/services/audio/mpd.nix
@@ -11,74 +11,6 @@ let
uid = config.ids.uids.mpd;
gid = config.ids.gids.mpd;
cfg = config.services.mpd;
- atomType =
- with lib.types;
- oneOf [
- str
- int
- bool
- path
- ];
-
- mkKeyValue =
- a:
- lib.mapAttrsToList (
- k: v:
- k
- + " "
- + (
- if builtins.isBool v then
- # Mainly for https://mpd.readthedocs.io/en/stable/user.html#zeroconf
- "\"" + (lib.boolToYesNo v) + "\""
- else
- "\"" + (toString v) + "\""
- )
- ) a;
- nonBlockSettings = lib.filterAttrs (n: v: !(builtins.isAttrs v || builtins.isList v)) cfg.settings;
- pureBlockSettings = builtins.removeAttrs cfg.settings (builtins.attrNames nonBlockSettings);
- fluidsynthBlock = [
- {
- plugin = "fluidsynth";
- soundfont = "${pkgs.soundfont-fluid}/share/soundfonts/FluidR3_GM2-2.sf2";
- }
- ];
- blocks =
- pureBlockSettings
- // lib.optionalAttrs cfg.fluidsynth {
- decoder = (pureBlockSettings.decoder or [ ]) ++ fluidsynthBlock;
- };
- processSingleBlock =
- n: v:
- [
- (n + " {")
- ]
- # Add indentation, for better readability
- ++ (map (l: " " + l) (mkKeyValue v))
- ++ [ "}" ];
- mpdConf = pkgs.writeText "mpd.conf" (
- ''
- # This file was automatically generated by NixOS. Edit mpd's configuration
- # via NixOS' configuration.nix, as this file will be rewritten upon mpd's
- # restart.
- ''
- + lib.concatStringsSep "\n" (
- mkKeyValue (
- {
- state_file = "${cfg.dataDir}/state";
- sticker_file = "${cfg.dataDir}/sticker.sql";
- }
- // nonBlockSettings
- )
- ++ lib.flatten (
- lib.mapAttrsToList (
- n: v: if builtins.isList v then (map (b: processSingleBlock n b) v) else (processSingleBlock n v)
- ) blocks
- )
- ++ lib.imap0 (
- i: a: "password \"{{password-${toString i}}}@${lib.concatStringsSep "," a.permissions}\""
- ) cfg.credentials
- )
- );
in
{
@@ -147,11 +79,19 @@ in
freeformType =
let
inherit (lib.types) oneOf attrsOf listOf;
+ atomType =
+ with lib.types;
+ oneOf [
+ str
+ int
+ bool
+ path
+ ];
in
- oneOf [
+ attrsOf (oneOf [
atomType
- (attrsOf (listOf (attrsOf atomType)))
- ];
+ (listOf (attrsOf atomType))
+ ]);
options = {
music_directory = lib.mkOption {
type = with lib.types; either path (strMatching "([a-z]+)://.+");
@@ -233,7 +173,7 @@ in
Can be inserted with:
```nix
- autio_output = [
+ audio_output = [
{
type = "alsa";
name = "My specific ALSA output";
@@ -347,6 +287,16 @@ in
];
config = lib.mkIf cfg.enable {
+ services.mpd.settings = {
+ state_file = lib.mkDefault "${cfg.dataDir}/state";
+ sticker_file = lib.mkDefault "${cfg.dataDir}/sticker.sql";
+ decoder = lib.mkIf cfg.fluidsynth [
+ {
+ plugin = "fluidsynth";
+ soundfont = "${pkgs.soundfont-fluid}/share/soundfonts/FluidR3_GM2-2.sf2";
+ }
+ ];
+ };
# install mpd units
systemd.packages = [ pkgs.mpd ];
@@ -366,43 +316,115 @@ in
];
};
- systemd.services.mpd = {
- wantedBy = lib.optional (!cfg.startWhenNeeded) "multi-user.target";
-
- preStart = ''
- set -euo pipefail
- install -m 600 ${mpdConf} /run/mpd/mpd.conf
- ''
- + lib.optionalString (cfg.credentials != [ ]) (
- lib.concatStringsSep "\n" (
- lib.imap0 (
- i: c:
- ''${pkgs.replace-secret}/bin/replace-secret '{{password-${toString i}}}' '${c.passwordFile}' /run/mpd/mpd.conf''
- ) cfg.credentials
- )
- );
-
- serviceConfig = {
- User = "${cfg.user}";
- # Note: the first "" overrides the ExecStart from the upstream unit
- ExecStart = [
- ""
- "${pkgs.mpd}/bin/mpd --systemd /run/mpd/mpd.conf"
- ];
- RuntimeDirectory = "mpd";
- StateDirectory =
- [ ]
- ++ lib.optionals (cfg.dataDir == "/var/lib/${name}") [ name ]
- ++ lib.optionals (cfg.settings.playlist_directory == "/var/lib/${name}/playlists") [
- name
- "${name}/playlists"
- ]
- ++ lib.optionals (cfg.settings.music_directory == "/var/lib/${name}/music") [
- name
- "${name}/music"
+ systemd.services.mpd =
+ let
+ mpdConf =
+ let
+ nonBlockSettings = lib.filterAttrs (_k: v: !(builtins.isList v)) cfg.settings;
+ blockSettings = builtins.removeAttrs cfg.settings (builtins.attrNames nonBlockSettings);
+
+ renderNonBlock = k: v: ''${k} "${if builtins.isBool v then lib.boolToYesNo v else toString v}"'';
+ renderBlock = n: v: ''
+ ${n} {
+ ${lib.concatMapStringsSep "\n" (l: " " + l) (lib.mapAttrsToList renderNonBlock v)}
+ }
+ '';
+
+ configContent = ''
+ # This file was automatically generated by NixOS. Edit mpd's configuration
+ # via NixOS' configuration.nix, as this file will be rewritten upon mpd's
+ # restart.
+ ''
+ + lib.concatStringsSep "\n" (
+ (lib.mapAttrsToList renderNonBlock nonBlockSettings)
+ ++ (lib.concatLists (lib.mapAttrsToList (n: v: map (b: renderBlock n b) v) blockSettings))
+ ++ lib.imap0 (
+ i: a: "password \"{{password-${toString i}}}@${lib.concatStringsSep "," a.permissions}\""
+ ) cfg.credentials
+ );
+ in
+ pkgs.stdenvNoCC.mkDerivation {
+ name = "mpd.conf";
+ preferLocalBuild = true;
+
+ dontUnpack = true;
+ dontConfigure = true;
+ dontBuild = true;
+
+ expectScript = ''
+ spawn mpd --no-daemon "$env(configFilePath)"
+ expect {
+ "exception: Error in \"$env(configFilePath)\"" {
+ puts "Config file invalid\n"
+ exit 1
+ }
+ "exception:" {
+ exit 0
+ }
+ }
+ '';
+
+ configFile = configContent;
+ passAsFile = [
+ "expectScript"
+ "configFile"
+ ];
+
+ doCheck = true;
+ nativeCheckInputs = [
+ pkgs.expect
+ pkgs.mpd
+ ];
+ checkPhase = ''
+ runHook preCheckPhase
+ expect -f "$expectScriptPath"
+ runHook postCheckPhase
+ '';
+
+ installPhase = ''
+ runHook preCheckPhase
+ install -Dm444 "$configFilePath" "$out"
+ runHook postCheckPhase
+ '';
+ };
+ in
+ {
+ wantedBy = lib.optional (!cfg.startWhenNeeded) "multi-user.target";
+
+ preStart = ''
+ set -euo pipefail
+ install -m 600 ${mpdConf} /run/mpd/mpd.conf
+ ''
+ + lib.optionalString (cfg.credentials != [ ]) (
+ lib.concatStringsSep "\n" (
+ lib.imap0 (
+ i: c:
+ ''${pkgs.replace-secret}/bin/replace-secret '{{password-${toString i}}}' '${c.passwordFile}' /run/mpd/mpd.conf''
+ ) cfg.credentials
+ )
+ );
+
+ serviceConfig = {
+ User = "${cfg.user}";
+ # Note: the first "" overrides the ExecStart from the upstream unit
+ ExecStart = [
+ ""
+ "${pkgs.mpd}/bin/mpd --systemd /run/mpd/mpd.conf"
];
+ RuntimeDirectory = "mpd";
+ StateDirectory =
+ [ ]
+ ++ lib.optionals (cfg.dataDir == "/var/lib/${name}") [ name ]
+ ++ lib.optionals (cfg.settings.playlist_directory == "/var/lib/${name}/playlists") [
+ name
+ "${name}/playlists"
+ ]
+ ++ lib.optionals (cfg.settings.music_directory == "/var/lib/${name}/music") [
+ name
+ "${name}/music"
+ ];
+ };
};
- };
networking.firewall.allowedTCPPorts = lib.optionals cfg.openFirewall [ cfg.settings.port ];There was a problem hiding this comment.
+ attrsOf (oneOf [ atomType - (attrsOf (listOf (attrsOf atomType))) - ]; + (listOf (attrsOf atomType)) + ]);
Thanks!
+ ++ lib.optionals (cfg.dataDir == "/var/lib/${name}") [ name ]
Doesn't it mean you still depend on a dataDir configuration value? I thought you wish to get rid of it in favor of only state_file and sticker_file.
+ expectScript = '' + spawn mpd --no-daemon "$env(configFilePath)" + expect { + "exception: Error in \"$env(configFilePath)\"" { + puts "Config file invalid\n" + exit 1 + } + "exception:" { + exit 0 + } + } + ''; + + configFile = configContent; + passAsFile = [ + "expectScript" + "configFile" + ]; + + doCheck = true; + nativeCheckInputs = [ + pkgs.expect + pkgs.mpd + ]; + checkPhase = '' + runHook preCheckPhase + expect -f "$expectScriptPath" + runHook postCheckPhase + ''; + + installPhase = '' + runHook preCheckPhase + install -Dm444 "$configFilePath" "$out" + runHook postCheckPhase + '';
Nice! I used your expectScript and used pkgs.writeTextFile instead of pkgs.writeText, which allows specifying checkPhase and the other derivationArgs.
+ renderNonBlock = k: v: ''${k} "${if builtins.isBool v then lib.boolToYesNo v else toString v}"''; + renderBlock = n: v: '' + ${n} { + ${lib.concatMapStringsSep "\n" (l: " " + l) (lib.mapAttrsToList renderNonBlock v)} + } + '';
This seems perfectly fine, but I hope you'd forgive me for not incorporating these signatures.
+ decoder = lib.mkIf cfg.fluidsynth [ + { + plugin = "fluidsynth"; + soundfont = "${pkgs.soundfont-fluid}/share/soundfonts/FluidR3_GM2-2.sf2"; + } + ]; + };
Are you sure this works better then my code? It seems that defining multiple decoders along with services.mpd.fluidsynth? Why not handle cfg.fluidsynth before processing the blocks?
- autio_output = [ + audio_output = [
Thanks!
- Move the
atomTypedown to where it is used
(EDITED) I have incorporated this change, thanks too!
- Move the file config file derivation down to around where it is used.
(EDITED) I haven't incorporated this change, to make the diff as small as possible.
There was a problem hiding this comment.
+ ++ lib.optionals (cfg.dataDir == "/var/lib/${name}") [ name ]Doesn't it mean you still depend on a
dataDirconfiguration value? I thought you
wish to get rid of it in favor of onlystate_fileandsticker_file.
I think this piece of code is only part of the diff due to the indent. I wasn't trying to get rid of dataDir - rather I was trying to separate the default values of state_file, sticker_file and the fluid synth decoder from the code that renders the config file.
+ decoder = lib.mkIf cfg.fluidsynth [ + { + plugin = "fluidsynth"; + soundfont = "${pkgs.soundfont-fluid}/share/soundfonts/>FluidR3_GM2-2.sf2"; + } + ]; + };Are you sure this works better then my code? It seems that defining multiple decoders
along withservices.mpd.fluidsynth? Why not handlecfg.fluidsynthbefore
processing the blocks?
Same thing here, this is just moving this piece of code out of the config rendering step to make it cleaner. The only intended effect is to enhance code readability, so I'm not sure I'd say it "works better".
fluidsynthBlock = [
{
plugin = "fluidsynth";
soundfont = "${pkgs.soundfont-fluid}/share/soundfonts/FluidR3_GM2-2.sf2";
}
];
blocks =
pureBlockSettings
// lib.optionalAttrs cfg.fluidsynth {
decoder = (pureBlockSettings.decoder or [ ]) ++ fluidsynthBlock;
};The result should be the exact same, it's just that the values are passed through services.mpd.settings instead of being added alongside services.mpd.settings. state_file and sticker_file is still overridable by the user setting them themselves, and if the user sets services.mpd.settings.decoder the module system should merge the lists (similarly to before the change).
The config file generator will then be able to just assume that these values are already present in settings and can be written solely with config rendering in mind, instead of having to add these values in the middle of the expression.
Whether or not the state_file and sticker_file refers to dataDir shouldn't really have any effect on this, which is why I'm a bit confused. I think we might be talking about two different things?
There was a problem hiding this comment.
With regards to the way the decoder blocks are assigned in this idea:
config = lib.mkIf cfg.enable { + services.mpd.settings = { + state_file = lib.mkDefault "${cfg.dataDir}/state"; + sticker_file = lib.mkDefault "${cfg.dataDir}/sticker.sql"; + decoder = lib.mkIf cfg.fluidsynth [ + { + plugin = "fluidsynth"; + soundfont = "${pkgs.soundfont-fluid}/share/soundfonts/FluidR3_GM2-2.sf2"; + } + ]; + };
@h7x4 I'm afraid I don't feel comfortable writing this, as I don't truly understand how the decoder is being set really... I will probably become the main maintainer of this module in the near future so I think it'll be better that everything will be intuitive for me there.
As for the cfg.dataDir, I too think we don't understand each other: What I thought you suggested initially, was to ditch cfg.dataDir in favor of two new settings state_file and sticker_file. If I understand you correctly, what you are suggesting is to allow the users to manually specify the {state,sticker}_files, but I think that even now this is supported thanks to:
The previous track was 404 probably due to changes in the website.
16b02d4 to
b0ebbb5
Compare
b0ebbb5 to
9e0a983
Compare
tobim
left a comment
There was a problem hiding this comment.
While I unfortunately don't have capacity to review such a big change at the moment I still wanted to say I wholeheartedly support this effort! ❤️
tobim
left a comment
There was a problem hiding this comment.
Tested this branch with this config in conjunction with with snapserver and it works fine:
services.mpd = {
enable = true;
musicDirectory = "/media/music";
network.listenAddress = "any";
openFirewall = true;
settings.audio_output = [
{
type = "fifo";
name = "snapcast";
path = "/run/snapserver/mpd";
format = "48000:16:2";
mixer_type = "software";
}
];
};5503de6 to
a4e827e
Compare
Also use `config.services.mpd.settings` directly in the documentation of mpdscribble.
|
I'd like to merge this in a day or two if there's no further objection. |
| && !cfg.openFirewall | ||
| ) | ||
| "Using '${cfg.bind_to_address}' as services.mpd.settings.bind_to_address without enabling services.mpd.openFirewall, might prevent you from accessing MPD from other clients."; | ||
|
|
There was a problem hiding this comment.
This doesn't evaluate as cfg.bind_to_address doesn't exist, bind_to_address can also be a unix domain socket.
There was a problem hiding this comment.
This doesn't evaluate as
cfg.bind_to_addressdoesn't exist,bind_to_addresscan also be a unix domain socket.
Thanks @gravndal for the quick comment! Here's a fix:
There was a problem hiding this comment.
I'm using other means to access mpd, how can I remove this warning without setting services.mpd.openFirewall = true;?
evaluation warning: Using 'any' as services.mpd.settings.bind_to_address without enabling services.mpd.openFirewall, might prevent you from accessing MPD from other clients.
There was a problem hiding this comment.
I'm using other means to access mpd, how can I remove this warning without setting
services.mpd.openFirewall = true;?evaluation warning: Using 'any' as services.mpd.settings.bind_to_address without enabling services.mpd.openFirewall, might prevent you from accessing MPD from other clients.
How do you access mpd?
There was a problem hiding this comment.
Only over tailscale:
networking.firewall.trustedInterfaces = [
config.services.tailscale.interfaceName
];
There was a problem hiding this comment.
BTW are you absolutely sure that using 127.0.0.1 will block you from access the mpd server?
There was a problem hiding this comment.
If you could share more details I might be able to help. I'm not familiar with tailscale and from skimming over its readme it is not trivially clear to me.
It's a VPN. I don't want to expose mpd to non-tailscale networks.
BTW are you absolutely sure that using
127.0.0.1will block you from access the mpd server?
I won't be able to access it from other devices if it listens on localhost.
There was a problem hiding this comment.
OK I'm just surprised that you are able to access it now - with any and from other hosts on the VPN. Not sure how to help.
There was a problem hiding this comment.
I'm using networking.firewall.trustedInterfaces to allow all ports to be available on my tailscale network. I just don't want to see any warnings. This is the only module that I use which has this issue.
There was a problem hiding this comment.
I actually have the same problem.
without enabling services.mpd.openFirewall, might prevent you from accessing MPD from other clients.
I think that warning is a little unexpected for me for nixpkgs services?
mpd.openFirewall opens the wirewall on all interfaces.
A lot of services have this as a convenience function, but also usually have the config.port setting, which consumers can use to manually open the port on specific interfaces.
My preference would be to remove the warning alltogether.
Alternatives:
We could check every interface and if config.port is enabled on one of them.
Fixes NixOS#484912 and addresses the comments here: NixOS#456989 (comment)
Fixes NixOS#484912 and addresses the comments here: NixOS#456989 (comment)
Fixes NixOS#484912 and addresses the comments here: NixOS#456989 (comment)
Fixes NixOS#484912 and addresses the comments here: NixOS#456989 (comment)
Description of changes
Things done
nix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.