[Updated] Append to RPATH (instead of overwrite) #245

fbrennen · 2020-06-05T07:06:35Z

Closes #137 (also closes #69 as far as I can tell)

A refreshed version of work done by @thomaslima in #140

codecov · 2020-06-05T07:42:56Z

Codecov Report

Merging #245 into master will increase coverage by 0.49%.
The diff coverage is 93.47%.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   89.72%   90.21%   +0.49%     
==========================================
  Files          20       20              
  Lines        1012     1053      +41     
  Branches      215      223       +8     
==========================================
+ Hits          908      950      +42     
+ Misses         64       61       -3     
- Partials       40       42       +2

Impacted Files	Coverage Δ
auditwheel/patcher.py	`95.89% <93.33%> (+5.56%)`	⬆️
auditwheel/repair.py	`83.09% <100.00%> (ø)`
auditwheel/genericpkgctx.py	`44.44% <0.00%> (+4.44%)`	⬆️

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 a9b4794...c0bb677. Read the comment docs.

lkollar · 2020-06-05T09:14:38Z

auditwheel/patcher.py

@@ -61,6 +61,10 @@ def set_rpath(self,
                  file_name: str,
                  rpath: str) -> None:

+        old_rpath = check_output(['patchelf', '--print-rpath',


I think this logic should not live in this method as other implementations of ElfPatcher in the future will not work consistently (my understanding is that having to append the RPATH is not a patchelf quirk, we should always do it). set_rpath should just set the provided RPATH.

We should either have get_rpath and append_rpath methods on the interface, or have an append flag which can trigger this behaviour.

Sounds good -- if you don't have a particular preference I'll extend the interface.

How's this?

fbrennen · 2020-06-05T12:15:09Z

~~I don't quite understand how to parse the coverage failures -- it looks like adding to the ElfPatcher interface has reduced coverage, but there are no tests for that at all.~~ I exempted NotImplementedError from the coverage calculation and that addressed the issue.

lkollar · 2020-06-05T13:23:35Z

Thanks @fbrennen, the change looks good. I thought about this a bit more and, still have a question about the overall change: what happens if an extension has a relative RPATH to some library, and this library is present on the system where the wheel is installed? I can imagine a situation where the wrong library is picked up from the system, instead of the vendored library. For example, liba.so has RUNPATH=/opt/lib/ and after we graft it into the wheel, it will still pick up dependencies from that location if they are present. This is not a problem at the moment because we clear out the RUNPATH, "isolating" the dependency. What do you think?

fbrennen · 2020-06-05T14:23:18Z

I see what you're saying -- the core issue that I (and I think @thomaslima ) have is that the RPATHS for libraries that are already in the wheel are getting trampled -- the place where those RPATHs are adjusted is separate from the one applied to generic libraries grafted into the wheel. I could make the first one use append and the second use set -- that way you're preserving RPATH entries only for libraries you've already explicitly included. How does that sound?

lkollar · 2020-06-06T08:13:32Z

Yes, I think that works. However, I would like to better understand the problem before we make this change, just to make sure we are not missing something here, as this is quite a significant change in existing logic. See #134 (comment) as well. Sorry for holding up the PR, I'll try to report back ASAP.

fbrennen · 2020-06-06T09:21:01Z

Not a problem -- I appreciate this could have unexpected consequences.

fbrennen · 2020-06-06T09:33:15Z

Ah, I see that the first RPATH adjustment is done inside an InWheelCtx -- I imagine it would not be hard to use that to require that preserved RPATH entries only point to paths within the wheel.

lkollar · 2020-06-07T13:55:39Z

I looked into this a bit more and I think we have to address the following issues:

We need to avoid the RPATH pointing to libraries outside of the wheel. This is not a problem at the moment as we always remove the existing RPATH, but the append logic can cause issues if the existing RPATH points to a location outside of the wheel. I think the best solution for this is to actually resolve the existing RPATH (replacing tokens like $ORIGIN as described in the "Rpath token expansion" section in the ld.so man page) and strip it if it points outside the wheel. We can also consider only appending the RPATH of the extension modules, but this still removes the guarantee we provide today, that an existing RPATH on the extension will be stripped. This can cause issues.
If we blindly append the RPATH, multiple auditwheel invocations can add the same RPATH multiple times. We should check if what we are appending is already present.

I think given these, the best solution would be to create a function which is used to append the RPATH of Python extensions (but not other .sos), but it also checks if the RPATH is already present, and resolves the existing RPATH or RUNPATH entries into "real" paths and strips them if they point outside of the wheel. How does this sound @fbrennen?

fbrennen · 2020-06-07T14:16:19Z

That sound good to me, and not too hard to do -- I'll adjust append_rpath() to additionally require the main wheel directory, which should make it straightforward to preserve only "safe" RPATH entries. We can deal with duplicate entries at the same time. auditwheel already isolates the Python extensions and deals with them separately to other libraries, so that will come for free.

…int within the wheel structure and are not duplicates

…st that they get installed in different locations

fbrennen · 2020-06-15T16:49:31Z

Hi @lkollar -- sorry for the delay. Here's an initial go at implementing what I think you were after. What do you think?

lkollar

Hey @fbrennen thanks for taking the time and implementing the changes. I think the overall approach looks good. I left a few comments, but I haven't had time to review the tests yet.

lkollar · 2020-06-15T21:32:13Z

auditwheel/patcher.py

+                     file_name: str,
+                     rpath: str,
+                     wheel_base_dir: str) -> None:
+        """Add a new rpath entry to a file while preserving as many existing


I think we should keep the ELF patcher implementation as a thin layer abstracting ELF operations, and move anything which is not an implementation detail of a specific ELF patcher to the caller. I think append should really just append and the caller should implement all the checks on duplicates, etc. Consider that all of this logic would probably be implemented the exact same way in a different implementation of ELFPatcher, so it's probably a better idea to extract it into a set of common functions.

Ah, good call, ok -- so move this into repair_wheel()?

Ok, I moved all this into repair_wheel() along with the tests. There is a slight change to the way patching works -- the patcher calls now always modify the absolute path to the library. I can't think of a reason why this would be a problem but it is different. If you object it would be very easy to change.

auditwheel/patcher.py

lkollar · 2020-06-15T21:47:41Z

auditwheel/patcher.py

+        2) Not be a duplicate of an already-existing rpath entry.
+        """
+        old_rpaths = self.get_rpath(file_name)
+        old_rpaths = _preserve_existing_rpaths(old_rpaths, file_name,


I think this implementation is OK, but I think it could be simplified a bit by breaking _preserve_existing_rpaths up.

Create a single function to check if an RPATH is under wheel_base (e.g _is_valid_rpath) and do

rpaths = filter(is_valid_rpath, old_rpaths.split(':'))

Then create the ordered set out of rpaths, and stick the new RPATH in the end. This way we don't have to do the duplicate check.

I might have missed something, but this is the general idea.

Not a problem -- that does sound simpler. =)

fbrennen · 2020-06-17T20:13:33Z

@lkollar the s390x build appears to have exploded before it could run -- it did pass here. Could you get it to run again? Thanks!

auditwheel/repair.py

lkollar · 2020-06-18T21:17:20Z

@lkollar the s390x build appears to have exploded before it could run -- it did pass here. Could you get it to run again? Thanks!

There seems to be an issue on Travis with s390x builds: https://travis-ci.community/t/s390-xenial-builds-fail-with-an-error-occurred-while-generating-the-build-script/9039. This platform has been problematic on Travis, I would just ignore the failure.

lkollar · 2020-06-22T21:36:18Z

Thanks @fbrennen!

fbrennen · 2020-06-23T07:08:12Z

No problem =)

sc1f · 2020-06-23T13:04:55Z

Thanks y'all for this fix. I was working on this issue yesterday and saw this PR was open, and I'm glad it's been merged!

@finos/perspective suffers from the same issue as described in #69 and #137 - we have a libbinding.so of Pybind bindings, which depend on libpsp.so and libtbb.so in the same folder next to libbinding.

Because it's detected via $ORIGIN in rpath, auditwheel does not include it in the external libs and so does not copy it out during auditwheel repair. Thus, when the rpath is overwritten to the libs folder created by auditwheel, libbinding.so can't find the libraries.

Looking over this patch it seems like that issue will be fixed in the next release? Thanks - appreciate the hard work!

Update I've been using auditwheel from master and it fixes our issue entirely. Thanks!

Seems like this is fixed via pypa/auditwheel#245

thomaslima and others added 5 commits February 1, 2019 18:21

appending to rpath instead of overwriting

8833c32

Update from master

8e9a5d1

Patcher appends to existing rpath entries

887b50c

Update rpath tests to add return values to check_call

7a3bd33

Use check_output instead of check_call

27b4b68

fbrennen mentioned this pull request Jun 5, 2020

Append to RPATH (instead of overwrite) #140

Closed

Clarify test output slightly

212e6eb

fbrennen force-pushed the issue137 branch from c54c655 to 212e6eb Compare June 5, 2020 07:12

Fix small typo

29084a5

auvipy requested a review from lkollar June 5, 2020 08:08

auvipy approved these changes Jun 5, 2020

View reviewed changes

lkollar requested changes Jun 5, 2020

View reviewed changes

fbrennen added 2 commits June 5, 2020 12:07

Separate out set_rpath into set_, get_, and append_

d7adb23

Correct linting error

488e904

Exempt NotImplementedError from coverage calculations

b594167

lkollar self-assigned this Jun 6, 2020

fbrennen added 5 commits June 15, 2020 08:41

append_rpath will now only preserve existing rpath entries if they po…

6f3cc6a

…int within the wheel structure and are not duplicates

Initial attempt at an integration test

c38b9a3

Switch to using is_subdir instead of duplicating its functionality

899f6f3

Also give example_b extension an RPATH entry so it can find liba

024d526

Search for libb in the correct directory

01a3a6b

fbrennen added 2 commits June 15, 2020 15:53

Split apart libs a and b for rpath test so that we can more easily te…

c757139

…st that they get installed in different locations

Lint

a32f179

lkollar reviewed Jun 15, 2020

View reviewed changes

fbrennen added 2 commits June 17, 2020 20:20

Move rpath appending logic into repair

42631c5

Also pass patcher to append_rpath_within_wheel

c0bb677

lkollar reviewed Jun 18, 2020

View reviewed changes

auditwheel/repair.py Outdated Show resolved Hide resolved

Correctly maintain OrderedDict ordering when initializing it.

70c0282

lkollar approved these changes Jun 22, 2020

View reviewed changes

lkollar merged commit 4709edb into pypa:master Jun 22, 2020

sc1f mentioned this pull request Jun 29, 2020

Generate/test Manylinux wheels, clean up wheel/build process finos/perspective#1105

Merged

This was referenced Jul 2, 2020

Bump auditwheel from 3.1.1 to 3.2.0 in /docker/build_scripts pypa/manylinux#646

Merged

Bump auditwheel from 3.1.1 to 3.2.0 in /docker/build_scripts pypa/manylinux#647

Merged

Bump auditwheel from 3.1.1 to 3.2.0 in /docker/build_scripts pypa/manylinux#648

Merged

lkollar mentioned this pull request Aug 7, 2020

Wrong rpath after repair #257

Open

dependabot bot mentioned this pull request Aug 11, 2020

Bump auditwheel from 3.1.1 to 3.2.0 home-assistant/wheels#126

Merged

dependabot bot mentioned this pull request Aug 26, 2020

Bump auditwheel from 2.1.1 to 3.2.0 in /glean-core/python mozilla/glean#1174

Merged

dependabot bot mentioned this pull request Sep 29, 2020

Bump auditwheel from 3.1.1 to 3.2.0 snowflakedb/snowflake-connector-python#444

Closed

bdice mentioned this pull request Feb 6, 2021

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

Closed

pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Apr 16, 2021

Use official auditwheel for RPATH handling (#1182)

e310108

Seems like this is fixed via pypa/auditwheel#245

alexsavulescu added a commit to neuronsimulator/nrn that referenced this pull request Apr 19, 2021

Use official auditwheel for RPATH handling (#1182)

b83407d

Seems like this is fixed via pypa/auditwheel#245

alexsavulescu added a commit to neuronsimulator/nrn that referenced this pull request Apr 30, 2021

Use official auditwheel for RPATH handling (#1182)

6856136

Seems like this is fixed via pypa/auditwheel#245

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Updated] Append to RPATH (instead of overwrite) #245

[Updated] Append to RPATH (instead of overwrite) #245

fbrennen commented Jun 5, 2020 •

edited

Loading

codecov bot commented Jun 5, 2020 •

edited

Loading

lkollar Jun 5, 2020

fbrennen Jun 5, 2020

fbrennen Jun 5, 2020

fbrennen commented Jun 5, 2020 •

edited

Loading

lkollar commented Jun 5, 2020 •

edited

Loading

fbrennen commented Jun 5, 2020

lkollar commented Jun 6, 2020

fbrennen commented Jun 6, 2020

fbrennen commented Jun 6, 2020

lkollar commented Jun 7, 2020

fbrennen commented Jun 7, 2020

fbrennen commented Jun 15, 2020

lkollar left a comment

lkollar Jun 15, 2020

fbrennen Jun 16, 2020

fbrennen Jun 17, 2020

lkollar Jun 15, 2020

fbrennen Jun 16, 2020

fbrennen Jun 17, 2020

fbrennen commented Jun 17, 2020

lkollar commented Jun 18, 2020

lkollar commented Jun 22, 2020

fbrennen commented Jun 23, 2020

sc1f commented Jun 23, 2020 •

edited

Loading

[Updated] Append to RPATH (instead of overwrite) #245

[Updated] Append to RPATH (instead of overwrite) #245

Conversation

fbrennen commented Jun 5, 2020 • edited Loading

codecov bot commented Jun 5, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrennen commented Jun 5, 2020 • edited Loading

lkollar commented Jun 5, 2020 • edited Loading

fbrennen commented Jun 5, 2020

lkollar commented Jun 6, 2020

fbrennen commented Jun 6, 2020

fbrennen commented Jun 6, 2020

lkollar commented Jun 7, 2020

fbrennen commented Jun 7, 2020

fbrennen commented Jun 15, 2020

lkollar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrennen commented Jun 17, 2020

lkollar commented Jun 18, 2020

lkollar commented Jun 22, 2020

fbrennen commented Jun 23, 2020

sc1f commented Jun 23, 2020 • edited Loading

fbrennen commented Jun 5, 2020 •

edited

Loading

codecov bot commented Jun 5, 2020 •

edited

Loading

fbrennen commented Jun 5, 2020 •

edited

Loading

lkollar commented Jun 5, 2020 •

edited

Loading

sc1f commented Jun 23, 2020 •

edited

Loading