Add mpdscribble service #23660
Add mpdscribble service #23660
Conversation
|
@matthiasbeyer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @offlinehacker and @bjornfor to be potential reviewers. |
| description = '' | ||
| HTTP proxy URL. | ||
| ''; | ||
| } |
| description = '' | ||
| The location of the pid file. mpdscribble saves its process id there. | ||
| ''; | ||
| } |
| enable = mkOption { | ||
| default = false; | ||
| type = types.bool; | ||
| description = " |
There was a problem hiding this comment.
Better use '' (two single quotes for multiline string).
But this is only one line so you can reduce two "...." in one line.
| description = '' | ||
| Change to this system user after daemonization. | ||
| ''; | ||
| } |
| }; | ||
|
|
||
| journal = mkOption { | ||
| default = "/tmp/lastfm-journal.mpdscribble"; |
There was a problem hiding this comment.
Is this journal persistent data? If yes, I would put into /var (for example /var/lib/mpdscribble). If no, I would put it in /run/ (if the amount of data stored is small).
There was a problem hiding this comment.
The amount is variable. mpdscribble stores scrobble data (about 5 lines of text, in human readable format) for each track played when it is not able to reach out to the last.fm servers... normally during network outage or when without internet connection. I'd opt for /run then.
| let | ||
| cfg = config.services.mpdscribble; | ||
|
|
||
| mkSection = cfg: optname: secname: |
There was a problem hiding this comment.
Options for each backend could be also reduced using a generic function, like you did for the configuration file.
There was a problem hiding this comment.
Not sure what you mean...
| wantedBy = "multi-user.target"; | ||
| path = [ pkgs.mpdscribble ]; | ||
| User = "mpdscribble"; | ||
| Type = "oneshot"; |
There was a problem hiding this comment.
This is a daemon, right? If yes, I would remove Type here (simple is the default value).
|
|
||
| config = mkIf cfg.enable { | ||
| systemd.services.mpdscribble = { | ||
| after = [ "network.target" ]; |
There was a problem hiding this comment.
How about:
after = [ "mpd.service", "network.target" ];
| after = [ "network.target" ]; | ||
| description = "mpdscribble mpd scrobble client"; | ||
| wantedBy = "multi-user.target"; | ||
| path = [ pkgs.mpdscribble ]; |
There was a problem hiding this comment.
This line should be not necessary because you start mpdscribble with the full path.
|
|
||
| config = mkIf cfg.enable { | ||
| systemd.services.mpdscribble = { | ||
| after = [ "mpd.service", "network.target" ]; |
There was a problem hiding this comment.
List syntax in Nix does not need comma here.
matthiasbeyer
force-pushed
the
mpdscribble-service
branch
from
61d2b49
to
bcb0462
Compare
|
Updated ... this was stalled way to long. I'm sorry. |
| ## http://mpd.wikia.com/wiki/Client:mpdscribble | ||
|
|
||
| # HTTP proxy URL. | ||
| ${if isNull cfg.proxy then "" else "proxy = ${cfg.proxy}"} |
There was a problem hiding this comment.
Is it deliberate that you do not use networking.proxy?
There was a problem hiding this comment.
What do you mean?
Also: This PR is stalled too long, but I can't get it finished... am starting mpdscribble after each boot by hand (which is ugly and sometimes I forget it)... so if someone wants to take this and finish it: feel free!
There was a problem hiding this comment.
I mean, you configure a proxy, but there is a system-wide proxy configuration variable.
I am not using mpd at all, I just hoped to merge a random sample of long-standing PRs.
|
Any reason why you closed this @matthiasbeyer ? |
|
Because I don't care anymore. Feel free to take the patches and continue the effort. |
Motivation for this change
I guess I did this before but failed. So here's a second try.
Would love to get some review on this before testing it.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)