Support loading shared libraries inside a wheel file globally#3264
Support loading shared libraries inside a wheel file globally#3264ryanking13 merged 12 commits intopyodide:mainfrom
Conversation
packages/shapely/patches/0001-Search-libs-from-auditwheel-repaired-directory.patch
Show resolved
Hide resolved
| // TODO(ryanking13): It is not clear why global libraries should be loaded first. | ||
| // But without this, we get a fatal error when loading the libraries. |
There was a problem hiding this comment.
I guess it's just lucky we understand what's going on as much as we do.
| const needed = Module.getDylinkMetadata(binary).neededDynlibs; | ||
| needed.forEach((lib: string) => { | ||
| globalLibs.add(lib); | ||
| }); |
There was a problem hiding this comment.
Is this a conservative overestimate of the libs that need to be loaded globally? Or is this exactly the condition? Also, does this code also run for shared libraries that are loaded separately? It's fairly efficient to turn an originally local library into a global one when it turns out to be needed later. Do we handle this case?
In any case, it's really nice to see these improvements.
There was a problem hiding this comment.
Is this a conservative overestimate of the libs that need to be loaded globally? Or is this exactly the condition?
Since neededDynlibs contains all dynamic libraries linked during the compilation (via -l flag), I think this is not an overestimate unless we find a way to load symbols of external libraries loaded locally.
On the other hand, this can rather be an "underestimate" of libs, because users who write compile commands might omit to specify dynamic libraries that should be linked. For example: in #3234, I found and fixed that our sharedlib-test-py package wasn't linked to sharedlib-test.so correctly.
Also, does this code also run for shared libraries that are loaded separately?
Good point, I think we can skip this part for shared libraries that are loaded separately.
It's fairly efficient to turn an originally local library into a global one when it turns out to be needed later. Do we handle this case?
No, I think we never turn a local library into a global one for now. Since global flag is propagated to dependencies when calling loadDynamicLibrary, a locally loaded dynamic library does not load its dependencies globally.
(I'm not sure this behavior is a bug in Emscripten, probably I should open an issue in Emscripten).
There was a problem hiding this comment.
(I'm not sure this behavior is a bug in Emscripten, probably I should open an issue in Emscripten).
Opened an issue in Emscripten (emscripten-core/emscripten#18264). If it is accepted, it might be possible to remove whole global checks.
|
Thanks for the review! |
Description
Resolve #3167
Close #3183
This is a final split-up of #3183.
This PR mostly does two things:
Add a way to detect whether libraries inside the wheel should be loaded globally or not: If a library is needed by another library inside a wheel file, load it globally.
Apply
vendor-sharedlib(Add vendor-sharedlib key in meta.yaml spec #3234) keys to some packages that depend on other shared libraries: fiona (gdal), pyheif (libheif), h5py (libhdf5), shapely (geos).Note that @rth had some concerns about downloading the same library multiple times (#3167). For now I applied it to the shared libraries that are used in only one or two packages, but we can discuss more about this.
Checklists