-
Notifications
You must be signed in to change notification settings - Fork 157
Remove libpython linkage instead of ignoring it. #590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I tested this approach using https://github.com/rapidsai/ucxx/, which has wheels that are currently (incorrectly) linking to libpython and it worked fine in patching those dependencies out. I will be updating that build to remove the dependency altogether on the build side so that we won't need this functionality from auditwheel anymore, but it would still be the safe choice in general IMHO. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #590 +/- ##
==========================================
+ Coverage 92.78% 92.82% +0.04%
==========================================
Files 21 21
Lines 1760 1771 +11
Branches 332 333 +1
==========================================
+ Hits 1633 1644 +11
Misses 77 77
Partials 50 50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
let's figure this out somewhere else if need be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR changes how libpython dependencies are handled by removing them during repair rather than silently ignoring them, and updates tests to cover the new behavior. It also relaxes the pre-commit Python version constraint to any Python 3.
- Drop
LIBPYTHON_RE
filtering inpolicy.filter_libs
- Introduce
remove_needed
calls inrepair_wheel
and implement it inpatcher
- Add
LIBPYTHON_RE
support and skipping logic inlddtree
- Update and add unit/integration tests to cover removal behavior
- Add a session fixture in
conftest.py
to clean environment variables
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/auditwheel/policy/init.py | Remove LIBPYTHON_RE exclusion from filter_libs |
src/auditwheel/lddtree.py | Add LIBPYTHON_RE , skip libpython resolution in ldd , update __all__ |
src/auditwheel/repair.py | Import LIBPYTHON_RE and call patcher.remove_needed for libpython |
src/auditwheel/patcher.py | Add remove_needed to base ElfPatcher and implement in Patchelf |
tests/unit/test_policy.py | Remove libpython entries from expected filtered libs |
tests/unit/test_lddtree.py | Add match/nomatch tests for LIBPYTHON_RE and a full ldd libpython test |
tests/unit/test_elfpatcher.py | Add test_remove_needed for Patchelf |
tests/integration/test_bundled_wheels.py | Extend integration tests for libpython removal and adjust imports |
conftest.py | Add clean_env fixture to unset auditwheel-related env vars before tests |
Comments suppressed due to low confidence (2)
tests/integration/test_bundled_wheels.py:11
- The test uses
mock.patch
butmock
is no longer imported. Addfrom unittest import mock
or importpatch
directly to avoid a NameError.
from unittest.mock import Mock
tests/integration/test_bundled_wheels.py:194
Namespace
is used but not imported; addfrom argparse import Namespace
(or adjust to use a different class) at the top of the file.
args = Namespace(
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I was wondering if we should introduce some sort of temporary override to disable this behaviour in case it causes issues, but it's always possible to pin to an older version, so I guess that's good enough.
@@ -57,6 +60,15 @@ def replace_needed(self, file_name: Path, *old_new_pairs: tuple[str, str]) -> No | |||
] | |||
) | |||
|
|||
def remove_needed(self, file_name: Path, *sonames: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is the right approach.
thanks for the review @lkollar
pinning is one way, adding I'll merge & tag a new version as I'll have some time end of next week if there are some bugs reported with a new version. |
Thanks for getting this cleaned up and merged so quickly! I appreciate the quick review. |
Resolves #589.
I also switched the pre-commit config to use any Python 3 instead of constraining to 3.9. Without that, running pre-commit fails as shown in pre-commit/pre-commit#1375.