badaix / snapos

Snapcast OS
GNU General Public License v3.0
98 stars 31 forks source link

openwrt: convert to procd #7

Closed xabolcs closed 5 months ago

xabolcs commented 4 years ago

procd is the new process management daemon and supports a new style init script.

xabolcs commented 4 years ago

I tested only the snapclient.init part.

xabolcs commented 4 years ago

After commit badaix/snapcast@a2311dfbfd1c06451ece242b7ff65a09f660f9e8 I'm wondering how to take the logging defaults into account from /etc/default/snap{client,server} in the init files.

procd have to run the programs in foreground. Therefore I have to set the logging sink to system (and also wipe out all -d and --daemon argument from SNAPSERVER_OPTS).

Should I override it like the --daemon? Should I create a patches directory and patch debian/snap{client,server}.default to add the log sink param to SNAPSERVER_OPTS?

xabolcs commented 4 years ago

... I'm wondering how to take the logging defaults into account ...

I decided to override them with --logging.sink system and --logsink system.

Filtering out -d and --daemon is still TODO.

badaix commented 4 years ago

Is it recommended by OpenWRT to migrate to procd? And yes: using system as log sink is the proper way, what about -d? Is it set somewhere?

xabolcs commented 4 years ago

As this is the new init format, I think it's recommended. Just see the related commits in openwrt/packages repository! I have to mention that I was unable to find an explicit recommendation on OpenWrt Wiki or in CONTRIBUTING.md.

I want the -d filter stuff because the old behavior automatically adds it to the options:

SNAPCLIENT_OPTS="-d $SNAPCLIENT_OPTS"

In turn, it forces to daemonize the process.

That's why I want to force running the process in the foreground ...

xabolcs commented 4 years ago

With the upcoming v0.21 version is it feasible to refresh this PR and upstream Snapcast to OpenWrt Packages?

db4rne commented 3 years ago

Hi, the fork of 21.0x already happened, maybe we can still upstream this into OpenWRT packages? is there any argument against it? I'd love to help.

db4rne commented 3 years ago

I have started to include this into openwrt, if any of you would like to test and/or change something please contact me. you can find my branch for the snapcast package here: https://github.com/db4rne/packages/tree/snapcast

it's just the makefile from this repo (changed for version 0.24), and the procd-scripts from this PR.

xabolcs commented 3 years ago

@db4rne , it would be nice to include it in OpenWrt Packages repo.

I think, this PR is ready to use.

xabolcs commented 1 year ago

master updated to v0.27.0, but develop was left behind. Should I re-target the PR?

xabolcs commented 1 year ago

Retargeted the PR to master.

davidandreoletti commented 6 months ago

@xabolcs With https://github.com/badaix/snapos/pull/29, snapcast can be built again for OpenWRT 23.05 and current SNAPSHOT.

Have you tried this PR with OpenWRT 23.05 yet ?

I am keen to help test this PR on OpenWRT snapshot.

davidandreoletti commented 6 months ago

Is it recommended by OpenWRT to migrate to procd?

@badaix Yes, procd is THE startup/process management facility for OpenWRT.

xabolcs commented 6 months ago

Have you tried this PR with OpenWRT 23.05 yet ?

I tried this PR long time ago, because of the debian files. But I use the init files directly with OpenWrt SNAPSHOT and 23.05.

davidandreoletti commented 6 months ago

But I use the init files

@xabolcs Do you mean the init files using procd (from this PR) were copied to your OpenWRT 23.05/snapshot device and are working as intended ?

xabolcs commented 6 months ago

Yup!

Just save those init files as /etc/init.d/snap{client,server} and they will be ready to use.

davidandreoletti commented 6 months ago

@xabolcs Thanks!

@badaix: @xabolcs confirmed the procd enabled init files in this PR are working on the latest OpenWRT stable and snapshot release.

Benefits for snapcast: better integration with OpenWRT's system service

Would you mind merging this PR please ?

davidandreoletti commented 6 months ago

@badaix This PR will take 4 min of your time max to review its usefullness/merge.