feature: allow specifying additional library paths (fixes: #373) #374
Conversation
|
NOTE: this option would actually also make sense for the |
Codecov Report
@@ Coverage Diff @@
## main #374 +/- ##
==========================================
- Coverage 92.36% 92.01% -0.36%
==========================================
Files 23 23
Lines 1258 1265 +7
Branches 307 309 +2
==========================================
+ Hits 1162 1164 +2
- Misses 55 59 +4
- Partials 41 42 +1
Continue to review full report at Codecov.
|
|
@mayeut note that all of the tests in |
Add new --add-path option to auditwheel repair to search for libraries in additional directories. This avoid having to manipulate LD_LIBRARY_PATH in the calling process (e.g. cibuildwheel), as this can have undesirable side-effects. The name of the option is chosen to match the one provided by `delvewheel` on Windows.
There was a problem hiding this comment.
Thanks for your contribution.
NOTE: this option would actually also make sense for the lddtree and show commands but I was unsure how to add it to multiple commands without duplicating code.
This is definitely needed. Probably requires a small helper file to setup the correct parser argument to all three & fix up LD_LIBRARY_PATH in a single place.
Note that all of the tests in tests/integration/test_manylinux.py which currently manipulate LD_LIBRARY_PATH could be rewritten to use this new option. Would you like me to do this?
For part of them yes, It'd be good to keep some of them using the environment variable directly in order to ensure this won't get broken in the future.
| p.add_argument( | ||
| "--add-path", | ||
| dest="PATHS", | ||
| default="", | ||
| help=f"Additional path(s) to search for libraries, {os.pathsep!r}-delimited", | ||
| ) |
There was a problem hiding this comment.
I understand that you wanted to use the same name as delvewheel which seems like a good thing however, it feels more like -L/--library-path or -rpath-link to mimic a bit more what feels like ld would be doing. I'd probably go with --library-path, allowed to be specified multiple times. Unfortunately, -L is already used for something else...
I'd be in favor of having the option specified multiple times with a single path each time rather than a single string option.
@lkollar, any thoughts on the name ? (Once it gets in, we won't be able to change it without breaking users).
There was a problem hiding this comment.
I agree that passing multiple paths is nicer with --library-path specified multiple times. However, overall I don't fully understand why this feature is necessary. What this PR does is functionally equivalent with running LD_LIBRARY_PATH=<path> audithweel repair. Doing this in cibuildwheel would need something like CIBW_REPAIR_WHEEL_COMMAND='LD_LIBRARY_PATH=<path> audithweel repair -w {dest_dir} {wheel}'. Maybe I'm missing something?
There was a problem hiding this comment.
@lkollar you're not missing anything, this is accurate, it can indeed be achieved by manipulating LD_LIBRARY_PATH. I hadn't realized that the "repair wheel command" was invoked in a shell, making it possible to alter environment variables. Now I'm not sure whether we actually want this option..
There was a problem hiding this comment.
@jlaine I guess too many people do not realize that LD_LIBRARY_PATH is a solution for ValueError: Cannot repair wheel, because required library "xxx.so" could not be located, and that's exactly the reason why this PR is needed.
Unfortunately,
-Lis already used for something else...
@mayeut and I can't say I didn't try it, because there was no other option.
--add-path is good for parity with delvewheel https://github.com/adang1345/delvewheel#additional-options
I would also add the pointer to it the error message, so that instead of plain ValueError there is a hint what to do next.
There was a problem hiding this comment.
Actually the proper fix is to propagate the option values to lddtree here. It already parses LD_LIBRARY_PATH, so no need to handle that.
auditwheel/src/auditwheel/wheel_abi.py
Line 83 in 55bc5ee
There was a problem hiding this comment.
Appears it is not that easy. If ldpaths to lddtree() is supplied, then LD_LIBRARY_PATH won't be parsed. Don't know if it is a good thing or not. Also my knowledge is not enough to understand the dict params.
auditwheel/src/auditwheel/lddtree.py
Lines 315 to 318 in 55bc5ee
|
What the progress on this? I have ran into this issue repairing wheels that were built linking to libraries installed bia conda. |
|
Would this PR resolve limitation #1? For instance, I'd like to create a package using pure |
Add new --add-path option to auditwheel repair to search for libraries
in additional directories. This avoid having to manipulate
LD_LIBRARY_PATH in the calling process (e.g. cibuildwheel), as this can
have undesirable side-effects.
The name of the option is chosen to match the one provided by
delvewheelon Windows.fixes #373