pop-os / system76-driver

System76 Driver for Pop!_OS
Other
112 stars 29 forks source link

Add sensor data #192

Closed ahoneybun closed 3 years ago

ahoneybun commented 3 years ago

This adds the following to the driver:

Tested on Pop 20.10.

ahoneybun commented 3 years ago

@jacobgkau I can agree with moving it to a recommends as I just wanted to be sure that it is installed with the driver or though updates to provide this feature.

jacobgkau commented 3 years ago

Well, that was a bit of a rabbit hole. I first confirmed that lm-sensors still gets installed automatically when installing system76-driver on Ubuntu (unless --no-install-recommends is passed). However, I started getting a hang if I tried to generate logs after manually removing lm-sensors.

While trying to catch that error, I found that util.py was using a SubProcess function defined in mockable.py. It looks like this was done to facilitate some automated testing (although the automated testing did not catch the hang in this case, and one of the tests started failing when I tried to catch subprocess errors.)

By catching FileNotFoundError in mockable.py, I was able to avoid a hang without affecting the testing in place, but I was unable to find a way to print any sort of message to the sensors file (or other affected files), only to sys.stdout; if sensors was missing, an empty file was created, which could be confusing. Looking around, there are already various calls to the standard subprocess function throughout the program, so I switched this file's two calls to subprocess. I would propose we move toward eliminating SubProcess for simplicity (not touching anything else right now, though.)

By using subprocess.run with shell=True, errors from the shell like "No such file or directory" are simply passed along into the output file as normal output. On my test machine, several other stderr lines that were previously hidden (only visible if running the app from the terminal) are now being included in the output files. For this particular use case, this seems like what we want (the point of this feature is to capture output so it doesn't have to be copied/pasted out of the terminal.)

One last thing that still wasn't quite right was that the stderr lines were being placed in the middle of stdout lines, instead of on their own lines like the terminal shows. I wasn't able to figure out why that was happening (just that it's not due to which shell's being used.) To get around that, I've got stderr printing at the end of the file, instead of being interspersed with stdout (this is still an improvement over the current behavior of not capturing stderr at all.)

Tomorrow, I'll plan to test this thoroughly across Ubuntu, Pop, and at least one machine with a non-standard shell to make sure I'm not breaking anything. Then we can see if anything I've done sets off alarms in the engineering review.