Skip to content
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.

Sign up for GitHub

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

audit: forbid links to libBLAS and libLAPACK #6174

Closed
wants to merge 1 commit into from
Closed

audit: forbid links to libBLAS and libLAPACK #6174

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions Library/Homebrew/extend/os/mac/formula_cellar_checks.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,31 @@ def check_openssl_links
EOS
end

def check_accelerate_framework_links
return unless @core_tap
return unless formula.prefix.directory?
return if formula.name == "veclibfort" # veclibfort exists to wrap Accelerate

keg = Keg.new(formula.prefix)
system_accelerate = keg.mach_o_files.select do |obj|
dlls = obj.dynamically_linked_libraries
# libBLAS and libLAPACK are superseded by functions in OpenBLAS
# This can be linked via multiple paths, so use a wildcard match
dlls.any? { |dll| %r{/System/Library/Frameworks/Accelerate.framework/.*/(libBLAS|libLAPACK).dylib$}.match dll }
end
return if system_accelerate.empty?

<<~EOS
object files were linked against system Accelerate
These object files link against outdated BLAS/LAPACK routines provided
by the system Accelerate framework. Core tap formulae should link
against OpenBLAS instead. Removing `depends_on "veclibfort"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we already have a separate audit for this I'm not sure what this check specifically is resolving for us. What's the actual problem users are experiencing that will be solved with this check?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The original audit for accelerate linkage was rolled back in Allow Accelerate linkage, deny veclibfort & lapack #6168 and replaced with a check for dependencies on veclibfort and lapack.
  • Some build systems (e.g., python packages according to @jonchang) link directly against BLAS/LAPACK dylibs within the Accelerate framework
  • This audit is catching those cases.
  • The actual problem would be degraded performance, and occasional build bugs due to out of date BLAS/LAPACK implementation...

FWIW, I think this check probably has marginal benefits in practice, but may still be worth doing to prevent some confusion if we end up tripping over a subtle BLAS/LAPACK bug in a formula in the future due to the antiquated implementation in Accelerate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(So I'm 👍 to 🚢 but won't be devastated if we don', FWIW)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would anyone object to holding on this for e.g. a few weeks and see if we feel like we need it then? I'd like to knowingly avoid more false positives at this point.

and/or adding `depends_on "openblas"` to the formula may help.
CMake builds may need `-DBLA_VENDOR=OpenBLAS` in the configure flags.
#{system_accelerate * "\n "}
EOS
end

def check_python_framework_links(lib)
python_modules = Pathname.glob lib/"python*/site-packages/**/*.so"
framework_links = python_modules.select do |obj|
Expand Down Expand Up @@ -95,6 +120,7 @@ def audit_installed
generic_audit_installed
problem_if_output(check_shadowed_headers)
problem_if_output(check_openssl_links)
problem_if_output(check_accelerate_framework_links)
problem_if_output(check_python_framework_links(formula.lib))
check_linkage
end
Expand Down