nixos/mpdscribble: init #97553

Sohalt · 2020-09-09T15:45:14Z

Motivation for this change

This adds a NixOS service for mpdscribble.

There have been prior attempts at this: #23660 #9940 #6806

Things done

I have the module running and working using a configuration of

services.mpdscribble = {
    enable = true;
    endpoints = {
      "libre.fm" = {
        username = "redacted";
        password = "redacted";
      };
      "last.fm" = {
        username = "redacted";
        password = "redacted";
      };
    };
  }

The password is stored either as MD5 or cleartext directly in the config file in the nix store. The description specifically states this.
It is not ideal, but ok for now IMHO. I've opened an issue upstream to support a password_file option (MusicPlayerDaemon/mpdscribble#24).

doronbehar · 2020-09-09T16:16:59Z

I like this. @matthiasbeyer do you have anything to comment?

I haven't deeply reviewed this PR yet. But, I'd like to verify that the passwords are handled right. As part of that, I'm waiting for a review at #95599 which I see as a dependency for this module's keeping passwords safe.

doronbehar · 2020-09-09T16:31:37Z

For reference, I'm using mpdas which is a similar daemon, in my personal config for a while now, with both #95599 in my nixpkgs fork and this:

    systemd.services.mpdas = {
      description = "Music Player Daemon Scrobbler (mpdas)";
      bindsTo = ["mpd.service"];
      wantedBy = [ "multi-user.target" ];
      unitConfig = {
        RequiresMountsFor = "/var/lib/mpd";
      };
      # workaround for https://github.com/hrkfdn/mpdas/issues/58 
      environment.HOME = "/";
      serviceConfig = {
        User = "mpd";
        # That file's format is configured here:
        # https://github.com/hrkfdn/mpdas/tree/master#configuration
        ExecStart = "${pkgs.mpdas}/bin/mpdas -c /etc/mpdasrc";
        # A little bit more complicated then mpd's prestart script because it
        # has to parse the mpd conf file format
        ExecStartPre = pkgs.writeScript "mpdas-start-pre" ''
          #!${pkgs.runtimeShell}
          set -euo pipefail
          password=$(${pkgs.gawk}/bin/awk -F'[@"]' '$1 == "password " {print $2}' /var/lib/secrets/mpd.conf)
          echo "mpdpassword = $password" > /etc/mpdasrc
          cat /var/lib/secrets/mpdas.conf >> /etc/mpdasrc
        '';
        Type = "simple";
      };
    };
    environment.etc."mpdasrc" = {
      mode = "0660";
      group = "transmission";
      user = "mpd";
      text = ''
      '';
    };

Where /var/lib/secrets/mpdas.conf is where I keep my librefm password and username.

doronbehar · 2020-09-09T19:48:44Z

nixos/modules/services/audio/mpdscribble.nix

+
+    # The host running MPD, possibly protected by a password
+    # ([PASSWORD@]HOSTNAME).
+    host = ${cfg.host}


@Sohalt I was trying to suggest, that mpd's password shouldn't be plain visible here, and more over, at the store. You should use a environment.etc. file, with the proper permissions for the mpdscribble user. But, this is a dumb request when we do write to the store the mpd passwords - without #95599 . Could you give me a hand there with a review / test please?

matthiasbeyer · 2020-09-10T06:28:30Z

I like this. @matthiasbeyer do you have anything to comment?

No, I don't use lastfm/etc anymore.

doronbehar · 2020-12-23T14:51:00Z

@Sohalt I've only taken a rough look in the meantime and it looks fantastically promising :). I've failed in the attempt of doing it myself! Thanks so much for working on this. There's an ofborg eval error though: https://gist.github.com/GrahamcOfBorg/b3fb7048bc6528d554c2421e8353c812

Sohalt · 2020-12-23T18:42:30Z

@doronbehar Thanks! I've somehow missed that. Now everything's green :)

doronbehar

Other then that LGTM but I haven't tested.

nixos/modules/services/audio/mpdscribble.nix

aanderse

I think this module needs a few changes before it is ready. If you need any clarification, or even help, please don't hesitate to ping me and I'll do what I can. Thanks for working on this, it seems like a desired feature for our OS 👍

nixos/modules/services/audio/mpdscribble.nix

Sohalt · 2020-12-23T22:04:30Z

@aanderse @doronbehar thanks a lot for your review! I think I've addressed everything.

nixos/modules/services/audio/mpdscribble.nix

doronbehar

Built my system with this and without the typo and it seems fairly functional.

doronbehar · 2020-12-25T17:17:58Z

nixos/modules/services/audio/mpdscribble.nix

+        Endpoints to scrobble to.
+        If the endpoint is one of "${
+          concatStringsSep "\", \"" (attrNames endpointUrls)
+        }" the url is set automatically.


Saying the url is set automatically gives a slight feeling it's possible to set a certain url, for an arbitrary endpoint, but there isn't.. So maybe remove that last part of the sentence? Or alternatively provide a default value for a url option?

Not sure I understand you correctly. You can set an arbitrary url for an arbitrary endpoint, but when you use one of the predefined endpoints, there is already a default with the "official" endpoint for those services (defined here).

doronbehar · 2020-12-25T17:23:46Z

nixos/modules/services/audio/mpdscribble.nix

+    # "syslog" makes mpdscribble use the local syslog daemon.  On most
+    # systems, log messages will appear in /var/log/daemon.log then.
+    # "-" means log to stderr (the current terminal).
+    log = -


Why did we remove this option? I personally like the idea of having the list of scrobbles also in a separate file. If I remember correctly that this is what this log holds.

It does not only hold lists of scrobbles but also other logging output. There is the journal, which is used to store a list of scrobbles when there is no connection to the scrobble server.
Since the service runs as DynamicUser it does not make too much sense to be able to specify a separate location, because the service won't have access (I could add ReadWritePaths, but I don't think it's really necessary. - will make mpdscribble log to the systemd journal.

nixos/modules/services/audio/mpdscribble.nix

Sohalt · 2020-12-26T17:23:52Z

I have a commit that automatically sets the passwordFile to the correct value if it is configured through the services.mpd.credentials option that I've added here #107508
I think it would make sense to merge that first, then I can add that commit and this PR will already include that useful feature.

aanderse

LGTM 👍

doronbehar · 2021-01-04T06:38:24Z

I will test this with my laptop, along with the PR for mpd on my next system update. Then I will merge. Thanks again for working on this.

…

On Jan 3 2021, at 4:28 pm, Aaron Andersen ***@***.***> wrote: @aanderse approved this pull request. LGTM 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ***@***.***/0?redirect=https%3A%2F%2Fgithub.com%2FNixOS%2Fnixpkgs%2Fpull%2F97553%23pullrequestreview-560700885&recipient=cmVwbHkrQUNUNUlNNDVWR1ZNWlZYVlhaSE9TNDU1N1cyWUpFVkJOSEhDVENSN0E0QHJlcGx5LmdpdGh1Yi5jb20%3D), or unsubscribe ***@***.***/1?redirect=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACT5IM4LLUMQZX3SRDXKIIDSYB5IJANCNFSM4RCS4UFA&recipient=cmVwbHkrQUNUNUlNNDVWR1ZNWlZYVlhaSE9TNDU1N1cyWUpFVkJOSEhDVENSN0E0QHJlcGx5LmdpdGh1Yi5jb20%3D).

doronbehar · 2021-01-09T18:55:23Z

From some reason I wasn't able to merge this on top of #107508 so I didn't test this. So let's unmark it as a draft and rebase?

doronbehar

Works! Thanks so much for working on this. Enjoy the music 🎷.

Sohalt · 2021-01-10T14:56:57Z

Thank you for the review and merging!

ofborg bot added 6.topic: nixos 8.has: module (update) labels Sep 9, 2020

Sohalt force-pushed the nixos/mpdscribble branch from 63e9b84 to f9d9dd0 Compare September 9, 2020 18:15

doronbehar reviewed Sep 9, 2020

View reviewed changes

Sohalt force-pushed the nixos/mpdscribble branch 2 times, most recently from 3be68aa to cde9d33 Compare December 20, 2020 20:31

Sohalt requested a review from doronbehar December 22, 2020 21:09

Sohalt force-pushed the nixos/mpdscribble branch 2 times, most recently from 30ede12 to 843c77d Compare December 23, 2020 18:25

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Dec 23, 2020

doronbehar requested changes Dec 23, 2020

View reviewed changes

nixos/modules/services/audio/mpdscribble.nix Outdated Show resolved Hide resolved

nixos/modules/services/audio/mpdscribble.nix Outdated Show resolved Hide resolved

nixos/modules/services/audio/mpdscribble.nix Show resolved Hide resolved

aanderse requested changes Dec 23, 2020

View reviewed changes

aanderse mentioned this pull request Dec 23, 2020

lib.generators.toINI cannot generate files without "sections" #107491

Closed

Sohalt force-pushed the nixos/mpdscribble branch from 843c77d to 743161f Compare December 23, 2020 21:58

Sohalt force-pushed the nixos/mpdscribble branch from 743161f to cac09d4 Compare December 24, 2020 00:31

Sohalt mentioned this pull request Dec 24, 2020

nixos/mpd: support passwords in separate files #107508

Merged

doronbehar reviewed Dec 24, 2020

View reviewed changes

nixos/modules/services/audio/mpdscribble.nix Outdated Show resolved Hide resolved

nixos/modules/services/audio/mpdscribble.nix Show resolved Hide resolved

Sohalt force-pushed the nixos/mpdscribble branch from cac09d4 to 412f39b Compare December 24, 2020 10:43

doronbehar requested changes Dec 25, 2020

View reviewed changes

Sohalt force-pushed the nixos/mpdscribble branch 2 times, most recently from efd7afc to bcb6e3a Compare December 25, 2020 20:52

Sohalt marked this pull request as draft December 26, 2020 17:22

aanderse approved these changes Jan 3, 2021

View reviewed changes

Sohalt force-pushed the nixos/mpdscribble branch from bcb6e3a to f6a08f3 Compare January 9, 2021 22:35

nixos/mpdscribble: init

59bd4c1

Sohalt force-pushed the nixos/mpdscribble branch from f6a08f3 to 59bd4c1 Compare January 9, 2021 22:38

Sohalt marked this pull request as ready for review January 9, 2021 22:38

doronbehar approved these changes Jan 10, 2021

View reviewed changes

doronbehar merged commit 054b7c4 into NixOS:master Jan 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/mpdscribble: init #97553

nixos/mpdscribble: init #97553

Sohalt commented Sep 9, 2020

doronbehar commented Sep 9, 2020

doronbehar commented Sep 9, 2020

doronbehar Sep 9, 2020

matthiasbeyer commented Sep 10, 2020

doronbehar commented Dec 23, 2020

Sohalt commented Dec 23, 2020

doronbehar left a comment

aanderse left a comment

Sohalt commented Dec 23, 2020

doronbehar left a comment

doronbehar Dec 25, 2020

Sohalt Dec 25, 2020

doronbehar Dec 25, 2020

Sohalt Dec 25, 2020

Sohalt commented Dec 26, 2020

aanderse left a comment

doronbehar commented Jan 4, 2021 via email

doronbehar commented Jan 9, 2021

doronbehar left a comment

Sohalt commented Jan 10, 2021

nixos/mpdscribble: init #97553

nixos/mpdscribble: init #97553

Conversation

Sohalt commented Sep 9, 2020

Motivation for this change

Things done

doronbehar commented Sep 9, 2020

doronbehar commented Sep 9, 2020

doronbehar Sep 9, 2020

Choose a reason for hiding this comment

matthiasbeyer commented Sep 10, 2020

doronbehar commented Dec 23, 2020

Sohalt commented Dec 23, 2020

doronbehar left a comment

Choose a reason for hiding this comment

aanderse left a comment

Choose a reason for hiding this comment

Sohalt commented Dec 23, 2020

doronbehar left a comment

Choose a reason for hiding this comment

doronbehar Dec 25, 2020

Choose a reason for hiding this comment

Sohalt Dec 25, 2020

Choose a reason for hiding this comment

doronbehar Dec 25, 2020

Choose a reason for hiding this comment

Sohalt Dec 25, 2020

Choose a reason for hiding this comment

Sohalt commented Dec 26, 2020

aanderse left a comment

Choose a reason for hiding this comment

doronbehar commented Jan 4, 2021 via email

doronbehar commented Jan 9, 2021

doronbehar left a comment

Choose a reason for hiding this comment

Sohalt commented Jan 10, 2021