New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
formula_cellar_checks: check for cpuid
instruction when needed
#11644
Conversation
Review period will end on 2021-07-06 at 00:00:00 UTC. |
987a3bc
to
9ddf5c3
Compare
This implements the second audit discussed in Homebrew#11608.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. I saw some discussion of an allowlist for this in #11608, is that still the plan?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlocab Fine with me 👍🏻
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thanks, @carlocab!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
objdump
on the system -- our macOS runners have it, and I believe our Linux runners do as well.
🆒.
Nice work @carlocab! |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with 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.