[bug#70845] [PATCH v2] services: Add fancontrol-service-type.

[Juliana Sims]

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