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: Patch RPATHs of non-Python extension dependencies #298

Merged
merged 3 commits into from
Mar 18, 2021

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Mar 6, 2021

This PR resolves #136, and replaces the open (stalled) PR #139 from @thomaslima (closes #139) and rebase/rework the open PR #283 from @bdice (closes #283)

Summary in #283 is the nice one, so go there for more information.

Tests are much simpler here and still reproduce the issue without the fix of course.

Thanks @bdice, @thomaslima

@mayeut mayeut requested a review from lkollar March 6, 2021 16:07
…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
Copy link

codecov bot commented Mar 6, 2021

Codecov Report

Merging #298 (8193336) into master (4141cd1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #298   +/-   ##
=======================================
  Coverage   90.99%   91.00%           
=======================================
  Files          20       20           
  Lines        1088     1089    +1     
  Branches      235      235           
=======================================
+ Hits          990      991    +1     
  Misses         55       55           
  Partials       43       43           
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 4141cd1...8193336. Read the comment docs.

@bdice
Copy link
Contributor

bdice commented Mar 6, 2021

@mayeut Thank you so much for helping with this, I really appreciate it. I got a little stuck on #283 — some pieces of the code needed for testing this fix are new to me, but I’ve learned a lot! I just read over the PR and had a couple minor suggestions that I will mark in a review.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

This looks good to me. This PR is much cleaner than what I was doing on #283. Nice work, @mayeut! 👍

auditwheel/wheel_abi.py Outdated Show resolved Hide resolved
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@bdice
Copy link
Contributor

bdice commented Mar 16, 2021

I'm going to use this PR in my wheel building process until it's merged. Here's a simple script for anyone else that needs it:

git clone https://github.com/mayeut/auditwheel.git -b issue136 auditwheel
cd auditwheel
pip install .

For some reason, the version appears to be auditwheel 1.8.1.dev251 instead of something like 3.3.1, but the software works and I verified that the bug fix works as intended.

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.

Non-python extension dependencies with external dependencies are not recursively patched
4 participants