spotifyd: init at 0.2.4 #55923
spotifyd: init at 0.2.4 #55923
Conversation
| }; | ||
|
|
||
| users = { | ||
| users.spotifyd = { |
There was a problem hiding this comment.
You probably should use systemd's DynamicUser and probably StateDirectory as well.
| homepage = https://github.com/Spotifyd/spotifyd; | ||
| license = with licenses; [ gpl3 ]; | ||
| maintainers = [ maintainers.jmgrosen ]; | ||
| platforms = platforms.all; |
There was a problem hiding this comment.
I think buildRustPackage sets this for us, also I don't think this would build for all platforms anyway.
There was a problem hiding this comment.
I can't find anywhere that suggests buildRustPackage does this -- at the least, the example in the nixpkgs manual still has a meta section. I added (what I think should be) darwin support in my latest commit, so I think platforms.all is appropriate too.
There was a problem hiding this comment.
I can't find anywhere that suggests
buildRustPackagedoes this -- at the least, the example in the nixpkgs manual still has a meta section.
Ahh, yes that is correct.
. I added (what I think should be) darwin support in my latest commit, so I think
platforms.allis appropriate too.
I think all is way too broad in this context. If you want darwin and linux support you can use unix.
|
Thanks for all your helpful comments, @worldofpeace! |
| description = "The directory where spotifyd stores its state, relative to /var/lib/."; | ||
| }; | ||
|
|
||
| config = mkOption { |
There was a problem hiding this comment.
could this be made an attrset instead, which gets assembled into config below the [global] section?
There was a problem hiding this comment.
Or maybe just generate the ini with lib.generators.toINI?
There was a problem hiding this comment.
yeah, that's what I meant :-)
| dataDir = mkOption { | ||
| default = "spotifyd"; | ||
| type = types.str; | ||
| description = "The directory where spotifyd stores its state, relative to /var/lib/."; |
There was a problem hiding this comment.
do we need to make this configurable? Wonder about the usecases, given it's a unit with dynamic users and the "state" should be mostly caches, right?
| after = [ "network.target" "sound.target" ]; | ||
| description = "spotifyd, a Spotify playing daemon"; | ||
| serviceConfig = { | ||
| ExecStart = "${pkgs.spotifyd}/bin/spotifyd --no-daemon --config ${spotifydConf}"; |
There was a problem hiding this comment.
We probably don't need to pass --no-daemon - systemd journal should capture syslog() calls.
| RestartSec = 12; | ||
| DynamicUser = true; | ||
| StateDirectory = "${cfg.dataDir}"; | ||
| Environment = "HOME=/var/lib/${cfg.dataDir}"; |
There was a problem hiding this comment.
Is that only to set cache_path? We could probably also use the corresponding command line option.
|
@jmgrosen can you adress the comments? |
|
Any progress on this? |
| # buildRustPackage currently seems to have some problems with [replace] | ||
| # directives in Cargo files (see #30742 and #47172), so don't use the | ||
| # replacement. | ||
| cargoPatches = [ ./noavxcrypto.patch ]; |
There was a problem hiding this comment.
spotifyd 0.2.11 was released and doesn't required this marsam@179aaa85c43
Motivation for this change
Spotifyd is a nice spotify connect server. I worked around the issues in #47172 by just not using the avx version of rust-crypto (achieved through a patch).
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)(Apologies if I've done something wrong, this is my first submitted PR.)