As of upstream commit 6115 <http://lm-sensors.org/changeset/6115/lm-sensors/trunk>, sensors-detect no longer generates MODULE_0, MODULE_1, MODULE_2 variables. However, /etc/init.d/lm_sensors as supplied by sys-apps/lm_sensors-3.3.4 still checks for MODULE_0 in the case that modules are to be loaded. Since this variable does not exist anymore, attempting to start the lm_sensors service fails with a "MODULE_0 is not set in /etc/conf.d/lm_sensors, try running sensors-detect" error, even after running sensors-detect. Should this be changed to check for HWMON_MODULES instead? Reproducible: Always
I confirm the bug in lm_sensors-3.3.4-r1. I worked around it defining MODULE_0 with the same value as HWMON_MODULES (just one module here).
This should really be an easy fix, yet nearly one year later, this bug still hit me today. Any reason this isn't "fixed", yet?
Confirm on AMD64
Created attachment 399878 [details, diff] Proposed Patch First ever time writting shell script and submitting patch. I'm not sure if the script is portable or follows any coding standards. I tried to make it backwards compatable with MODULE_0 because MODULE_ way of doing things allows you to specify arguments to pass to the module. I didn't change any copywrite date or anything, I don't know if I needed to do that. It worked on my system.
Created attachment 400244 [details, diff] initd patch initd patch for version 3.3.4 and above. Must work with version 3.3.3 which is also in the tree.
Sorry, did not see prev patch comment. I think MODULE_X variables support not needed, because it exist only for compatibility reasons and was removed from >=3.3.4. No sense if 3.3.3 version will be removed from tree in close future.
Both proposed patches do not handle BUS_MODULES and therefore only solve half of the problem. In gentoo we patch lm_sensors to modify the output with -sensors-detect-gentoo.patch. I suggest to revert http://lm-sensors.org/changeset/6115/lm-sensors/trunk as part of that patch and just keep the old init scripts.
BUS_MODULES is optional because all module dependency must be loaded automatically. Maybe im wrong. Anyway, must be simple to add support for it too.
Created attachment 400766 [details, diff] initd patch
One loop with concatenated module lists is enough, but even then i do not think that is very elegant. Thinking about it the whole module loading should be done by suggesting people to modify /etc/conf.d/modules. That should be the sensors-detect patch gentoo should apply, and the lm_sensors init script would get a "after modules".
Just tried it ... /etc/init./lm_sensors: ... > LOADMODULES=no ... /etc/conf.d/modules: ... lm_sensors_modules="coretemp" modules="$modules $lm_sensors_modules" added the "after modules" dep to /etc/init./lm_sensors and now run "/etc/init.d/modules zap; /etc/init.d/modules start", as a way to load you new additional modules without unloading the old ones.
Anton's patch works great on my system. It seems like a good idea to get rid of MODULES_X. Addition of ${BUS_MODULES} makes it much more like upstream's lm_sensors.init
Comment on attachment 399878 [details, diff] Proposed Patch >--- orig.lm_sensors-3-init.d 2008-03-17 02:59:28.000000000 -0500 >+++ lm_sensors-3-init.d 2015-03-27 11:02:28.003256446 -0500 >@@ -10,8 +10,12 @@ > fi > > if [ "${LOADMODULES}" = "yes" -a -f /proc/modules ]; then >- if [ -z "${MODULE_0}" ]; then >- eerror "MODULE_0 is not set in /etc/conf.d/lm_sensors, try running sensors-detect" >+ if [ -z "${MODULE_0}" ] && [ -z "${HWMON_MODULES}" ]; then >+ eerror "HWMON_MODULES or MODULE_0 is not set in /etc/conf.d/lm_sensors, try running sensors-detect" >+ return 1 >+ fi >+ if ! [ -z "${MODULE_0}" ] && ! [ -z "${HWMON_MODULES}" ]; then >+ eerror "HWMON_MODULES or MODULE_0 can't be both set in /etc/conf.d/lm_sensors, try running sensors-detect" > return 1 > fi > fi >@@ -46,18 +50,28 @@ > eend 0 > fi > >- i=0 >- while true; do >- module=`eval echo '$'MODULE_${i}` >- module_args=`eval echo '$'MODULE_${i}_ARGS` >- if [ -z "${module}" ]; then >- break >- fi >- ebegin " Loading ${module}" >- modprobe ${module} ${module_args} >/dev/null 2>&1 >- eend $? >- i=$(($i+1)) >- done >+ if ! [ -z "${MODULE_0}" ]; then >+ i=0 >+ while true; do >+ module=`eval echo '$'MODULE_${i}` >+ module_args=`eval echo '$'MODULE_${i}_ARGS` >+ if [ -z "${module}" ]; then >+ break >+ fi >+ ebegin " Loading ${module}" >+ modprobe ${module} ${module_args} >/dev/null 2>&1 >+ eend $? >+ i=$(($i+1)) >+ done >+ fi >+ >+ if ! [ -z "${HWMON_MODULES}" ]; then >+ for module in ${HWMON_MODULES}; do >+ ebegin " Loading ${module}" >+ modprobe ${module} >/dev/null 2>&1 >+ eend $? >+ done >+ fi > fi > > if [ "${INITSENSORS}" = "yes" ]; then >@@ -78,23 +92,39 @@ > if [ "${LOADMODULES}" = "yes" -a -f /proc/modules ]; then > einfo "Unloading lm_sensors modules..." > >- # find the highest possible MODULE_ number >- i=0 >- while true; do >- module=`eval echo '$'MODULE_${i}` >- if [ -z "${module}" ] ; then >- break >- fi >- i=$(($i+1)) >- done >+ if ! [ -z "${MODULE_0}" ]; then >+ # find the highest possible MODULE_ number >+ i=0 >+ while true; do >+ module=`eval echo '$'MODULE_${i}` >+ if [ -z "${module}" ] ; then >+ break >+ fi >+ i=$(($i+1)) >+ done > >- while [ ${i} -gt 0 ]; do >- i=$(($i-1)) >- module=`eval echo '$'MODULE_${i}` >- ebegin " Unloading ${module}" >- rmmod ${module} >/dev/null 2>&1 >- eend $? >- done >+ while [ ${i} -gt 0 ]; do >+ i=$(($i-1)) >+ module=`eval echo '$'MODULE_${i}` >+ ebegin " Unloading ${module}" >+ rmmod ${module} >/dev/null 2>&1 >+ eend $? >+ done >+ fi >+ >+ if ! [ -z "${HWMON_MODULES}" ]; then >+ # unload modules in reverse order >+ REV_HW_MODULES="" >+ for module in ${HWMON_MODULES}; do >+ REV_HWMON_MODULES=${module}" "${REV_HWMON_MODULES} >+ done >+ >+ for module in ${REV_HWMON_MODULES}; do >+ ebegin " Unloading ${module}" >+ rmmod ${module} >/dev/null 2>&1 >+ eend $? >+ done >+ fi > > if [ -e /proc/sys/dev/sensors ] ; then > ebegin " Unloading i2c-proc"
The gentoo way of loading modules is to use /etc/init.d/modules. And since we have to change the lm_sensors init script i propose to move to the existing infrastructure and not write "modprobe" and "rmmod" loops again and again. Moving to modules would result in a much shorter init script for lm_sensors, all the module related code would be turned into a "after modules" dep. The whole init script will boil down to an optional "sensors -s", see INIT_SENSORS. We would however loose the possibilty to unload modules right when we stop lm_sensors. I actually do not know what this feature is good for. init.d/modules does not care about unloading. Running a rmmod loop will just slow down your system shutdown. Does anyone have a convincing argument for the rmmod loop in the stop function? Moving to modules would mean we have to patch the sensors-detect tool of lm_sensors to suggest config changes that will work in /etc/conf.d/modules. Gentoo already patches exactly that perl script, no extra effort, just different. We probably need a enews to tell users about the change. That is an RFC so please let me knot what you think, i might send patches. But it would be nice to hear about major issues i overlooked.
Created attachment 409754 [details] files/lm_sensors-3-r1-init.d
Created attachment 409756 [details] files/lm_sensors-3-r1-conf.d
Created attachment 409758 [details, diff] files/lm_sensors-3.4.0-sensors-detect-gentoo.patch
Created attachment 409760 [details] lm_sensors-3.4.0-r1.ebuild
Guys. Please give this ebuild a try. It offloads the module-loading to openrc and will assist the user in changing /etc/conf.d/modules. That really simplifies the init script in a nice way. But the change is kind of drastic and will require a news item or some yellow/red color in the einfo. But seeing dispatch-conf messing with the init script and conf will give users an idea as well ... How are the modules handled in the systemd case? Seems i did not brake anything unless systemd reused the old openrc script.
CCing openrc.
Is there something holding up the change mentioned in comment 18? It's 6 months old.
I have no idea, maybe these days one needs to propose something like that as a github pull request to get attention. Christopher maybe you can try it and comment on how it worked for you.
If changing /etc/conf.d/modules seems too intrusive we could apply the symlink scheme used for /etc/init.d/net.* and eventually have /etc/(conf.d|init.d)/modules.lm_sensors and have /etc/init.d/lm_sensors depend on modules.lm_sensors. But that might require changing openrc to support the symlink scheme on the modules init-file. Something that might help simplify other init-scripts that deal with loading modules as well. But before going that way i would prefer getting a review an the current proposal.
Hello all. I agree, using /etc/conf.d/modules is a right way to load modules, instead of rewriting module-loading loops over and over. It may be more convenient to use multiple files like conf.d/modules.name or /etc/modules.d, but the current approach will also do it's job. I've tested this ebuild, everything seems to be working flawlessly. This so-hard-to-fix bug is almost three years old, let's do something about it.
I am currently working on this bug as part of the Gentoo Proxy-Maintainers project. Should be resolved within this week.
(In reply to Thomas D. from comment #25) > I am currently working on this bug as part of the Gentoo Proxy-Maintainers > project. Should be resolved within this week. Pinging.
This is in progress however I need this weekend to run more tests: I am not just fixing this bug, I am also working on the other open bugs of this package. Thank you for your patience.
sys-apps/lm_sensors-3.4.0-r1::x-portage works great! Good patch set, good work. THX
What is currently blocking progress on this bug? Can I offer any help? I don't want to duplicate work, and I also don't understand what is technically deficient with the solution from Henning Schild (comment 18).
Given the state of the package as broken-out-of-the-box, should we simply mask the package to actually figure out if someone is using it? Or, alternately, just remove /etc/init.d/lm_sensors ? As far as I can tell, the only significant role it plays is to load modules. Maybe it would be easier to patch sensors-detect to output a list of module names to add to /etc/conf.d/modules ...
(In reply to Christohper Harrington from comment #30) > Given the state of the package as broken-out-of-the-box, should we simply > mask the package to actually figure out if someone is using it? If you have an already working conf.d/lm_sensors it continues to work fine. > Or, alternately, just remove /etc/init.d/lm_sensors ? As far as I can tell, > the only significant role it plays is to load modules. Maybe it would be > easier to patch sensors-detect to output a list of module names to add to > /etc/conf.d/modules ... It also runs "sensors -s", which is crucial on some systems (without it my fans run full blast). whissi: you're a dev now, please go ahead and fix it, in such a way that doesn't break people with old conf.d/lm_sensors.
I am currently patching sensors-detect the following way: For systemd: It asks the user if it should generate or overwrite "/etc/modules-load.d/lm_sensors.conf". For OpenRC: It asks the user if it should add a block like > # BEGIN sys-apps/lm_sensors > # Generated by sensors-detect on Mon Aug 8 22:37:03 2016 > modules="$modules coretemp" > # END sys-apps/lm_sensors to "/etc/conf.d/modules" but _only_ if the user doesn't have set modules for a specific kernel version (i.e. 'modules_2="ipv6"'). In that case we will tell the user that he/she has to manually add the modules. This block can be overwritten if you re-run sensors-detect (if the marker wasn't touched). I am not sure what to do with existing configurations. I'd prefer that existing user will have to migrate their configurations (i.e. moving modules from "/etc/conf.d/lm_sensors" to "/etc/conf.d/modules"). This would allow us to keep the new runscript clean. Objections?
(In reply to Thomas Deutschmann from comment #32) > I am currently patching sensors-detect the following way: ... > I am not sure what to do with existing configurations. I'd prefer that > existing user will have to migrate their configurations (i.e. moving modules > from "/etc/conf.d/lm_sensors" to "/etc/conf.d/modules"). This would allow us > to keep the new runscript clean. > > Objections? Make sure you can convert module parameters as well. In my case, the w83627ehf module, needs fan_debounce=1. It used to need force_id as well, but that got fixed.
My changes are currently beeing reviewed. If you don't want to wait or want to test too feel free to grab the changes from my overlay at https://git.io/v6WX4. Summary of changes: - Based on latest unreleased changes (snapshot release from 2016-07-25; updated/added detection for Fintek F81768, Nuvoton NCT6793D, Microchip MCP9808 and Mark F71868A chips; various fixes for sensors-detect, see https://git.io/v6Wig for all changes) - sensors-detect now writes to "/etc/conf.d/modules" (OpenRC) or "/etc/modules-load.d/lm_sensors.conf" (systemd) and uses a config file protection mechanism like known from emerge (i.e. after making changes you have to call tools such as dispatch-conf, cfg-update or etc-update to merge the changes; new "--no-gentoo-config-protect" parameter will disable config file protection) Gentoo-specific changes can be reviewed from https://git.io/v6WXq - Ebuild is now multilib compatible (see bug 529684) - Runscripts no longer passes config file as argument. While this removes the possibility to use different configurations per default services it allows you to use "/etc/sensors.d" (see bug 490502 and bug 307249) - Runscripts improved in general. I.e. if you start fancontrol service without a valid config (so that fancontrol doesn't start and exit with an error) you will now know at least, that the service didn't start. Now you will also have reliable status functions to know service state. - lm_service service no longer loads modules. You have to use mechanism your init system provides to do that (that's the scope of this bug) Thank you for your patience.
Again, thank you for you patience. We had to delay lm_sensors due to some problems in OpenRC's new modules-load service but now everything is ready and in tree: > commit 9ba6c0d353dd2ad3936fe35095588648b472f188 > Author: Thomas Deutschmann > Date: Fri Sep 16 14:36:11 2016 +0200 > > sys-apps/lm_sensors: Version bump to snapshot release from 2016-07-25; Package now uses module-load service > > Changes: > ======== > - Based on latest unreleased changes (snapshot release from 2016-07-25; > updated/added detection for Fintek F81768, Nuvoton NCT6793D, Microchip > MCP9808 and Mark F71868A chips; various fixes for sensors-detect, see > https://git.io/v6Wig for all changes) > > - sensors-detect now writes to "/etc/modules-load.d/lm_sensors.conf" and > uses a config file protection mechanism like known from emerge (i.e. > after making changes you have to call tools such as dispatch-conf, > cfg-update or etc-update to merge the changes; > new "--no-gentoo-config-protect" parameter will disable config file > protection) > > - Due to previous change lm_service service no longer loads modules. You > have to use mechanism your init system provides to do that (i.e. make > sure your init system's modules-load service is enabled). > > - Ebuild is now multilib compatible (see bug 529684) > > - Runscripts no longer passes config file as argument. While this removes > the possibility to use different configurations per default services it > allows you to use "/etc/sensors.d" (see bug 490502 and bug 307249) > > - Runscripts improved in general. I.e. if you start fancontrol service > without a valid config (so that fancontrol doesn't start and exit with > an error) you will now know at least, that the service didn't start. > Now you will also have reliable status functions to know service state. > > Gentoo-Bug: https://bugs.gentoo.org/592916 > Gentoo-Bug: https://bugs.gentoo.org/529684 > Gentoo-Bug: https://bugs.gentoo.org/490502 > Gentoo-Bug: https://bugs.gentoo.org/307249 > > Package-Manager: portage-2.3.0
Many thanks for your efforts!
