audit: forbid links to libBLAS and libLAPACK#6174
audit: forbid links to libBLAS and libLAPACK#6174jonchang wants to merge 1 commit intoHomebrew:masterfrom jonchang:accelerate-v2
Conversation
Yes, in this case I'd much rather have false negatives than false positives. |
zbeekman
left a comment
zbeekman
left a comment
There was a problem hiding this comment.
Looks good to me. I added some suggestions for the message that I think would be improvements, but you're welcome to take them or leave them.
|
Updated per review comments. |
| 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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
- 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
veclibfortandlapack. - 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.
There was a problem hiding this comment.
(So I'm 👍 to 🚢 but won't be devastated if we don', FWIW)
There was a problem hiding this comment.
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.
|
Assuming this is 💀 |
4 participants
brew stylewith your changes locally?brew testswith your changes locally?This pull request partially reverts #6168 to be stricter about the types of undesirable links into Accelerate.framework that we would like to forbid in core formulae.
This diff may be more useful for review purposes.
As outlined in #6130, we would generally like to avoid linking against system Accelerate for core tap formulae as it includes outdated BLAS routines that are being dropped by packages in favor of OpenBLAS.
However, as we discovered in #6168 there are many other non-BLAS routines in Accelerate that can and are used in formula, so the original implementation was too restrictive.
Although many formulae just link to the base Accelerate library (#6168 (comment)), I've found that it's possible for some build systems to do "deep links" into Accelerate. That is, occasionally you see formula linking specifically to e.g.
libLAPACK.dylib, contrary to the earlier comment. This pull request checks for that case.There is a more precise solution, which is to enumerate symbols exported from
libLAPACKandlibBLASspecifically and ensure that formula that link againstAccelerate.frameworkaren't using those symbols. This would catch more improper links to Accelerate, but would also be a lot more effort and complexity and would likely also be more fragile. I'm ok with a higher false negative rate if it means easier maintenance.