Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spotifyd: enable dbus features, fix playback issues #77853

Closed
wants to merge 2 commits into from

Conversation

jb55
Copy link
Contributor

@jb55 jb55 commented Jan 17, 2020

];

nativeBuildInputs = [ pkgconfig ];

buildInputs = [ openssl ]
buildInputs = [ openssl dbus_libs ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use dbus. dbus_libs is a deprecated alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha

Comment on lines 8 to 13
features = ["dbus_mpris"
"dbus_keyring"
]
++ stdenv.lib.optional withALSA "alsa_backend"
++ stdenv.lib.optional withPulseAudio "pulseaudio_backend"
++ stdenv.lib.optional withPortAudio "portaudio_backend";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please use the two space indentation as recommended by coding style:

Suggested change
features = ["dbus_mpris"
"dbus_keyring"
]
++ stdenv.lib.optional withALSA "alsa_backend"
++ stdenv.lib.optional withPulseAudio "pulseaudio_backend"
++ stdenv.lib.optional withPortAudio "portaudio_backend";
features = [
"dbus_mpris"
"dbus_keyring"
]
++ stdenv.lib.optional withALSA "alsa_backend"
++ stdenv.lib.optional withPulseAudio "pulseaudio_backend"
++ stdenv.lib.optional withPortAudio "portaudio_backend";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -23,17 +23,14 @@ in
};

config = mkIf cfg.enable {
systemd.services.spotifyd = {
systemd.user.services.spotifyd = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think using the service file from the package would be better:

diff --git a/nixos/modules/services/audio/spotifyd.nix b/nixos/modules/services/audio/spotifyd.nix
index 61058a2f419..102259f8b0d 100644
--- a/nixos/modules/services/audio/spotifyd.nix
+++ b/nixos/modules/services/audio/spotifyd.nix
@@ -24,15 +24,14 @@ in
 
   config = mkIf cfg.enable {
     systemd.user.services.spotifyd = {
-      wantedBy = [ "multi-user.target" ];
-      after = [ "network-online.target" "sound.target" ];
-      description = "spotifyd, a Spotify playing daemon";
       serviceConfig = {
+        # Overriding config file location
+        # Keep in sync with ${spotifyd}/lib/systemd/user/spotifyd.service
         ExecStart = "${pkgs.spotifyd}/bin/spotifyd --no-daemon --config-path ${spotifydConf}";
-        Restart = "always";
-        RestartSec = 12;
       };
     };
+
+    systemd.packages = [ pkgs.spotifyd ];
   };
 
   meta.maintainers = [ maintainers.anderslundstedt ];
diff --git a/pkgs/applications/audio/spotifyd/default.nix b/pkgs/applications/audio/spotifyd/default.nix
index ad84d5298d4..266a27ccf13 100644
--- a/pkgs/applications/audio/spotifyd/default.nix
+++ b/pkgs/applications/audio/spotifyd/default.nix
@@ -39,6 +39,14 @@ in rustPlatform.buildRustPackage rec {
 
   doCheck = false;
 
+  postPatch = ''
+    substituteInPlace contrib/spotifyd.service --replace "/usr/bin" "$out/bin"
+  '';
+
+  postInstall = ''
+    install -Dm644 contrib/spotifyd.service $out/lib/systemd/user/spotifyd.service
+  '';
+
   meta = with stdenv.lib; {
     description = "An open source Spotify client running as a UNIX daemon";
     homepage = "https://github.com/Spotifyd/spotifyd";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we want wantedBy, after and description ? why do we care to keep in sync with the package's version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if we use the file from the package, there already are those fields. The assumption is that the upstream file contains the correct options in ExecStart so we should sync that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I didn't know systemd somehow merges config options from multiple service definitions? maybe I'm confused on how that works.

@anderslundstedt
Copy link
Contributor

I understand that switching to user service is required to get some of the functionality of spotifyd to work. But for us that does not use that functionality, I guess this change is slightly bad for security? Would it make sense to have an option to not run spotifyd as a user service? (Otherwise, I could of course write my own service.)

Also, I remember that the last attempt of adding the dbus features caused problems for spotifyd when X11 is not present (for example, on my headless Raspberry Pi where I run spotifyd as a Spotify Connect client). Currently compiling, will report how it works out this time.

@jb55
Copy link
Contributor Author

jb55 commented Jan 17, 2020

change is slightly bad for security

why would it be bad for security? I don't mind if we could get the other way to work, but I copied this from @colemickens's patch as that was the only way I could get it working.

I remember that the last attempt of adding the dbus features caused problems for spotifyd when X11 is not present

we could make it optional, but what's the point of spotifyd if there's no way of controlling it without the Spotify app? I guess if you're controlling it from your phone?

@NilsIrl
Copy link
Member

NilsIrl commented Jan 17, 2020

Has the problem got to do with pulseaudio or connecting to spotify? Because from what I remember it had to do with pulseaudio.

But at the same time @anderslundstedt said it worked for him.

@anderslundstedt
Copy link
Contributor

anderslundstedt commented Jan 18, 2020

change is slightly bad for security

why would it be bad for security? I don't mind if we could get the other way to work, but I copied this from @colemickens's patch as that was the only way I could get it working.

I just thought that running spotifyd as a user service would be less secure than running it under a separate user. But I do not understand systemd very well so perhaps I am misunderstanding what "user service" entails.

I remember that the last attempt of adding the dbus features caused problems for spotifyd when X11 is not present

we could make it optional, but what's the point of spotifyd if there's no way of controlling it without the Spotify app? I guess if you're controlling it from your phone?

Yes, exactly—that is my use case. And indeed the dbus features still causes trouble:

Caught panic with message: Failed to initialize DBus connection: D-Bus error: Unable to autolaunch a dbus-daemon without a $DISPLAY for X11 (org.freedesktop.DBus.Error.NotSupported)

@NilsIrl
Copy link
Member

NilsIrl commented Jan 18, 2020

@anderslundstedt

When you are spotifyd, is there sound coming out of your raspberry pi (or speaker that is connected to it)?

Because I don't think the problem has anything to do with controlling spotifyd.

@anderslundstedt
Copy link
Contributor

I do not really understand. With the dbus features enabled spotifyd crashes immediately with that error message. Without them spotifyd runs and works well (and I control it from my phone using the official iOS app). In particular, I get sound output (but I am not sure that was what you were wondering).

@NilsIrl
Copy link
Member

NilsIrl commented Jan 18, 2020

I do not really understand. With the dbus features enabled spotifyd crashes immediately with that error message. Without them spotifyd runs and works well (and I control it from my phone using the official iOS app). In particular, I get sound output (but I am not sure that was what you were wondering).

That is exactly what I am trying to do except it doesn't work. (Though I'm trying to control it using the web app)

@anderslundstedt
Copy link
Contributor

I just tried using the web app. I can use it to control spotifyd but only because it has my credentials cached since earlier. If I start up a fresh spotifyd instance then I cannot control it using the web app (unless I connect to it in some other way first). I guess this is because the web app cannot scan the local network for Spotify Connect clients.

@NilsIrl
Copy link
Member

NilsIrl commented Jan 18, 2020

I guess this is because the web app cannot scan the local network for Spotify Connect clients.

That should be normal. It's normal that it needs the credentials to work.

My problem (#71362) is that I can control it but then nothing plays.

@jb55
Copy link
Contributor Author

jb55 commented Jan 19, 2020

@NilsIrl I had the same issue, can you confirm that this fixes it for you?

I think this should be fine to merge once dbus is made optional + with the suggested changes by @jtojnar ?

@anderslundstedt
Copy link
Contributor

I still wonder about the potential security implications of switching to a user service.

I just thought that running spotifyd as a user service would be less secure than running it under a separate user. But I do not understand systemd very well so perhaps I am misunderstanding what "user service" entails.

Before merging I would like to know what security implications switching to a user service has. If security is worse off in the user service case then perhaps we should only use that for those cases that need it (dbus? anything else?).

@jb55
Copy link
Contributor Author

jb55 commented Jan 19, 2020

@anderslundstedt I think the issue is that playback with pulseaudio doesn't work otherwise, are you using the alsa backend? security wise it wouldn't be any different than running any other piece of software as your user.

@NilsIrl
Copy link
Member

NilsIrl commented Jan 19, 2020

IIRC both backends didn't work.

@jb55
Copy link
Contributor Author

jb55 commented Jan 20, 2020

I was able to get ALSA working before this when setting a specific hw device, but not pulseaudio

@anderslundstedt
Copy link
Contributor

@anderslundstedt I think the issue is that playback with pulseaudio doesn't work otherwise, are you using the alsa backend?

Yes.

security wise it wouldn't be any different than running any other piece of software as your user.

Which then is less secure than running that software under a separate user, right? I do not know whether it would be worth it to add options to the module for running under a separate user, for those who do not need pulseaudio playback. If not, then I will just use a private service so no problem.

@jb55
Copy link
Contributor Author

jb55 commented Jan 20, 2020

Which then is less secure than running that software under a separate user, right? I do not know whether it would be worth it to add options to the module for running under a separate user

do you run every piece of software you use as a separate user? I guess your concern is the author of spotifyd slipping in some code that reads secrets from your user dir? I don't understand the threat model here, its an open source project and I would hope that would be detected.

I'm just trying to get this software to work with pulseaudio out of the box, and would be open to suggestions if someone can get working with a DynamicUser

@anderslundstedt
Copy link
Contributor

do you run every piece of software you use as a separate user?

When possible and convenient, yes. I understand that switching to a user service is currently the best solution we have for getting spotifyd to work with pulseaudio. I also understand that the extra logic required for options for different service setups might not be worth it. In that case, I am fine with that and would not object to merging this (once dbus has been made optional).

@NilsIrl
Copy link
Member

NilsIrl commented Jan 20, 2020

I'm just trying to get this software to work with pulseaudio out of the box, and would be open to suggestions if someone can get working with a DynamicUser

So is it now able to run inside pulseaudio (with this PR)?

@jb55
Copy link
Contributor Author

jb55 commented Jan 20, 2020

@NilsIrl yes with the PR I can run spotifyd with the pulseaudio backend, that was the point of this, plus to fix the dbus controller.

@NilsIrl
Copy link
Member

NilsIrl commented Jan 30, 2020

@anderslundstedt I think the issue is that playback with pulseaudio doesn't work otherwise, are you using the alsa backend?

Yes.

I've tried again and I can't get either backends to work (using nixos-unstable, not this PR). Is there anything special you do to get the alsa backend to work?

@anderslundstedt
Copy link
Contributor

My spotifyd config is simply services.spotifyd.enable = true; My audio config is the following (using an external USB sound card):

  sound.enable = true;
  sound.extraConfig = ''
    defaults.pcm.card 1
    defaults.ctl.card 1
  '';

I recall that when I set this up I could not get audio output via the headphone jack working on my Raspberry Pi. Have you checked that your problem is specific to spotifyd? (For ALSA check with for example speaker-test.)

I've tried again and I can't get either backends to work (using nixos-unstable, not this PR).

Why not try with this PR?

@anderslundstedt
Copy link
Contributor

I think this should be fine to merge once dbus is made optional + with the suggested changes by @jtojnar ?

@jb55 Any progress on this?

@jb55
Copy link
Contributor Author

jb55 commented Jan 31, 2020

I didn't get to it yet, I mostly made the PR to get it working on my system, would be happy if people improved on the patch though :]

@gunnihinn
Copy link

Some of this may have been addressed in commit 11be0cd. I can't find what PR that was a part of or what issues it may have fixed, so I don't know more.

@j-hui
Copy link
Contributor

j-hui commented Feb 19, 2021

Is there any progress on this? In particular, looking at running spotifyd as a user service.

This fixes playback/permission issues

Signed-off-by: William Casarin <jb55@jb55.com>
This allows you to control it directly with applications such as
playerctl.

Signed-off-by: William Casarin <jb55@jb55.com>
@jb55
Copy link
Contributor Author

jb55 commented Feb 19, 2021

@j-hui I've pushed my rebased patches that I've been using since I originally opened this PR.

@stale
Copy link

stale bot commented Aug 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 21, 2021
@Artturin Artturin marked this pull request as draft March 6, 2023 17:55
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 6, 2023
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@jb55 jb55 closed this Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spotifyd service doesn't work
8 participants