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

Include a systemd.service file to run as a user #800

Merged
merged 2 commits into from
Jun 18, 2021
Merged

Include a systemd.service file to run as a user #800

merged 2 commits into from
Jun 18, 2021

Conversation

WhyNotHugo
Copy link
Contributor

This new systemd.service file allows running and controlling the service as
an unpriviledged user (generally, as part of a user session).

Also document this and the pre-existing service file and how they are used.

Copy link
Contributor

@JasonLG1979 JasonLG1979 left a comment

Choose a reason for hiding this comment

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

Another possibility could be Description=Librespot Spotify Connect client But that's a little long and awkward to use as a noun. I think I prefer just Description=Librespot as I think that:

  1. It's short and simple.
  2. It's not like librespot is going to be some unknown service. If it's enabled the user would have explicitly installed and enabled it so I feel like the "too generic" clause does not apply.

@@ -1,5 +1,6 @@
[Unit]
Description=Librespot
Description=An open source Spotify client
Copy link
Contributor

Choose a reason for hiding this comment

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

Confusingly Description should be the name of the service not a description of what it is. So a noun is best as that's how it will be used.

The systemd unit spec reads:

A human readable name for the unit. This is used by systemd (and other UIs) as the label for the unit, so this string should identify the unit rather than describe it, despite the name. "Apache2 Web Server" is a good example. Bad examples are "high-performance light-weight HTTP server" (too generic) or "Apache2" (too specific and meaningless for people who do not know Apache). systemd will use this string as a noun in status messages ("Starting description...", "Started description.", "Reached target description.", "Failed to start description."), so it should be capitalized, and should not be a full sentence or a phrase with a continuous verb. Bad examples include "exiting the container" or "updating the database once per day.".

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'd never looked at the docs for that particular one. On my local system around 3-4 or 401 unit files actually do what the documentation suggests.

Go ahead and run ag Description /usr/lib/systemd/system, to see the values of all local units.

I do think that putting the name in the description is weird, since then the name and the description contain the same information, which makes the Description= field valueless and results in no real description.

If you think that's a blocker I'll change it, but I'd rather follow the de-facto standard, rather than the written standard that is used by <1%.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not for me to say. I was just pointing out that it doesn't follow the spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Go ahead and run ag Description /usr/lib/systemd/system, to see the values of all local units.

I did and some follow the spec and some don't. So really it's a tossup. The ones that don't follow the spec prob didn't read the docs either,lol!!!

Copy link
Member

Choose a reason for hiding this comment

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

I think it's not too difficult to do both: follow current practice and the spec, if you were to change this into: "Librespot open source Spotify client" or the likes. This way, the systemd status messages will make sense and it's clear to the user what librespot actually is.

@@ -0,0 +1,11 @@
[Unit]
Description=An open source Spotify client
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. It should be Description=Librespot

@roderickvd
Copy link
Member

Thanks for your PR. For my understanding, what's the difference between this and Raspotify?

@JasonLG1979
Copy link
Contributor

Thanks for your PR. For my understanding, what's the difference between this and Raspotify?

Raspotify only supports Raspberry Pi's. I think it still has a valid use case for other non-Pi devices like your average x86 desktop or home audio server.

@WhyNotHugo
Copy link
Contributor Author

@roderickvd Raspotify seems to be a tool for Raspberry PIs, while librespot targets pretty much any Linux environment.

@roderickvd
Copy link
Member

Yeah the name kind of reveals it, but looking at the service definition that's included there, I'm not sure why it wouldn't work on other devices than a RPi?

@WhyNotHugo
Copy link
Contributor Author

WhyNotHugo commented Jun 17, 2021

I don't think raspotify is packaged in most distributions, whereas librespot is.

Also, the service included there is a system service (e.g.: to be managed and controlled by root only). That's very much like the one librespot currently ships.

The one I'm adding here is so that it can be run and managed as an unprivileged user.

@JasonLG1979
Copy link
Contributor

@roderickvd it basically comes down to this: On a desktop system with Pulseaudio installed a system level librespot service won't work out of the box. A user level librespot service will.

@roderickvd
Copy link
Member

Sorry if I come across as pedantic but I just want to understand. You say the Raspotify service definition runs as root, but I don't think that it does? I'm seeing User=raspotify and Group=raspotify or the likes.

I don't think that librespot is packaged in any distribution, other than audiophile OS's that already handle it with their own logic, is it?

Anyway, seeing as there already is a service definition for running as root, I think it's a nice addition to also provide a sample for running as a user.

Who thought a little service definition would gather so may comments this quickly? 😄 keep it going! If the reviewers can let me know once they're OK with this, we can get it in.

@roderickvd roderickvd self-assigned this Jun 17, 2021
@JasonLG1979
Copy link
Contributor

JasonLG1979 commented Jun 17, 2021

Sorry if I come across as pedantic but I just want to understand. You say the Raspotify service definition runs as root, but I don't think that it does? I'm seeing User=raspotify and Group=raspotify or the likes.

That is the user and group that the librespot binary is ran as. Basically for security reasons a user with no premissions other than network and audio and who owns a folder that audio can be cached to. That's not who can admin the service.

I don't think that librespot is packaged in any distribution, other than audiophile OS's that already handle it with their own logic, is it?

That's kinda my main argument about the service. How it's presented. I don't mind them as starting points for diy types but I don't like the idea of presenting it as something a downstream packager should or must use. They should create their own unit file that suits their needs and not copypasta.

@WhyNotHugo
Copy link
Contributor Author

That's kinda my main argument about the service. How it's presented. I don't mind them as starting points for diy types but I don't like the idea of presenting it as something a downstream packager should or must use. They should create their own unit file that suits their needs and not copypasta.

They don't copy-paste, they re-use upstream files programtically. As a random example, here's the ArchLinux package for Deluge. It installs the systemd.service files provided by upstream by coping from the tarball at build time.

Several hundred other packages do the same. The whole point of things like systemd is that upstreams can provide service files and avoid distributions have to re-do all the same work over and over (and also, makes applications behave a bit more consistently).

Any packager copy-pasting is just doing it wrong; the same way the shouldn't be copy-pasting a Makefile.

@JasonLG1979
Copy link
Contributor

They don't copy-paste, they re-use upstream files programtically

There is no difference between copy and paste by a person and copy and paste programtically. The result is the same.

As a random example, here's the ArchLinux package for Deluge.

That is a literal example of a programtic copypasta.

Several hundred other packages do the same. The whole point of things like systemd is that upstreams can provide service files and avoid distributions have to re-do all the same work over and over (and also, makes applications behave a bit more consistently).

And the intent of those programs are to create systemd service daemons. That is not the intent of librespot. There are upstream projects that already do that like Raspotify and Spotifyd. librespot is lower level than that.

@WhyNotHugo
Copy link
Contributor Author

I'm missing your point, sorry. Can you clarify what you're trying to say?

@JasonLG1979
Copy link
Contributor

I'm missing your point, sorry. Can you clarify what you're trying to say?

Making librespot useable as a systemd service is not the concern of librespot. It's primary purpose is to be a set of libs that other upstream projects can use to create whatever they like including a systemd service daemon if they wish. The fact that it can be compiled as a minimal Spotify Connect client is a side effect of being a complete set of libs.

@JasonLG1979
Copy link
Contributor

JasonLG1979 commented Jun 17, 2021

I have no problem with the services as examples. My problem is presenting them as canonical and supported in any way and upstream using them as-is without modification or at least looking at them and considering their use case.

@WhyNotHugo
Copy link
Contributor Author

Librespot has been usable as a systemd service since 2017. This PR only adds a file to run as a user service. I’m sorry you feel that it’s such a bad idea, but I’d hope it would be useful to others.

Regarding downstream packagers: they’re always are free to patch at will though. Regardless of whether we decide to encourage it or recommend against it, packagers will ultimately do as they please.

@JasonLG1979
Copy link
Contributor

JasonLG1979 commented Jun 17, 2021

Librespot has been usable as a systemd service since 2017.

The direction is for that to change as librespot becomes more modular. There may come a time when actual playback is not a part of librespot.

This PR only adds a file to run as a user service. I’m sorry you feel that it’s such a bad idea, but I’d hope it would be useful to others.

I don't think it's a bad idea, like I said, as an unsupported example.

Regarding downstream packagers: they’re always are free to patch at will though. Regardless of whether we decide to encourage it or recommend against it, packagers will ultimately do as they please.

I realize that but if you say "you should use this..." You are implying that it's the canonical and supported way it should be done. That is not the case. The burden of implementation and support as far as a systemd unit file and librespot usage in general is concerned rests solely on the down stream packagers.

@JasonLG1979
Copy link
Contributor

JasonLG1979 commented Jun 17, 2021

@WhyNotHugo I fixed the wiki.

  1. I fixed the sidebar you didn't edit it to include your new page and you didn't remove the dead link to the page you deleted (Running-in-the-background)

  2. I removed the parts saying upstream should use the unit files.

  3. I noted that the units can be used as an example to write your own unit file or used as-is.

  4. I noted that another reason for the use of a user service is to avoid PulseAudio incompatability.

Here's a comparison

That about covers my complaints.

Edit: Super trivial: I also changed "This'll" to "This will" "This'll" is not a word in formal written English. It's only used in informal writing and speech.

@@ -1,5 +1,6 @@
[Unit]
Description=Librespot
Description=An open source Spotify client
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not too difficult to do both: follow current practice and the spec, if you were to change this into: "Librespot open source Spotify client" or the likes. This way, the systemd status messages will make sense and it's clear to the user what librespot actually is.

@roderickvd
Copy link
Member

I addressed the final review point I think, if you could look into that then we can merge afterwards.

Hugo Osvaldo Barrera added 2 commits June 18, 2021 10:40
This new `systemd.service` file allows running and controlling the service as
an unpriviledged user (generally, as part of a user session).

Also document this and the pre-existing service file and how they are used.
@WhyNotHugo
Copy link
Contributor Author

I've reworded the description to Librespot (an open source Spotify client). I believe this follows the spec and the de-facto standard, but also keeps a clearer sentence structure (that also looks correct in systemd logs and alike).

Lemme know if that looks good.

@roderickvd
Copy link
Member

Looks good to me, I'll let the CI run and merge on success.

@WhyNotHugo
Copy link
Contributor Author

I don't think CI failures are related 😅

@roderickvd
Copy link
Member

No they're network errors. I'll go ahead now.

@roderickvd roderickvd merged commit 51a6972 into librespot-org:dev Jun 18, 2021
@WhyNotHugo
Copy link
Contributor Author

WhyNotHugo commented Jun 18, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants