Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2987023002:
swarming: report CPU temperature on OSX as state. (Closed)
Descriptionswarming: report CPU temperature on OSX as state.
Manually encode the call prototype instead of using the pyobjc bridge. It's both
faster and the Object-C python bridge unconditionally writes to stderr, which is
annoying. So in the end, the code calls Object-C functions via ctypes.
Everything is fine.
Fix OSX state values to all be unicode.
R=vadimsh@chromium.org
BUG=749840
Review-Url: https://codereview.chromium.org/2987023002
Committed: https://github.com/luci/luci-py/commit/13d61ef9aea5822094a129b0fa6150130f5b8245
Patch Set 1 #
Total comments: 11
Patch Set 2 : Generalize #
Total comments: 5
Patch Set 3 : whitespace #
Messages
Total messages: 17 (6 generated)
https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/api/os_utilities.py (right): https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/os_utilities.py:1034: state[u'temp'] = temp on linux this is a dict. Is there a way to make this dict (with preferably same kind of keys) on OSX too? https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/api/platforms/osx.py (right): https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/platforms/osx.py:128: if iokit.IOServiceOpen(dev, libkern.mach_task_self(), 0, ctypes.byref(conn)): I assume all such connections are automatically released when the process dies? https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/platforms/osx.py:154: # Call with SMC_CMD_READ_BYTES. this looks horrifying :(
kbr@chromium.org changed reviewers: + kbr@chromium.org
lgtm overall. Thanks for cleaning up my prototype. https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/api/platforms/osx.py (right): https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/platforms/osx.py:63: class SMCKeyData_vers_t(ctypes.Structure): How about a pointer to https://github.com/lavoiesl/osx-cpu-temp/ in a comment with thanks? We wouldn't have reverse engineered this ourselves... https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/platforms/osx.py:168: def SMC_get_temperature(conn, key): Comment that this is in Celsius.
https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/api/platforms/osx.py (right): https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/platforms/osx.py:128: if iokit.IOServiceOpen(dev, libkern.mach_task_self(), 0, ctypes.byref(conn)): The code kbr@ linked to releases the device after this call. Perhaps we can do it too.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/api/os_utilities.py (right): https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/os_utilities.py:1034: state[u'temp'] = temp On 2017/07/28 18:15:56, Vadim Sh. wrote: > on linux this is a dict. Is there a way to make this dict (with preferably same > kind of keys) on OSX too? Generalized. https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/api/platforms/osx.py (right): https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/platforms/osx.py:63: class SMCKeyData_vers_t(ctypes.Structure): On 2017/07/28 18:22:01, Ken Russell (switch to Gerrit) wrote: > How about a pointer to https://github.com/lavoiesl/osx-cpu-temp/ in a comment > with thanks? We wouldn't have reverse engineered this ourselves... It is not the original source either. I found sources from 2006. https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/platforms/osx.py:128: if iokit.IOServiceOpen(dev, libkern.mach_task_self(), 0, ctypes.byref(conn)): On 2017/07/28 18:15:56, Vadim Sh. wrote: > I assume all such connections are automatically released when the process dies? Yes, I've did not implement code to close the handle since it's not necessary. https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/platforms/osx.py:154: # Call with SMC_CMD_READ_BYTES. On 2017/07/28 18:15:56, Vadim Sh. wrote: > this looks horrifying :( Na, I've done much worse recently. 💣 https://codereview.chromium.org/2987023002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/api/platforms/osx.py:168: def SMC_get_temperature(conn, key): On 2017/07/28 18:22:01, Ken Russell (switch to Gerrit) wrote: > Comment that this is in Celsius. Celsius or Death. Well it could have been Kelvin. :) I've rewrote the function to be more general.
https://codereview.chromium.org/2987023002/diff/40001/appengine/swarming/swar... File appengine/swarming/swarming_bot/api/platforms/osx.py (right): https://codereview.chromium.org/2987023002/diff/40001/appengine/swarming/swar... appengine/swarming/swarming_bot/api/platforms/osx.py:120: _sensor_names = { do we really need all that stuff?... https://codereview.chromium.org/2987023002/diff/40001/appengine/swarming/swar... appengine/swarming/swarming_bot/api/platforms/osx.py:558: for key in _sensor_found_cache: nit: remove extra space after 'in'
https://codereview.chromium.org/2987023002/diff/40001/appengine/swarming/swar... File appengine/swarming/swarming_bot/api/platforms/osx.py (right): https://codereview.chromium.org/2987023002/diff/40001/appengine/swarming/swar... appengine/swarming/swarming_bot/api/platforms/osx.py:120: _sensor_names = { On 2017/07/28 20:54:55, Vadim Sh. wrote: > do we really need all that stuff?... No, I want to make it live then trim based on what seems useful. Sadly on staging there's only one VM, no hardware so I can't see the scope beside on my work laptop. https://codereview.chromium.org/2987023002/diff/40001/appengine/swarming/swar... appengine/swarming/swarming_bot/api/platforms/osx.py:558: for key in _sensor_found_cache: On 2017/07/28 20:54:55, Vadim Sh. wrote: > nit: remove extra space after 'in' Done.
https://codereview.chromium.org/2987023002/diff/40001/appengine/swarming/swar... File appengine/swarming/swarming_bot/api/platforms/osx.py (right): https://codereview.chromium.org/2987023002/diff/40001/appengine/swarming/swar... appengine/swarming/swarming_bot/api/platforms/osx.py:120: _sensor_names = { On 2017/07/28 20:58:46, M-A Ruel wrote: > On 2017/07/28 20:54:55, Vadim Sh. wrote: > > do we really need all that stuff?... > > No, I want to make it live then trim based on what seems useful. Sadly on > staging there's only one VM, no hardware so I can't see the scope beside on my > work laptop. Actually, VM *do* report temperatures. That's so awkward.
lgtm
The CQ bit was checked by maruel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2987023002/#ps60001 (title: "whitespace")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501275975774760,
"parent_rev": "5925b54b4ff3522c85710eda81d4305074663fef", "commit_rev":
"13d61ef9aea5822094a129b0fa6150130f5b8245"}
Message was sent while issue was closed.
Description was changed from ========== swarming: report CPU temperature on OSX as state. Manually encode the call prototype instead of using the pyobjc bridge. It's both faster and the Object-C python bridge unconditionally writes to stderr, which is annoying. So in the end, the code calls Object-C functions via ctypes. Everything is fine. Fix OSX state values to all be unicode. R=vadimsh@chromium.org BUG=749840 ========== to ========== swarming: report CPU temperature on OSX as state. Manually encode the call prototype instead of using the pyobjc bridge. It's both faster and the Object-C python bridge unconditionally writes to stderr, which is annoying. So in the end, the code calls Object-C functions via ctypes. Everything is fine. Fix OSX state values to all be unicode. R=vadimsh@chromium.org BUG=749840 Review-Url: https://codereview.chromium.org/2987023002 Committed: https://github.com/luci/luci-py/commit/13d61ef9aea5822094a129b0fa6150130f5b8245 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://github.com/luci/luci-py/commit/13d61ef9aea5822094a129b0fa6150130f5b8245 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
