formula_cellar_checks: check for cpuid instruction when needed#11644
formula_cellar_checks: check for cpuid instruction when needed#11644carlocab merged 3 commits intoHomebrew:masterfrom carlocab:cpuid-check
cpuid instruction when needed#11644Conversation
|
Review period will end on 2021-07-06 at 00:00:00 UTC. |
BrewTestBot
added
the
waiting for feedback
This implements the second audit discussed in #11608.
| return unless Hardware::CPU.intel? | ||
|
|
||
| # macOS `objdump` is a bit slow, so we prioritise llvm's `llvm-objdump` (~5.7x faster) | ||
| # or binutils' `objdump` (~1.8x faster) if they are installed. |
There was a problem hiding this comment.
Do these times include the installation time of llvm or binutils? Even if so: I'm not sure it's worth installing binutils just for this rare edge-case (but could be convinced otherwise) given the potential to mess with builds.
There was a problem hiding this comment.
They do not include installation time -- this is the time difference reported from running hyperfine on [llvm-]objdump -d libopenblas.dylib.
I'm fine with returning an audit failure if there is no objdump on the system -- our macOS runners have it, and I believe our Linux runners do as well.
There was a problem hiding this comment.
I think it's worth benchmarking the installation time as part of the "faster" times. Otherwise it's not a great comparison.
I'm fine with returning an audit failure if there is no
objdumpon the system -- our macOS runners have it, and I believe our Linux runners do as well.
🆒.
There was a problem hiding this comment.
I think it's worth benchmarking the installation time as part of the "faster" times.
I think benchmarking this would be important if we were installing something in this audit, but we no longer are. I agree that it would be a useful benchmark to have, though -- especially if we end up considering installing something for this audit down the road.
1. Never install `binutils`. Instead, report an audit failure. 2. Tighten instruction check with a stricter matching strategy.
|
I've skipped installing I've also tightened the match for |
Co-authored-by: Rylan Polster <rslpolster@gmail.com>
BrewTestBot
removed
the
waiting for feedback
|
Review period ended. |
| return unless Hardware::CPU.intel? | ||
|
|
||
| # macOS `objdump` is a bit slow, so we prioritise llvm's `llvm-objdump` (~5.7x faster) | ||
| # or binutils' `objdump` (~1.8x faster) if they are installed. |
There was a problem hiding this comment.
I think it's worth benchmarking the installation time as part of the "faster" times. Otherwise it's not a great comparison.
I'm fine with returning an audit failure if there is no
objdumpon the system -- our macOS runners have it, and I believe our Linux runners do as well.
🆒.
|
Nice work @carlocab! |
4 participants
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?This implements the second audit discussed in #11608.
I've only done this on macOS for now, since I make use ofKeg#mach_o_files, but I wanted to get feedback on the approach here before doing something more generic.