Fix for macos local wheels issue#2874
Conversation
kipawaa
added
the
author:build-wheels
|
Hello. You may have forgotten to update the changelog!
|
| lib_with_r_path = "@rpath/libcustom_calls.so" | ||
|
|
||
| original_path = frontend_path[0] if frontend_path else build_path[0] | ||
| original_path = build_path[0] if build_path else frontend_path[0] |
There was a problem hiding this comment.
I wondering if this would fail when the order of installation is reversed? i.e. installing the wheel first and then make all.
Would it make sense to run install_name_tool on both? 🤔
There was a problem hiding this comment.
When building from source the INSTALLED python variable is set to false, and the path resolution searches static relative paths. I think this would cause the dylib resolution to fail, but maybe I'm wrong. Can test this tomorrow and report back.
There was a problem hiding this comment.
@mehrdad2m it seems that running install_name_tool on both succeeds! (4fb384f)
There was a problem hiding this comment.
Isn't this writing to the same field twice?
EDIT: oh nvm it's the opposite, you're writing the same value to different libraries
There was a problem hiding this comment.
Good catch on this build folder thing! I wonder if instead of us searching particular paths, shouldn't we able to find the lib we just built directly (since this is the code that builds it in the first place)?
There was a problem hiding this comment.
shouldn't we able to find the lib we just built directly
coming back to this, I think this makes sense to me. Looking at the build_ext class, it has a get_ext_filename method that we could potentially use but I am not familiar with that module enough to know for sure without testing.
There was a problem hiding this comment.
I was wondering why I was seeing this line in River's PR, I thought we had already merged this 😅
There was a problem hiding this comment.
But yeah I think it's worth updating this properly for good, if it doesn't take too long
There was a problem hiding this comment.
I likely won't be able to get back to this today, but I can try to check that out tomorrow/monday.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2874 +/- ##
=======================================
Coverage 97.01% 97.01%
=======================================
Files 167 167
Lines 18828 18828
Branches 1768 1768
=======================================
Hits 18266 18266
Misses 417 417
Partials 145 145 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
Context:
MacOS locally-built wheels would fail thanks to
install_name_toolpreferring the wrong dylib.Description of the Change:
Prefers the correct dylib.
Benefits:
working wheels
Possible Drawbacks:
Related GitHub Issues: