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

Put libraries in $WHEELNAME.libs #90

Merged
merged 1 commit into from
Jan 23, 2020
Merged

Put libraries in $WHEELNAME.libs #90

merged 1 commit into from
Jan 23, 2020

Conversation

lpsinger
Copy link
Contributor

@lpsinger lpsinger commented Feb 7, 2018

Should fix #89.

@lpsinger lpsinger changed the title WIP: Put libraries in $WHEELNAME.libs Put libraries in $WHEELNAME.libs Feb 7, 2018
@lpsinger
Copy link
Contributor Author

lpsinger commented Feb 7, 2018

Oh, wow. That actually worked. Now it just needs a unit test.

@lpsinger
Copy link
Contributor Author

lpsinger commented Feb 7, 2018

@njsmith, this works on all of the existing unit tests, and I can see that the libraries are getting placed where we expected. Do you think it is actually necessary to add a unit test of a wheel with multiple C extensions?

lpsinger added a commit to lpsinger/delocate that referenced this pull request Feb 7, 2018
If you have a package that contains multiple C extensions that
depend on the same shared libraries, then `delocate-wheel` saves
duplicate copies of all of the shared libraries. An example is
shown below.

This makes for packages that are several times larger than
necessary. This patch causes `delocate-wheel` to sweep up all
of the dependencies and put them in a single directory, rather
than creating duplicates.

The convention is based on the proposal in pypa/auditwheel#89
and pypa/auditwheel#90.
lpsinger added a commit to lpsinger/delocate that referenced this pull request Feb 7, 2018
If you have a package that contains multiple C extensions that
depend on the same shared libraries, then `delocate-wheel` saves
duplicate copies of all of the shared libraries.

This makes for packages that are several times larger than
necessary. This patch causes `delocate-wheel` to sweep up all
of the dependencies and put them in a single directory, rather
than creating duplicates.

The convention is based on the proposal in pypa/auditwheel#89
and pypa/auditwheel#90.
lpsinger added a commit to lpsinger/delocate that referenced this pull request Oct 2, 2018
If you have a package that contains multiple C extensions that
depend on the same shared libraries, then `delocate-wheel` saves
duplicate copies of all of the shared libraries.

This makes for packages that are several times larger than
necessary. This patch causes `delocate-wheel` to sweep up all
of the dependencies and put them in a single directory, rather
than creating duplicates.

The convention is based on the proposal in pypa/auditwheel#89
and pypa/auditwheel#90.
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

I think it is necessary to add a test to ensure the behaviour works right now and in the future when changes are made. Please do add one.

@daa
Copy link
Contributor

daa commented Dec 12, 2018

I can make a pull request with unit test to @lpsinger branch but it has to be merged with pypa/auditwheel master, or I can make such pull request directly to pypa/auditwheel but it must be merged only after this. Which way would be better? @ehashman @lpsinger

@lpsinger
Copy link
Contributor Author

@daa you can actually make a pull request relative to my branch.

@daa
Copy link
Contributor

daa commented Dec 13, 2018

@lpsinger @ehashman please take a look at test when you'll have time for it.

@lpsinger
Copy link
Contributor Author

@lpsinger @ehashman please take a look at test when you'll have time for it.

Will do. I'm a little slammed right now but I should get to it by the weekend. Thank you!

@daa
Copy link
Contributor

daa commented Dec 19, 2018

I haven't noticed at the time that lib/ is ignored and thus required test files were not added to git, will fix it shortly. Sorry.
New pull request with fix: https://github.com/lpsinger/auditwheel/pull/2 please take a look when you'll have some time.

@daa
Copy link
Contributor

daa commented Dec 19, 2018

@ehashman
Copy link
Member

This is a pretty big semantic change so I'm going to hold off until after the 2.0 release to review.

@ehashman ehashman dismissed their stale review January 23, 2019 23:56

Tests have been added

lpsinger added a commit to lpsinger/delocate that referenced this pull request Feb 7, 2019
If you have a package that contains multiple C extensions that
depend on the same shared libraries, then `delocate-wheel` saves
duplicate copies of all of the shared libraries.

This makes for packages that are several times larger than
necessary. This patch causes `delocate-wheel` to sweep up all
of the dependencies and put them in a single directory, rather
than creating duplicates.

The convention is based on the proposal in pypa/auditwheel#89
and pypa/auditwheel#90.
lpsinger added a commit to lpsinger/delocate that referenced this pull request Feb 7, 2019
If you have a package that contains multiple C extensions that
depend on the same shared libraries, then `delocate-wheel` saves
duplicate copies of all of the shared libraries.

This makes for packages that are several times larger than
necessary. This patch causes `delocate-wheel` to sweep up all
of the dependencies and put them in a single directory, rather
than creating duplicates.

The convention is based on the proposal in pypa/auditwheel#89
and pypa/auditwheel#90.
lpsinger added a commit to lpsinger/delocate that referenced this pull request Feb 25, 2019
If you have a package that contains multiple C extensions that
depend on the same shared libraries, then `delocate-wheel` saves
duplicate copies of all of the shared libraries.

This makes for packages that are several times larger than
necessary. This patch causes `delocate-wheel` to sweep up all
of the dependencies and put them in a single directory, rather
than creating duplicates.

The convention is based on the proposal in pypa/auditwheel#89
and pypa/auditwheel#90.
@codecov
Copy link

codecov bot commented Jul 26, 2019

Codecov Report

Merging #90 into master will decrease coverage by 0.12%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   87.32%   87.19%   -0.13%     
==========================================
  Files          19       19              
  Lines         978      976       -2     
  Branches      215      214       -1     
==========================================
- Hits          854      851       -3     
  Misses         86       86              
- Partials       38       39       +1
Impacted Files Coverage Δ
auditwheel/repair.py 83.52% <75%> (-1.53%) ⬇️

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 0500098...4d9e3df. Read the comment docs.

@lpsinger
Copy link
Contributor Author

I've rebased the patch and it's passing the tests.

auditwheel/repair.py Outdated Show resolved Hide resolved
@lpsinger
Copy link
Contributor Author

I've rebased this PR again. Is there any further feedback, or is this ready to be merged?

@auvipy
Copy link
Contributor

auvipy commented Jan 21, 2020

can you check te builds?

@lpsinger
Copy link
Contributor Author

can you check te builds?

Yes, one of the jobs failed here:

INFO     test_manylinux:test_manylinux.py:108 docker exec 06c4e504c87a: 'yum install -y atlas atlas-devel'
278Loaded plugins: fastestmirror, ovl
279Determining fastest mirrors
280https://download.sinenomine.net/clefos/7/sclo/s390x/rh/repodata/repomd.xml: [Errno 14] HTTPS Error 404 - Not Found
281Trying other mirror.
282

Would you please click retry in travis?

@lkollar
Copy link
Contributor

lkollar commented Jan 21, 2020

I restarted the build but now it's failing because of another issue:

auditwheel repair: error: argument --plat: invalid choice: 'manylinux1' (choose from 'linux_x86_64', 'manylinux1_x86_64', 'manylinux2010_x86_64', 'manylinux2014_x86_64')

For some reason it's passing the wrong platform name.

@lpsinger
Copy link
Contributor Author

Oh, weird. They're all failing now, too.

@lpsinger
Copy link
Contributor Author

@lkollar
Copy link
Contributor

lkollar commented Jan 22, 2020

Looks like the pip 20 release is causing the failures because it deprecates tags like cp3-none-any. Opened #219 to fix it.

@lpsinger
Copy link
Contributor Author

That worked. Builds are passing now. @auvipy, @lkollar, any more feedback?

@lkollar lkollar merged commit f0b52b3 into pypa:master Jan 23, 2020
@lkollar
Copy link
Contributor

lkollar commented Jan 23, 2020

Thanks @lpsinger!

@lpsinger lpsinger deleted the fix-duplicate-libs branch January 23, 2020 22:28
@lpsinger
Copy link
Contributor Author

Thanks @lpsinger!

Well, thank you! It'll be great to have this in a release so that we don't have to use my fork any more for building lalsuite.

@daa
Copy link
Contributor

daa commented Jan 24, 2020

Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants