Fix: Patch RPATHs of non-Python extension dependencies #283

bdice · 2021-02-06T20:06:34Z

This PR resolves #136, and replaces the open (stalled) PR #139 from @thomaslima (closes #139).

Summary of the problem

From my comment in #136, slightly edited for clarity:

We have a few Python extensions that link to libfreud.so and libtbb.so, neither of which are Python extensions. libfreud.so is also linked to libtbb.so. auditwheel correctly relocates the RPATHs of our Python extensions, but does not alter the RPATH of libfreud.so to point to the relocated libtbb.so.

Solution

Add non-Python extensions to the dictionary of full_external_refs in get_wheel_elfdata in wheel_abi.py.

Context

This PR adds the fix and ports over tests from a previous PR #134 that was reverted in #190 due to tests failing (I think the failures were caused by changes in the testing infrastructure, but I'm not 100% sure). Also related to this is PR #245 which handles a related issue that @thomaslima worked on in #140.

Testing

The hello package in tests/integration/nonpy_rpath is a minimal-ish example of such a package with a non-Python extension library with an RPATH that needs to be repaired. The tests I added are based on @thomaslima's previous work, updated to match newer conventions in the testing code for auditwheel. I think the tests are relatively complete. I manually verified that the tests fail without the fix.

…n-py extensions (pypa#134) * Adding minimal test wheel based on PR pypa#134 * Separate tests for each change in PR pypa#134 pypa#134 * non-py extension does not show in auditwheel show (ignoring assert) * Confirming tests on macOS * pytest in travis is weirdly triggering a manual test * Sorry! Forgot to install zlib-devel

codecov · 2021-02-06T21:45:07Z

Codecov Report

Merging #283 (b75e8ed) into master (fc1ed1c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
+ Coverage   89.03%   89.04%   +0.01%     
==========================================
  Files          20       20              
  Lines        1058     1059       +1     
  Branches      226      226              
==========================================
+ Hits          942      943       +1     
  Misses         69       69              
  Partials       47       47

Impacted Files	Coverage Δ
auditwheel/wheel_abi.py	`98.41% <100.00%> (+0.01%)`	⬆️

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 fc1ed1c...b75e8ed. Read the comment docs.

bdice · 2021-02-06T23:18:41Z

auditwheel/wheel_abi.py

+            full_external_refs[fn] = lddtree_external_references(
+                nonpy_elftree[fn], ctx.path)


The core of the fix in this PR is just to move this line out of the "if" statement above. Almost everything else in this PR is just adding tests.

thomaslima · 2021-02-07T20:06:22Z

Thank you so much for providing this. I really wasn't able to work on the original PRs. I wonder if you have used any of the code that I wrote for this.

bdice · 2021-02-07T20:24:44Z

Thank you so much for providing this. I really wasn't able to work on the original PRs. I wonder if you have used any of the code that I wrote for this.

Yes, I based this on your previous PRs. You can see in the commit history that I cherry-picked your previous work onto a new branch before updating the code, so that you receive credit in the git history. 👍

mayeut · 2021-02-07T20:28:09Z

Thanks for working on this @bdice, @thomaslima
I will review this next week-end at the latest.

mayeut

The functional code change is OK however, I find the test case a bit too complicated and might conflict (or more exactly, the test would become useless) with #152

A simpler, more contained test should work as well here, maybe something like tests/integration/testdependencies but going one level deeper in dependencies to cover the issue would work if I understand correctly ?

bdice · 2021-02-16T03:09:27Z

@mayeut I agree that your suggestion for a simpler test extension would be good. I had hoped it would be good enough as-is, but you're right that won't work any more with #152. It will take me some time to make these changes -- I will try to have a simpler test extension by the end of the week. Is there a release schedule or other timeline to be aware of? I would like to get this fix into the next release if possible.

psavery · 2021-02-19T18:53:23Z

@bdice Thanks for working on this. I can confirm this fixes #136 for us.

Until it's in a release, we'll patch this change into the docker image we're using with cibuildwheel.

pypa/auditwheel#136 results in pure C++ libraries not getting patched by auditwheel. This is a problem for us, since our pure C++ library, libstem.so, needs its rpath patched for hdf5. pypa/auditwheel#283 fixes this issue, but is not yet merged. Until it gets merged and added to quay.io/pypa/manylinux2010_x86_64, we will patch auditwheel inside quay.io/pypa/manylinux2010_x86_64 to include this fix. Signed-off-by: Patrick Avery <[email protected]>

bdice · 2021-02-22T06:07:56Z

Update: I just finished writing a new, simpler test case. I have to do some local testing and code cleanup before re-requesting review. I anticipate having updated tests for this PR finished in the next 2 days.

mayeut · 2021-02-22T07:43:09Z

Thanks for the update @bdice

Is there a release schedule or other timeline to be aware of? I would like to get this fix into the next release if possible.

We have no release schedule for auditwheel. Given this fixes an issue, I'd plan to release auditwheel at most 1 week after merging the PR depending on my free time to do so. The other PRs are not blocker for a release.

bdice · 2021-02-22T19:56:23Z

@mayeut I tried to create a new test, in tests/integration/test_nonpy_dependencies, but it's not catching the problem correctly (the tests pass without the fix). I had to learn the Python C API and a lot of gcc linker behaviors that I haven't dealt with before, and it looks like I didn't quite get the "failing case" correct. Maybe I can use the previous test infrastructure but just use another external library that isn't zlib? Would that fix the conflict with #152 and be alright with you as a maintainer? Do you have a recommended library that I should try to use? I appreciate the help.

Edit: also, I noticed that the command auditwheel repair --plat {policy} was resulting in a wheel for manylinux1, even if the policy was supposed to be manylinux2010 or manylinux2014. Is it expected that auditwheel will use the oldest possible platform tag for the ABIs in the wheel, even if --plat is given a newer tag? I got confused by this and couldn't confirm if that behavior is expected or not. I think my tests fail on ARM because of this (I hardcoded manylinux1_x86_64 as the expected tag).

mayeut · 2021-02-26T06:50:56Z

@bdice,
Anything other than zlib and expat would be fine I think. You can probably try using symbols from libxcrypt, it won't be whitelisted again and can be grafted properly.

Is it expected that auditwheel will use the oldest possible platform tag for the ABIs in the wheel, even if --plat is given a newer tag?

Yes, see also #281. Should be easier for tests once #289 lands.

thomaslima and others added 8 commits February 6, 2021 14:01

needed non-py extensions can also be full_external_refs

3f308f1

Move test code into integration tests directory, rename to nonpy_rpath.

6c6575c

Move test module out of the subdirectory.

40d5dfb

Update Python C API to use Py_ssize_t instead of int.

f9e9aa5

Remove old test file.

6871346

Add test for issue 136.

a5ae073

Fix lint.

501ab18

bdice commented Feb 6, 2021

View reviewed changes

bdice added 5 commits February 6, 2021 17:19

Improve .gitignore and README.

4a394a0

Format test extension Python code.

2445495

Fix file modes of non-executable files.

e14a99f

Define __all__.

eaa0ddc

Test repaired wheel outside container.

d13f36b

bdice changed the title ~~WIP: Patch RPATHs of non-Python extension dependencies~~ Patch RPATHs of non-Python extension dependencies Feb 6, 2021

bdice marked this pull request as ready for review February 6, 2021 23:54

This was referenced Feb 6, 2021

TBB linking issue on Linux when installed with pip glotzerlab/freud#720

Closed

Non-python extension dependencies with external dependencies are not recursively patched #136

Closed

bdice changed the title ~~Patch RPATHs of non-Python extension dependencies~~ Fix: Patch RPATHs of non-Python extension dependencies Feb 7, 2021

mayeut self-requested a review February 7, 2021 20:27

mayeut requested changes Feb 14, 2021

View reviewed changes

bdice added 3 commits February 22, 2021 10:36

Add tests with new extension.

3076485

Rename modules.

82a255e

Update tests.

b75e8ed

mayeut mentioned this pull request Mar 6, 2021

Fix: Patch RPATHs of non-Python extension dependencies #298

Merged

auvipy closed this in #298 Mar 18, 2021

bdice mentioned this pull request Jun 25, 2021

RPATH not getting set for linux wheel for release 2.6.1 glotzerlab/freud#803

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Patch RPATHs of non-Python extension dependencies #283

Fix: Patch RPATHs of non-Python extension dependencies #283

bdice commented Feb 6, 2021 •

edited

Loading

codecov bot commented Feb 6, 2021 •

edited

Loading

bdice Feb 6, 2021 •

edited

Loading

thomaslima commented Feb 7, 2021

bdice commented Feb 7, 2021

mayeut commented Feb 7, 2021

mayeut left a comment

bdice commented Feb 16, 2021

psavery commented Feb 19, 2021

bdice commented Feb 22, 2021

mayeut commented Feb 22, 2021

bdice commented Feb 22, 2021 •

edited

Loading

mayeut commented Feb 26, 2021 •

edited

Loading

		full_external_refs[fn] = lddtree_external_references(
		nonpy_elftree[fn], ctx.path)

Fix: Patch RPATHs of non-Python extension dependencies #283

Fix: Patch RPATHs of non-Python extension dependencies #283

Conversation

bdice commented Feb 6, 2021 • edited Loading

Summary of the problem

Solution

Context

Testing

codecov bot commented Feb 6, 2021 • edited Loading

Codecov Report

bdice Feb 6, 2021 • edited Loading

Choose a reason for hiding this comment

thomaslima commented Feb 7, 2021

bdice commented Feb 7, 2021

mayeut commented Feb 7, 2021

mayeut left a comment

Choose a reason for hiding this comment

bdice commented Feb 16, 2021

psavery commented Feb 19, 2021

bdice commented Feb 22, 2021

mayeut commented Feb 22, 2021

bdice commented Feb 22, 2021 • edited Loading

mayeut commented Feb 26, 2021 • edited Loading

bdice commented Feb 6, 2021 •

edited

Loading

codecov bot commented Feb 6, 2021 •

edited

Loading

bdice Feb 6, 2021 •

edited

Loading

bdice commented Feb 22, 2021 •

edited

Loading

mayeut commented Feb 26, 2021 •

edited

Loading