Skip to content

audit: check Accelerate linkage for core formulae#6130

Merged
MikeMcQuaid merged 1 commit intoHomebrew:masterfrom
jonchang:audit-accelerate
May 14, 2019
Merged

audit: check Accelerate linkage for core formulae#6130
MikeMcQuaid merged 1 commit intoHomebrew:masterfrom
jonchang:audit-accelerate

Conversation

@jonchang
Copy link
Copy Markdown
Contributor

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

In concert with Homebrew/homebrew-core#39809, Homebrew/homebrew-core#39226, and Homebrew/homebrew-core#37898 (comment); this new audit will check core tap formulae for undesired linkage against system Accelerate.

No new tests written since none exist for this file.

@jonchang jonchang requested review from fxcoudert and zbeekman May 14, 2019 03:41
Copy link
Copy Markdown
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks @jonchang!

@MikeMcQuaid MikeMcQuaid merged commit 66dfd8c into Homebrew:master May 14, 2019
@jonchang jonchang deleted the audit-accelerate branch May 14, 2019 11:58
@zbeekman
Copy link
Copy Markdown
Contributor

Nice! N.B., however, that veclibfort.rb, by design, will always link to Accelerate and should be an exception to this rule.

Should I change this to make an exception for veclibfort? Or should we end up removing veclibfort? (I do not favor removal, personally.)

@MikeMcQuaid
Copy link
Copy Markdown
Member

Should I change this to make an exception for veclibfort?

Yes, please

@jonchang
Copy link
Copy Markdown
Contributor Author

Good point. I think adding an exception is probably the best idea. Want to file a follow up? I can do it later this week as well.

@zbeekman
Copy link
Copy Markdown
Contributor

@MikeMcQuaid I think this is actually a bit too strict. There are cases in which users may want to link accelerate, as it provides more than just linear algebra.

https://developer.apple.com/documentation/accelerate

I think instead we should check for linkage against veclibfort and lapack. (Veclibfort is specifically for BLAS/LAPACK provided through Accelerate, and lapack reference implementation is old and minimally maintained and outdated.)

I can open a new PR with this. I hit an instance in core where a package links to Accelerate, seemingly for reasons other than linear algebra: Homebrew/homebrew-core#39418

@zbeekman
Copy link
Copy Markdown
Contributor

I'm tempted to add -DBLA_VENDOR=OpenBLAS to the std_cmake_flags which will prevent CMake formula using the included standard FindBLAS and FindLAPACK intrinsic modules from picking up Accelerate. The only issue I foresee arising may be if they want, e.g., scalapack over OpenBLAS.

@zbeekman
Copy link
Copy Markdown
Contributor

In fact, both tests could apply on linux too. 1) veclibfort should not be ported to linux (hopefully this is already true) and 2) lapack is outdated/crusty on linux also.

zbeekman added a commit to zbeekman/brew that referenced this pull request May 23, 2019
 - Accelerate provides more than just BLAS and LAPACK functionality, see
   https://developer.apple.com/documentation/accelerate
 - Veclibfort exists only to wrap Accelerate's BLAS/LAPACK
 - LAPACK is a slow, seldom updated reference implementation
 - Encourage usage of OpenBLAS
 - Reverts PR Homebrew#6130
@lock lock bot added the outdated PR was locked due to age label Jan 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants