Load shared libraries locally#4876
Conversation
|
Thanks @ryanking13. The patch is so small for something that's caused us so much trouble! |
emsdk/patches/0006-Make-RTLD_LOCAL-work-correctly-for-preloaded-DSOs-21.patch
Show resolved
Hide resolved
| } | ||
|
|
||
| // Step 2) try to find the library by searching child directories of the library directory | ||
| // (This should not be necessary in most cases, but some libraries have dependencies in the child directories) |
There was a problem hiding this comment.
This is odd. Does normal linux have logic like this? Or does it have some different behavior with LD_LIBRARY_PATH or something?
There was a problem hiding this comment.
Locating the path of the shared library is handled two ways in linux.
- Using
LD_LIBRARY_PATH - Using
rpath
In emscripten, LD_LIBRARY_PATH is used in _dlopen, but we are not using it.
rpath is not supported in emscripten. I made a proposal to add it before (#3854), I guess I need to open a same proposal in emscripten too.
There was a problem hiding this comment.
Can we add a // TODO: add rpath to Emscripten dsos and remove this logic?
Yes it is. Actually, this one (emscripten-core/emscripten#18924) was a more critical fix (which is already included in Emscripten < 3.1.58), but it's also quite a small PR. |
Split off from #4876. What this PR does is basically fixing incorrect shared library dependencies. For instance, `cryptography` package depends on `openssl`, so the python extension should link to `libssl.so`. However, before this PR, it was not linked correctly. ```sh $ pyodide auditwheel show cryptography-42.0.5-cp312-cp312-pyodide_2024_0_wasm32.whl {'cryptography\\hazmat\\bindings\\_rust.cpython-312-wasm32-emscripten.so': []} ``` After this PR, it is linked correctly. ```bash $ pyodide auditwheel show cryptography-42.0.5-cp312-cp312-pyodide_2024_0_wasm32.whl { │ 'cryptography\\hazmat\\bindings\\_rust.cpython-312-wasm32-emscripten.so': [ │ │ 'libssl.so', │ │ 'libcrypto.so' │ ] } ``` __Why do we need this change?__ Because it is how shared libraries should work. __Why was this working before?__ Because we loaded dependencies globally. `openssl` is loaded globally, so its symbols were visible to `cryptography` even if they are not linked. However, #4876 will change all library loading mechanism to local, so all shared libraries should now have correct dependencies to work.
hoodmane
left a comment
hoodmane
left a comment
There was a problem hiding this comment.
Thanks @ryanking13! Great work.
| } | ||
|
|
||
| // Step 2) try to find the library by searching child directories of the library directory | ||
| // (This should not be necessary in most cases, but some libraries have dependencies in the child directories) |
There was a problem hiding this comment.
Can we add a // TODO: add rpath to Emscripten dsos and remove this logic?
|
|
Yes, agreed. Or increase the timeout. |
|
It seems like there is an additional error in the Scipy test suite. It's good to have a Scipy test so we can catch this. |
|
The and among them, |
|
Okay, I think I figured out what is happening.
The possible workarounds would be:
cc: @lesteve |
|
Why not link |
Great to hear that ❤️ Other than this, no strong opinion/expertise on this so do whatever you think makes sense. I guess at the time I worked on OpenBLAS packaging I tried a few things and stuck with the first thing I managed to get working (probably #3331 (comment) is the most relevant comment related to this) ... |
Yes, this patch will link those modules to openblas. We may even link openblas.so to every C-extension modules, which would make our life easier... but it may impact the overall load time, so let me see if I can locate all fortran-based C-extension modules and link to them |
3 participants
Description
Fixes issues with symbol resolution and changes the behavior of loading shared libraries globally to local.
I found several libraries misconfigured while working on this: not having correct dependencies, so this PR fixes them as well.
Checklists