Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #683 +/- ##
==========================================
+ Coverage 95.24% 95.29% +0.04%
==========================================
Files 22 22
Lines 1830 1849 +19
Branches 343 347 +4
==========================================
+ Hits 1743 1762 +19
Misses 48 48
Partials 39 39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
53d3e1d to
037f6a0
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to fix auditwheel repair behavior for wheels that contain internal shared libraries that lddtree fails to resolve (e.g., manylinux_2_28 case in #531), and also fixes a case where repair could graft/retag using the wrong ABI policy.
Changes:
- Add
repair_external_refsalongsidefull_external_refsand introduce filtering logic intended to drop unresolved internal sonames that already exist in the wheel. - Update
repair_wheelto graft/retag using an explicittarget_abi(requested policy) rather than relying onabis[0]. - Add unit tests covering the new filtering behavior and the requested-ABI grafting behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/auditwheel/wheel_abi.py |
Adds repair_external_refs and filtering of unresolved internal sonames for repair. |
src/auditwheel/repair.py |
Uses repair_external_refs and a new target_abi parameter to choose which policy drives grafting/retagging. |
src/auditwheel/main_repair.py |
Passes the requested policy name as target_abi into repair_wheel. |
tests/unit/test_wheel_abi.py |
Adds tests around filtering internal unresolved references for repair. |
tests/unit/test_repair.py |
Adds a test ensuring grafting uses the requested ABI policy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elftree, | ||
| ctx.path, | ||
| ) | ||
| repair_external_refs[fn] = full_external_refs[fn] |
There was a problem hiding this comment.
repair_external_refs for Python extensions is currently set to full_external_refs without applying _filter_repair_refs. That means unresolved DT_NEEDED entries that are actually internal (the library exists somewhere in the wheel, but lddtree couldn't resolve it so the path is None) will still be present for the extension and will still trigger ValueError: required library ... could not be located during repair_wheel. Consider building repair_external_refs for all ELFs (including Python extensions) by filtering against wheel_sonames after you’ve collected the wheel’s sonames, and only keeping refs that still have libs/blacklist after filtering.
| repair_external_refs[fn] = full_external_refs[fn] | |
| # Apply the same external-reference filtering used for other ELFs | |
| # so that unresolved DT_NEEDED entries that are actually provided | |
| # by libraries inside the wheel (wheel_sonames) do not cause | |
| # spurious repair failures. | |
| filtered_refs = _filter_repair_refs( | |
| {fn: full_external_refs[fn]}, | |
| wheel_sonames, | |
| ) | |
| if fn in filtered_refs: | |
| repair_external_refs[fn] = filtered_refs[fn] |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wheel_sonames: frozenset[str], | ||
| ) -> dict[str, ExternalReference]: | ||
| """Return external refs with unresolved sonames already present in the | ||
| wheel removed. | ||
|
|
||
| ``external_refs`` maps policy name to the libraries needed by one ELF. | ||
| ``wheel_sonames`` is the set of shared-library sonames already present in | ||
| the wheel. The returned mapping keeps only refs that still need repair. | ||
| """ | ||
| filtered_refs = {} | ||
| for name, external_ref in external_refs.items(): | ||
| libs = { | ||
| lib: path | ||
| for lib, path in external_ref.libs.items() | ||
| if path is not None or lib not in wheel_sonames |
There was a problem hiding this comment.
wheel_sonames is built from fn.name (the filenames of ELF entries in the wheel), but _filter_repair_refs and its docstring describe this as a set of SONAMEs. To avoid confusion for future maintainers, consider renaming the parameter/variable (e.g., to reflect DT_NEEDED/filename matching) and updating the docstring accordingly.
| wheel_sonames: frozenset[str], | |
| ) -> dict[str, ExternalReference]: | |
| """Return external refs with unresolved sonames already present in the | |
| wheel removed. | |
| ``external_refs`` maps policy name to the libraries needed by one ELF. | |
| ``wheel_sonames`` is the set of shared-library sonames already present in | |
| the wheel. The returned mapping keeps only refs that still need repair. | |
| """ | |
| filtered_refs = {} | |
| for name, external_ref in external_refs.items(): | |
| libs = { | |
| lib: path | |
| for lib, path in external_ref.libs.items() | |
| if path is not None or lib not in wheel_sonames | |
| wheel_lib_names: frozenset[str], | |
| ) -> dict[str, ExternalReference]: | |
| """Return external refs with unresolved libraries already present in the | |
| wheel removed. | |
| ``external_refs`` maps policy name to the libraries needed by one ELF. | |
| ``wheel_lib_names`` is the set of shared-library names/filenames already | |
| present in the wheel (used for DT_NEEDED/filename-style matching). The | |
| returned mapping keeps only refs that still need repair. | |
| """ | |
| filtered_refs = {} | |
| for name, external_ref in external_refs.items(): | |
| libs = { | |
| lib: path | |
| for lib, path in external_ref.libs.items() | |
| if path is not None or lib not in wheel_lib_names |
cc740a2 to
646c046
Compare
I fail to understand the issue, I guess this is related to the I'm trying to understand what the new filtering is doing exactly. Do you have an example before/after to ease this (or an integration test) ? |
|
You're right, that change is unrelated. I'll move it to a new PR and try to come up with test to reproduce the issue. |
Non-extension shared objects that are already reached through a wheel ELF are being analysed again as standalone repair roots. This drops the load context from the parent ELF and can turn vendored wheel libraries into unresolved external refs. To fix this, only collect external refs for non-extension ELFs that are true roots.
for more information, see https://pre-commit.ci
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
for more information, see https://pre-commit.ci
This should fix #531. While investigating, also discovered a subtle bug when auditwheel can use the wrong manylinux tag when repairing the wheel.