| Message ID | 20240509154032.5047-1-neox@gnu.org |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#70845] services: Add fancontrol-service-type | expand |
Hi, Thanks a lot for the patch. For the record I'm not a Guix maintainer / committer, and that I didn't look at everything but I spotted something that could be improved: the user has no way to override the lm-sensors package being used. Most services provide a way to do that kind of override, so it would make sense to add that at some point. Denis.
Le jeudi 09 mai 2024 à 19:39 +0200, Denis 'GNUtoo' Carikli a écrit : > Hi, > > Thanks a lot for the patch. > > For the record I'm not a Guix maintainer / committer, and that I > didn't look at everything but I spotted something that could be > improved: the user has no way to override the lm-sensors package being > used. > > Most services provide a way to do that kind of override, so it would > make sense to add that at some point. > > Denis. Hi and thanks for you feedback. Not every service I saw allows such override, but it seems indeed a good idea. However, it would complicate a bit this patch as it would require to create a proper configuration type for the service and I also did not dig into that (did not have much time). Someone else might be able to do it. Happy hacking!
Le jeudi 09 mai 2024 à 19:39 +0200, Denis 'GNUtoo' Carikli a écrit : > Hi, > > Thanks a lot for the patch. > > For the record I'm not a Guix maintainer / committer, and that I > didn't look at everything but I spotted something that could be > improved: the user has no way to override the lm-sensors package being > used. > > Most services provide a way to do that kind of override, so it would > make sense to add that at some point. > > Denis. Hi and thanks for you feedback. Not every service I saw allows such override, but it seems indeed a good idea. However, it would complicate a bit this patch as it would require to create a proper configuration type for the service and I also did not dig into that (did not have much time). Someone else might be able to do it. Happy hacking!
Hi again, While looking at the patches I interacted with on issues.guix.gnu.org, I noticed a small issue: the commit message summary lacks a dot at the end. Denis.
Hi Adrien, Thanks for this patch! It looks pretty good, though I do have a few small bits of feedback. First and foremost, this service needs documentation. Could you add that as well? Speaking of documentation, the 'documentation' field of your Shepherd service has an extraneous bit of whitespace. Is it absolutely vital to use root for this service? Could you instead create a user and usergroup with only the privileges required to run fancontrol? You may need to do something with udev so that works. I'm not sure exactly what privileges are required, but avoiding root seems like a good idea. That's the only real critique I have here. Consider the rest of this email fun ideas rather than review per se :) We had an out-of-band exchange about this patch that I'll summarize here for the record. I echoed the sentiments of the reviewer who suggested exposing the fancontrol package so that users could change it. Your response was that the configuration is generated by pwmconfig and therefore it wouldn't make sense to provide a Scheme interface to configuration. I don't know this package or how it works, but would it be possible for this service to generate that config automatically when it's first started? If the config is customizable about generation, you could then expose various settings through the Guix service interface for users to modify and rewrite the file for them. That would make using define-configuration worthwhile for more than simply the ability to change the package. All that aside, you should be able to expose the package setting to users without using define-configuration. Best, Juli
Le mercredi 15 mai 2024 à 18:16 -0400, Juliana Sims a écrit : > Hi Adrien, Hi Juliana and thanks a lot for your review. I'm now working on a v3 ;) > First and foremost, this service needs documentation. Could you add > that as well? Speaking of documentation, the 'documentation' field of > your Shepherd service has an extraneous bit of whitespace. Ok, seems sensible to do, I'm working on it and fixing the whitespace issue. > > Is it absolutely vital to use root for this service? Could you instead > create a user and usergroup with only the privileges required to run > fancontrol? You may need to do something with udev so that works. I'm > not sure exactly what privileges are required, but avoiding root seems > like a good idea. I don't actually know if this is vital, but it was the only way to make it work properly. I did not think about udev though, so I'll try to test things. I agree that avoiding root is a good idea. > I don't know this package or how it works, but would it be possible for > this service to generate that config automatically when it's first > started? If the config is customizable about generation, you could then > expose various settings through the Guix service interface for users to > modify and rewrite the file for them. That would make using > define-configuration worthwhile for more than simply the ability to > change the package. Okay, perhaps more details are needed then. The fancontrol software is made to control the speed of the fans (CPU heatsinks for example) based on a configuration that is proper to the fans and that depends on many physical parameters that might include, sometimes, the environment where the computer is used. This configuration has to be generated with the pmwconfig software, which will ask questions to the users (for example "Should we check I2C bus?" or "Did you hear a fan stopping?"). Since the configuration process is both critical and interactive, it appears difficult to me to do that automatically. One more thing is that users should recreate a configuration when the use environment has changed. > > All that aside, you should be able to expose the package setting to > users without using define-configuration. Ok, thanks for the information. I'll work on that. Have a nice day everyone and happy hacking!
diff --git a/gnu/services/pm.scm b/gnu/services/pm.scm index 3daf484cc1..1b305e76a6 100644 --- a/gnu/services/pm.scm +++ b/gnu/services/pm.scm @@ -31,7 +31,9 @@ (define-module (gnu services pm) tlp-configuration thermald-configuration - thermald-service-type)) + thermald-service-type + + fancontrol-service-type)) (define (uglify-field-name field-name) (let ((str (symbol->string field-name))) @@ -466,3 +468,31 @@ (define thermald-service-type (default-value (thermald-configuration)) (description "Run thermald, a CPU frequency scaling service that helps prevent overheating."))) + +;;; +;;; fancontrol +;;; +;;; This service implements fan control in conjunction with the tools in the +;;; lm-sensors package (pwmconfig/fancontrol). + +(define (fancontrol-shepherd-service config) + (shepherd-service + (documentation "Run the fancontrol daemon (fancontrol-daemon)." ) + (provision '(fancontrol)) + (requirement '(udev user-processes)) + (start #~(make-forkexec-constructor + (list #$(file-append lm-sensors "/sbin/fancontrol") + #$config) + #:user "root" #:group "root" + #:log-file "/var/log/fancontrol.log")) + (stop #~(make-kill-destructor)))) + +(define fancontrol-service-type + (service-type + (name 'fancontrol) + (description + "Run fancontrol as a daemon.") + (extensions + (list (service-extension shepherd-root-service-type + (compose list fancontrol-shepherd-service)))))) +
Hi! I've created a fancontrol service for my own use on a KGPE-D16 workstation, and wanted to share it with GNU Guix. The configuration has to be generated upstream with pwmconfig (lm-sensors package) and you just have to tell the service where it is for it to work. Change-Id: I120e54cbf849eebd088be2a4d0a0113ffcdfcd84 Signed-off-by: Adrien 'neox' Bourmault <neox@gnu.org> --- gnu/services/pm.scm | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)