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

Always exclude ELF dynamic linker/loader #213

Merged
merged 2 commits into from
Apr 18, 2020
Merged

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Dec 24, 2019

s390x/ppc64le ELF dynamic linkers do not start with ld-linuxand therefore, were not excluded from analysis.

Relates to #212
Fixes pypa/manylinux#412

The tests did not reveal this issue and shall be updated. I don't know how to get this done since all symbols in the linker are private (or seem to be).

@wdirons, @st-pasha, any simple test we could include to reproduce this behavior in tests ?

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #213 into master will decrease coverage by 2.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #213     +/-   ##
=========================================
- Coverage   89.25%   87.16%   -2.1%     
=========================================
  Files          19       19             
  Lines         996      974     -22     
  Branches      215      214      -1     
=========================================
- Hits          889      849     -40     
- Misses         65       86     +21     
+ Partials       42       39      -3
Impacted Files Coverage Δ
auditwheel/policy/external_references.py 97.61% <100%> (ø) ⬆️
auditwheel/main_addtag.py 29.41% <0%> (-57.44%) ⬇️
auditwheel/main_repair.py 72.09% <0%> (-4.51%) ⬇️
auditwheel/wheeltools.py 89.51% <0%> (-2.42%) ⬇️
auditwheel/lddtree.py 92.14% <0%> (-0.72%) ⬇️
auditwheel/wheel_abi.py 98.31% <0%> (-0.09%) ⬇️
auditwheel/tmpdirs.py 100% <0%> (ø) ⬆️
auditwheel/repair.py 83.33% <0%> (+2.15%) ⬆️
auditwheel/main_show.py 77.08% <0%> (+4.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b1d9ef...22f551f. Read the comment docs.

@di
Copy link
Member

di commented Feb 7, 2020

I added two integration tests for this using numpy wheels for s390x and ppc64le thanks to @wdirons in #212 -- if these aren't what we were looking for, please feel free to amend/remove my commit.

Also, I think the s390x build will probably fail here due to #228, but we should probably merge this anyways.

@di
Copy link
Member

di commented Feb 9, 2020

Tests are now passing, what do you think @mayeut?

@mayeut
Copy link
Member Author

mayeut commented Feb 14, 2020

@wdirons, @di,
thanks for the test case.
I'd really like not to have 25MB+ stored in the repo. I'll try to see if I can get this shrunk a bit.

@wdirons
Copy link

wdirons commented Feb 18, 2020

I'm trying to figure out how to modify the test_elfutils.py test for this instance.

I have noticed, the existing testcase for ld-linux is not testing what it should as this change still passes the test:

diff --git a/tests/unit/test_elfutils.py b/tests/unit/test_elfutils.py
index 23e31e7..045ad5b 100644
--- a/tests/unit/test_elfutils.py
+++ b/tests/unit/test_elfutils.py
@@ -99,7 +99,7 @@ class TestElfFindVersionedSymbols:
         # GIVEN
         elf = Mock()
         verneed = Mock()
-        verneed.configure_mock(name="ld-linux")
+        verneed.configure_mock(name="foo-bar")
         elf.get_section_by_name.return_value.iter_versions.return_value = (
             (verneed, []),
         )

@mayeut mayeut force-pushed the ld-linux branch 2 times, most recently from 7bc7c99 to 452dd83 Compare April 18, 2020 11:02
@mayeut
Copy link
Member Author

mayeut commented Apr 18, 2020

I finally managed to get some time to find a small test case.
Relying on TLS creates a dependency on the ELF dynamic linker/loader.

Failures can be seen here when the fix is not here: https://travis-ci.com/github/mayeut/auditwheel/jobs/320711188 & https://travis-ci.com/github/mayeut/auditwheel/jobs/320711187

@mayeut
Copy link
Member Author

mayeut commented Apr 18, 2020

CI not passing but passing here https://travis-ci.com/github/mayeut/auditwheel/builds/160833993
so going ahead with merge

@mayeut mayeut merged commit 1a4bf56 into pypa:master Apr 18, 2020
@mayeut mayeut deleted the ld-linux branch April 24, 2020 19:09
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.

Problems building wheel for manylinux2014_ppc64le platform
4 participants