fix: always set RPATH or clear RUNPATH/RPATH for grafted libraries#693
fix: always set RPATH or clear RUNPATH/RPATH for grafted libraries#693
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts how auditwheel handles RUNPATH/RPATH for grafted libraries to improve runtime reliability for transitive bundled dependencies while minimizing unnecessary patchelf mutations.
Changes:
- Set
$ORIGINRPATH on grafted libraries that depend on other grafted libraries; clear RUNPATH/RPATH for grafted “leaf” libraries. - Introduce
ElfPatcher.clear_rpath()and remove the now-unusedelf_read_rpaths()helper. - Remove unit tests that targeted the deleted
elf_read_rpaths()function.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/auditwheel/repair.py |
Updates grafted-lib postprocessing to set $ORIGIN RPATH when internal grafted dependencies exist; otherwise clears RUNPATH/RPATH. |
src/auditwheel/patcher.py |
Adds clear_rpath() API and implements it via patchelf --remove-rpath. |
src/auditwheel/elfutils.py |
Removes elf_read_rpaths() implementation and related import usage. |
tests/unit/test_elfutils.py |
Removes tests for the deleted elf_read_rpaths() function and its import. |
Comments suppressed due to low confidence (1)
src/auditwheel/repair.py:166
- The inline comment about clearing RUNPATH/RPATH and setting RPATH is now stale:
copylib()no longer reads or updates RPATH/RUNPATH, but the comment still claims it does. Please update/remove this comment so it matches the current behavior.
# Copy the a shared library from the system (src_path) into the wheel
# if the library has a RUNPATH/RPATH we clear it and set RPATH to point to
# its new location.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #693 +/- ##
==========================================
- Coverage 95.34% 95.31% -0.04%
==========================================
Files 22 22
Lines 1870 1858 -12
Branches 355 350 -5
==========================================
- Hits 1783 1771 -12
Misses 48 48
Partials 39 39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/auditwheel/repair.py:166
- The comment above
copylib()still says the function clears RUNPATH/RPATH and sets a new RPATH, butcopylib()no longer touches RPATH (it’s now handled later inrepair_wheel()when iteratingsoname_map). Please update/remove this comment so it matches the current behavior and doesn’t mislead future changes.
# Copy the a shared library from the system (src_path) into the wheel
# if the library has a RUNPATH/RPATH we clear it and set RPATH to point to
# its new location.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| patcher.set_rpath(path, "$ORIGIN") | ||
| patcher.replace_needed(path, *replacements) | ||
| else: | ||
| patcher.clear_rpath(path) |
There was a problem hiding this comment.
Can it happen that we clear the RPATH for entries which are internal to the wheel? I don't think those will show up in soname_map and this can cause a regression where we clear a RUNPATH which is required to pick up .so's internal to the wheel.
There was a problem hiding this comment.
This only applies to external grafted libraries so we can't be modifying for entries which are internal to the wheel.
| # we need to update those records so each now knows about the new | ||
| # name of the other. | ||
| # we also clear or set RPATH depending on the presence of internal dependencies | ||
| for _, path in soname_map.values(): |
There was a problem hiding this comment.
I think there can be a logic issue here if the same library is stored under multiple SONAME entries in soname_map:
soname_map["libfoo.so"] = ("libfoo-abc.so", libfoo-abc.so)
soname_map["libfoo.so.1"] = ("libfoo-abc.so", libfoo-abc.so)The first loop iteration will call set_rpath(path, "$ORIGIN") and rewrite internal DT_NEEDED entries. Processing the second SONAME entry pointing to the same library elf_read_dt_needed will see the already-rewritten name and find no matching replacements, and invoke clear_rpath(path).
One way to solve this would be to perform this in two passes: first record what needs changes and in a second pass, perform the actual changes to RPATH.
There was a problem hiding this comment.
Any given path can only have a single soname so this can't happen.
follow-up on #643 (comment)
The rationale for not setting
RPATHunconditionally is that every patchelf modification comes with a risk and that it's mostly not needed for runtime.There are a number of issues requesting that it'd be set anyway.
The proposed solution here is to set
RPATHwhen grafted dependency has itself other grafted dependencies and to clearRUNPATH/RPATHwhen it's not the case. The 2nd operation should mostly be no-op patchelf-wise thus reducing the risk at least for "leaf" dependencies.fix #671
fix #617
fix #459
fix #451
fix #344
close #672