Page MenuHomePhabricator

[test] On Mac, don't try to use result of sysctl command if calling it failed.
ClosedPublic

Authored by DavidSpickett on Jan 6 2020, 8:29 AM.

Details

Summary

If sysctl is not found at all, let the usual exception propogate
so that the user can fix their env. If it fails because of the
permissions required to read the property then print a warning
and continue.

Diff Detail

Event Timeline

DavidSpickett created this revision.Jan 6 2020, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 8:29 AM

I found this running lit tests on a Mac that doesn't have sysctl available. Not very familiar with it so I'm not sure why that is the case for our particular machine.

So this change is me assuming the intention was to just print and carry on if "sysctl" fails.

cmatthews accepted this revision.Jan 10 2020, 2:33 PM

LGTM. Could you maybe update the message something like warning: calling sysctl failed, defaulting to no fma3

This revision is now accepted and ready to land.Jan 10 2020, 2:33 PM
beanz added a comment.Jan 11 2020, 4:39 PM

I found this running lit tests on a Mac that doesn't have sysctl available. Not very familiar with it so I'm not sure why that is the case for our particular machine.

So this change is me assuming the intention was to just print and carry on if "sysctl" fails.

I would be excessively concerned about why a Mac doesn't have sysctl available. I'm not actually sure we should continue running tests if it is missing. sysctl is a fundamental unix tool that ships with macOS, for it to be missing your OS install is likely broken. If the command is failing (but not missing) that may be masking a different issue. Sandboxing or user permissions can limit which keys sysctl can interrogate.

DavidSpickett updated this revision to Diff 237643.EditedJan 13 2020, 5:43 AM
DavidSpickett edited the summary of this revision. (Show Details)

Turns out I didn't have /usr/sbin on my PATH (due to some machine hopping).

In light of that I think it makes more sense to allow "file not found" to be raised normally, then if sysctl returns non zero print the warning that was suggested.

ping, @cmatthews does the updated diff look good to you?

This revision was automatically updated to reflect the committed changes.