formula_cellar_checks: check keg for mismatched arches #11737
Conversation
There have been a few instances I've noticed that we've been silently installing binaries built for x86_64 on ARM. There's probably more that I haven't found yet, so it seems useful to check this with an audit.
|
Review period will end on 2021-07-20 at 00:00:00 UTC. |
BrewTestBot
added
the
waiting for feedback
The current offset results in ELF binaries returning an `#arch` of `:dunno`. Also, skip the `check_binary_arches` audit on the generic OS.
| ARCHITECTURE_X86_64 = 0x62 | ||
| ARCHITECTURE_X86_64 = 0x3E |
There was a problem hiding this comment.
This is needed because checking the arch of ELF files on Linux currently returns :dunno. See the CI failures from my first commit.
|
|
||
| keg = Keg.new(formula.prefix) | ||
| mismatches = keg.binary_executable_or_library_files.reject do |file| | ||
| file.arch == Hardware::CPU.arch |
There was a problem hiding this comment.
Hardware::CPU.arch returns :x86_64 when running under Rosetta, so this audit should behave as expected on ARM in /usr/local too.
There was a problem hiding this comment.
Though, one corner case worth considering, where I don't know what happens (and don't have the hardware to test...): a user who has an M1 machine with brew installed into /usr/local, and runs /usr/local/bin/brew install without running the process under Rosetta2. I suspect this audit will produce errors for them, but I don't know if brew will error out for that case earlier than that.
There was a problem hiding this comment.
I don't know if
brewwill error out for that case earlier than that.
it will 🎉
| # There is no `binary_executable_or_library_files` method for the generic OS | ||
| return if !OS.mac? && !OS.linux? |
There was a problem hiding this comment.
I'm just curious, why is this the case? I thought the default for this was to use the Linux version and, on macOS, the method was overridden. Is that not what happens with --generic?
There was a problem hiding this comment.
Yes. The default behaviour is to call the method elf_files, but that's Linux-only too.
|
|
||
| keg = Keg.new(formula.prefix) | ||
| mismatches = keg.binary_executable_or_library_files.reject do |file| | ||
| file.arch == Hardware::CPU.arch |
There was a problem hiding this comment.
I don't know if
brewwill error out for that case earlier than that.
it will 🎉
MikeMcQuaid
added
the
critical
|
Review period skipped due to |
BrewTestBot
removed
the
waiting for feedback
The LLVM formulae build universal binaries for compiler-rt. This is why these formulae use `ENV.permit_arch_flags`. Infer vendors its own clang, and so also has its own compiler-rt. `babel` and `contentful-cli` both install the npm package `fsevents`, which installs a universal native module `fsevents.node`. I suggest adding them to the allowlist for now so we have a list that keeps track of them. (It would be nice if JSON files allowed comments though.) I have changes in mind for `brew` that will allow us to easily extract the native slices from these binaries, but I'd like to see it needed by more formulae before we add it to `brew`. This list should help us keep track of the number of formulae that would need this. This commit unblocks Homebrew#81556 and Homebrew#81582. See Homebrew/brew#11737 for context.
The LLVM formulae build universal binaries for compiler-rt. This is why these formulae use `ENV.permit_arch_flags`. Infer vendors its own clang, and so also has its own compiler-rt. `babel` and `contentful-cli` both install the npm package `fsevents`, which installs a universal native module `fsevents.node`. I suggest adding them to the allowlist for now so we have a list that keeps track of them. (It would be nice if JSON files allowed comments though.) I have changes in mind for `brew` that will allow us to easily extract the native slices from these binaries, but I'd like to see it needed by more formulae before we add it to `brew`. This list should help us keep track of the number of formulae that would need this. This commit unblocks #81556 and #81582. See Homebrew/brew#11737 for context.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?There have been a few instances I've noticed that we've been silently
installing binaries built for x86_64 on ARM. There's probably more that
I haven't found yet, so it seems useful to check this with an audit.
This is usually a sign that the formula is installing pre-built binaries.
I'm not sure if the error from the audit should reflect this, so I've left
it out for now. Happy to tweak the wording as needed.