search

mcgillij / amdfan

Updated AMD Fan control utility forked from amdgpu-fan and updated.
https://mcgillij.dev/pages/amdfan.html
GNU General Public License v2.0
33 stars 7 forks source link

[feat] backgrounding the daemon #36

Closed mazunki closed 1 month ago

mazunki commented 1 month ago

I realize the openrc service I had created a template for was using the old format, and while fixing it I decided I wanted to clean up the backgrounding wrapper we use in openrc. Before making the PR, I'm stuck with a decision.

I've currently implemented the support for daemonizing our program (as I mildly hinted at with the --background comment in-code previously), and have done a few other changes (e.g improve some documentation, change some default values).

Currently: What used to be called the daemon is now called the manager, so we'd now be running FanController.start_manager(...) instead of .start_daemon(...), and we have two entry subcommands for it: manage (for running the controller in a shell manually) and daemon (intended for launching the service from the system). Their only real difference is the default values they pass forward.

My main question here is whether we should make the daemon default to forking into the background or to stay in the foreground (e.g should we add --background or --no-background as an option?).

My opinion is that daemon, by its word, suggests that the program would be daemonized (aka ran in the background), so I'd vote for default background, explicit foreground.

It's worth noting that if a user runs amdfan daemon and it defaults to --background, some users may get a little bit confused on where the application went, and won't know how to kill it. If we choose to background the application, this is a change in the old behaviour of the daemon subcommand (is that acceptable?).

We could also hide the daemon option from the main help page by default, and just keep it documented under the longer help of manage to avoid any oopsies, but on the flip side this may be considered obfuscating the details.

What do you think?

mcgillij commented 1 month ago

It would be kinda strange if it did background itself, however I don't really have a problem with it as long as it's documented to have that behavior. So whatever you think would be easiest to manage from there it's probably ok.