Skip to content
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

fix: --exclude libfoo.so shall ignore dependencies of libfoo.so #474

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Jan 7, 2024

When using --exclude libfoo.so, dependencies of libfoo.so are still being analyzed & grafted.
This commit moves the exclusion analysis to lddtree and filters libfoo.so DT_NEEDED entries thus excluding its dependencies from the tree.

@mayeut mayeut force-pushed the fix-exclude branch 2 times, most recently from 03ed693 to e1760d9 Compare January 7, 2024 16:12
When using `--exclude libfoo.so`, dependencies of `libfoo.so` are still being analyzed & grafted.
This commit moves the exclusion analysis to `lddtree` and filters  `libfoo.so` `DT_NEEDED` entries thus excluding its dependencies from the tree.
Copy link

codecov bot commented Jan 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (45a8c00) 92.17% compared to head (0368c2d) 92.17%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #474   +/-   ##
=======================================
  Coverage   92.17%   92.17%           
=======================================
  Files          20       20           
  Lines        1252     1253    +1     
  Branches      304      304           
=======================================
+ Hits         1154     1155    +1     
  Misses         56       56           
  Partials       42       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mayeut mayeut requested a review from lkollar January 7, 2024 17:41
Copy link
Contributor

@lkollar lkollar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The one thing I would add is that you may want to make the exclude argument optional in get_wheel_elfdata and analyze_wheel_abi as it isn't always necessary and otherwise they require an empty set to be passed in (i.e. in tests). Not a big deal though.

@mayeut
Copy link
Member Author

mayeut commented Jan 8, 2024

The one thing I would add is that you may want to make the exclude argument optional in get_wheel_elfdata and analyze_wheel_abi as it isn't always necessary and otherwise they require an empty set to be passed in (i.e. in tests).

This reminded me to update one of the tests.
I'd rather have this parameter mandatory. In the end, it shall be added to the show & lddtree commands.
I nevertheless kept it optional in lddtree because that's one file that came from an external source and I tried not to change the semantics too much on that one (even though it should be considered private in Auditwheel context).

@mayeut mayeut merged commit 24000c6 into pypa:main Jan 8, 2024
11 checks passed
@mayeut mayeut deleted the fix-exclude branch February 3, 2024 10:52
@mayeut mayeut mentioned this pull request Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants