-
Notifications
You must be signed in to change notification settings - Fork 353
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
Make sure BBHx knows how to link locally-installed LAPACK #4855
Make sure BBHx knows how to link locally-installed LAPACK #4855
Conversation
This seems to work. An alternative approach could be to change BBHx instead, and tell its extensions to explicitly look for libraries in |
@titodalcanton @spxiwh I think ideally this would be handled on the BBHx side by providing wheels rather than building from source. I have no objection to merging this if we have a plan to remove this kind of cruft down the line. (and note in the line change so we know when we can remove). |
I really don't understand why this is happening. Some things I've observed and am confused by:
Given all that, I'm happy to approve this now, but we should raise an issue to fix this in the future. I agree with Alex that having BBHx provide wheels is best, but not sure if that's a possibility. |
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.
Approve, on the proviso that we don't forget that this hack is here .... I think there's another hack here a few lines above this to avoid picking up the latest clang, which ligolw doesn't work with, which would also be good to resolve in the future!
I agree with your observations and questions @spxiwh. When I reproduced the error locally, I was getting missing message both for |
@titodalcanton When I built BBHx using |
Ok I will merge this to unblock a bunch of other PRs, and open an issue to remind us about all the cruft in |
* Make sure BBHx knows how to link locally-installed LAPACK * Woops, fix a mistake * That did not work, but this looks better
Fixes a recent CI failure noted by @maxtrevor in #4850.
Standard information about the request
This is (kind of) a bug fix. This change affects mostly the Tox tests.
Motivation
Running
pytest
under Tox requires to install BBHx, which needs to link against LAPACK when building its wheel. LAPACK is typically installed via Conda (specified intox.ini
) and its shared libraries end up in the temporary Conda env created by Tox.For some reason which I have not figured out, this used to work fine until a few days ago, when the linker started not being able to find
liblapacke
anymore. As far as I can see, nothing is explicitly telling the linker where to look for the LAPACK shared libraries, so I am not sure how it could work before 🤷.So this PR explicitly tells the linker to look for shared libraries inside the Conda env, by setting
LIBRARY_PATH
. I do not particularly like this solution, so better ideas are welcome.Contents
Adds a few bits to
tox.ini
in order to explicitly set the env varLIBRARY_PATH
to thelib
subdirectory of the Conda env prefix. I think assumingliblapack*
will be there is safe.Links to any issues or associated PRs
Fixes a CI failure in #4850.
Testing performed
Ran Tox locally before opening the PR.