Skip to content

Commit

Permalink
Redirect sensors error output to /dev/null
Browse files Browse the repository at this point in the history
lm-sensors will output warnings and other mostly harmless messages to
stdout often, even when it is possible to get good data. While often it
is possible to configure sensors to ignore these faulty sensors, this is
the most often requested change to Liquidprompt.

I was wrong, and I am sorry that it took me so long to see it. In short,
so many users asking for the same change means that I should have
accepted it a while ago.

Long version is that I didn't want to hide errors that users would need
to diagnose a real problem with their lm-sensors configuration. But
there are a number of problems with that stance:

1. Very rarely are these error messages actually about a
   misconfiguration. Most often they are worthless warnings because a
   piece of hardware was turned off, or with the new Linux
   CVE-2020-12912, that a normal user can't see power information. Which
   isn't what we even want; we want temperature information, not power,
   but there is no way to tell sensors to just give us that.
2. Of the currently 63 unique external command calls in Liquidprompt,
   only 12 of them (including this sensors call) do not void stderr. I
   did not realize this until I started working on every part of
   Liquidprompt, but now I see that "hiding problems" is not a problem,
   it is just how the prompt works. It is not the prompt's job to show
   you errors, just to show any data it can collect.
3. This should be fixed in lm-sensors, but they still have not fixed it.
4. I wanted the "perfect" fix for this, but there is not one (at least
   not yet). I have been avoiding the "ok" fix in the hopes that a
   better one would come along, but that obviously has not happened.

So if we assume lm-sensors is just broken, we can recommend that users
do not use it (and build a better data source). But we still need to
make this data source not completely broken.

It would be unrealistic to expect there to be no issues caused by this
change. I expect that some users will get no temperatures (or at least
not all sensors) when first installing lm-sensors, so we should probably
add some docs on this feature describing how to configure lm-sensors.

Again, sorry everyone. I promise to try to be more flexible and
understanding in the future.

A full list of PRs suggesting this change:
* #319, #381, #394, #534, #597, #633

Fixes #445, #598

Co-authored-by: Fabien MARTY <[email protected]>
Co-authored-by: Alessio Garzi <[email protected]>
Co-authored-by: Mathieu MD <[email protected]>
Co-authored-by: Romano Giannetti <[email protected]>
Co-authored-by: Paulo Cazarotto <[email protected]>
Co-authored-by: Jürgen Weigert <[email protected]>
Co-authored-by: Sven-Hendrik Haase <[email protected]>
  • Loading branch information
8 people committed Dec 10, 2020
1 parent 6f1dbcf commit 4a52696
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion liquidprompt
Original file line number Diff line number Diff line change
Expand Up @@ -2147,7 +2147,7 @@ __lp_temp_sensors() {
# Only the integer part is retained
local -i i
local IFS=$' \t\n'
for i in $(LC_ALL=C \sensors -u |
for i in $(LC_ALL=C \sensors -u 2>/dev/null |
sed -n 's/^ temp[0-9][0-9]*_input: \([0-9]*\)\..*$/\1/p'); do
(( $i > ${lp_temperature:-0} )) && lp_temperature=$i
done
Expand Down

0 comments on commit 4a52696

Please sign in to comment.