search

christf / snapcastc

C implementation of snapcast focussing on audio quality and ease of maintenance.
GNU General Public License v3.0
39 stars 6 forks source link

Add systemd units #45

Closed jreusch closed 4 years ago

jreusch commented 4 years ago

This PR takes the unit files written for #11 and shipps them with the debian package.

To be honest i did not manage to start snapcastc with those, it failed for me. But the general structure should do it, adapting the content is smaller change.

I also wanted to split up the package into two debian packages but skipped that, since i do not know (despite having found two manuals) how to do this with CMake.

Additional short questions: why are the binaries called snapcast-[server|client] and noch snapcastc like the rest of the package. do you intent to keep them protocol compatible ( i did not thought so?) to be a drop in replacement?

Also added a small helper script to actually build the debian package inside docker.

jreusch commented 4 years ago

I do not know why there is a segmenation fault for the test call. It also fails for me on the current master.

markusj commented 4 years ago

I do not know why there is a segmenation fault for the test call. It also fails for me on the current master.

Should be fixed in #42

jreusch commented 4 years ago

cool! i wonder how this ever got into master in the first place since the tests fail :) hope your's get merged soon, then i can rebase onto master.

jreusch commented 4 years ago

btw: the bump in the debhelper/compat version needs to be to switch from init to systemd default. since this version is in buster i assume this as safe to do.

christf commented 4 years ago

What was the reason for not being able to start snapcastc with the unit file?

snapcastc and snapcast are not compatible. Indeed the binaries should be renamed for clarity.

jreusch commented 4 years ago

hey hey, i'm currently at a conference, when i'm @home i can give you more information on this.

on additional question: pr #42 was merged into the mixer branch, also it seems there is more activity going on there. is this the one which should be the target instead of master?

jreusch commented 4 years ago

Hey hey, this is my journey to try it making it run:

-> seems to start successfully, but Type=notify -> no sd_notify signal sent -> startup fails

-> adding systemd headers does not help grepping codebase for sd_notify -> nothing? -> changed Type to simple(default)

server does still not start: snapcast-server[22334]: [54B blob data]

that's when i gave up for now, any advice?

christf commented 4 years ago

snapcastc should be started with sd_simple.

Edit: I just ed this out: Type must not be notify. If it is, systemd will cyclicly re-start the service.

christf commented 4 years ago

hm. After upgrading the OS of my raspberrypi I can confirm that your unit file indeed is working for the client and for the server.

Did you set up /etc/default/snapcastc?

This is what I have in there:

cat /etc/default/snapcastc SNAPCAST_SERVEROPTS="-s "pipe:///tmp/snapfifo?name=default&codec=opus&sampleformat=48000:16:2&buffer_ms=120" -b 30000 -p 1704" SNAPCAST_CLIENTOPTS="-s snapclient -m snapclient -H lappi.lan -p 1704"

The parameter of -H must resolve to the host running the snapcastc-server

jreusch commented 4 years ago

hey hey thanks for the clarification, i'll do the adaptions this weekend i hope.

jreusch commented 4 years ago

now it should be good for integration. still would opt for renaming the binaries, but most likely this is another PR :)

christf commented 4 years ago

@jreusch There are two more questions I would like to ask: Do you want to rebase this pre-merge?

What would be a good place to state that the config lives in /etc/default/snapcastc and state that the hostname should be adjusted for the client in case the server is not running locally? The readme could work but maybe it could be a notification during install as well..?

jreusch commented 4 years ago

rebase done.

for the other questions: hmm i do not know the packaging best practices in that area. tbh when i look at such things i have a peak into how it is started and there you would see it. maybe a section in the README.md and if this is too prominent (since this information is only interesting for debian deb deployments) maybe in the package readme itself? notifications during install i do not know, who reads them anyway? ;) maybe not a notification but a forced setting via debconf if you want to have it that prominent?

btw: also a small discepancy

$ snapcast-client --help
snapclient [-l | -H <servername> [...]

the $0 name seems to be wrong?

jreusch commented 4 years ago

sorry, messed up with git fetch, now the rebase should look good. :)

christf commented 4 years ago

Thank you your contribution