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

Allow for delocating top level modules. #123

Merged
merged 11 commits into from
Sep 21, 2021

Conversation

HexDecimal
Copy link
Collaborator

@HexDecimal HexDecimal commented Sep 18, 2021

This delocates libraries into one directory per wheel. I've added a top-level package-less wheel to test with.

Prevents copying duplicate libraries when a wheel has multiple packages. Fixes #35.

Fixes issues where the module to work on is at the top-level, such as a wheel with no packages. Generally any current issue where delocate mysteriously doesn't bundle libraries has been fixed. Fixes #72, fixes #22, fixes #45, fixes #63, fixes #121, fixes #66, fixes #49, fixes #67.

See _decide_dylib_bundle_directory for how the folder to use is determined. I've followed the advice from #72 to use an auditwheel-style name. Wheels without packages use the folder {package_name}.dylibs, wheels with packages will put a .dylibs folder in only one of the packages preferring the one that matches the wheel name. The one directory will hold the dependencies of the entire wheel.

After that the fix was simple: In delocate_wheel call delocate_path only once and call it with a tree_path that includes the entire wheel.

Since this always collects everything regardless of if a package is found or not. This will also resolve issues where namespace packages couldn't be delocated. Fixes #95.

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2021

Codecov Report

Merging #123 (bea78a7) into master (9038521) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
+ Coverage   94.76%   94.88%   +0.11%     
==========================================
  Files          13       13              
  Lines         994      997       +3     
==========================================
+ Hits          942      946       +4     
+ Misses         52       51       -1     
Impacted Files Coverage Δ
delocate/delocating.py 97.60% <100.00%> (+0.50%) ⬆️
delocate/tools.py 93.98% <100.00%> (+0.02%) ⬆️

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 9038521...bea78a7. Read the comment docs.

Add a top-level package-less wheel to test.

Fixes duplicate library issues when a wheel has multiple packages.

Fixes issues where a wheel has no packages.
Copy link
Owner

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

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

Fantastic - thanks! Just a few suggestions. Are you testing the case for more than one package in wheel?

delocate/delocating.py Show resolved Hide resolved
delocate/delocating.py Show resolved Hide resolved
delocate/delocating.py Outdated Show resolved Hide resolved
delocate/delocating.py Outdated Show resolved Hide resolved
delocate/delocating.py Outdated Show resolved Hide resolved
delocate/delocating.py Outdated Show resolved Hide resolved
delocate/delocating.py Outdated Show resolved Hide resolved
delocate/tests/test_wheelies.py Outdated Show resolved Hide resolved
@matthew-brett
Copy link
Owner

How about a test for namespace packages as well? (Sorry for the stream of requests).

@HexDecimal
Copy link
Collaborator Author

Are you testing the case for more than one package in wheel?
How about a test for namespace packages as well?

Only the current wheels are tested so more wheels either need to be added or the current need to be modified to be more diverse. There are no wheels using namespace packages, or wheels with more than one package.

Namespace packages are currently treated the same as wheels with no packages. I see this as acceptable since wheel names shouldn't collide.

While there are no wheels with multiple packages, both the conditions of selecting a package by the wheel name and selecting a package alphabetically have been covered by tests.

Move _make_install_name_ids_unique outside of the function.

Remove all_copied since this function no longer has a loop.
@HexDecimal
Copy link
Collaborator Author

I've added a namespace wheel with a typical test for it.

@HexDecimal HexDecimal force-pushed the toplevel branch 2 times, most recently from a7a4f22 to 317b3d9 Compare September 20, 2021 21:50
Copy link
Owner

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

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

Just a final few comments.

delocate/delocating.py Outdated Show resolved Hide resolved
delocate/delocating.py Outdated Show resolved Hide resolved
HexDecimal and others added 2 commits September 20, 2021 16:08
This parameter is no longer used by its function and may be removed in
the future.
@matthew-brett
Copy link
Owner

Thanks very much for this - it's a big step forward.

@HexDecimal HexDecimal deleted the toplevel branch September 21, 2021 13:12
@HexDecimal
Copy link
Collaborator Author

HexDecimal commented Sep 21, 2021

I'd like to make a release of a version with this merged. make_release.rst mentions making announcements to a mailing list, but I don't see the mailing list this is referring to. If you ever want to make a release then all you have to do right now is push a tag with the version (after following the release guide of course.)

We should also look at the other PR's which were trying to implement this feature: #36 #38 #39 #74 can probably be closed without merging. #96 might improve the handling of namespace packages but I don't think the author will care much now that namespace packages decloate correctly.

@matthew-brett
Copy link
Owner

matthew-brett commented Sep 21, 2021 via email

@HexDecimal
Copy link
Collaborator Author

I'm not sure about a 1.0 release yet. Semantic versioning gives more flexibility for public API changes before a 1.0.0 release and I'm not sure the API is settled yet.

@matthew-brett
Copy link
Owner

I have personally found little use for applying semantic versioning with any rigor. Basically, it seems to me that we always want to preserve the API, or migrate sensibly, and a version number change will do little to effectively warn users that a breaking API change is on the way. But if you'd rather play it that way, that's OK too.

@HexDecimal
Copy link
Collaborator Author

I'm not sure. It's probably not as big of a deal to have a stable programming API for a primarily CLI tool, but my current suggestion is to go with 0.10.0 as the next version.

@matthew-brett
Copy link
Owner

That's fine too - it's really not a big deal - I don't think people pay much attention to version numbers anyway.

@HexDecimal
Copy link
Collaborator Author

Semantic versioning just sets the rules for why versions are what they are. If you released 1.0.0 and then break the API right away then the next version will be 2.0.0 and it isn't really a big deal for that to happen as long as the versions are set correctly.

1.0.0 is used to express confidence that the current API is stable and that more care will be taken to keep compatibility with it. I wouldn't use it to mean anything else. With how careful you've been treating breaking cases you probably could've been using 1.0.0 a long time ago.

Personally, there are things I'd like to do or braking changes I'd consider making before 1.0.0. I'd rather not release that version before everything is type-hinted for example.

@asottile
Copy link
Contributor

can confirm this works -- thanks for picking up this patch!

dhermes added a commit to dhermes/bezier that referenced this pull request Oct 1, 2021
ipmb added a commit to lincolnloop/pyuwsgi-wheels that referenced this pull request Nov 11, 2021
It looks like the missing functionality was merged matthew-brett/delocate#123
ipmb added a commit to lincolnloop/pyuwsgi-wheels that referenced this pull request Nov 11, 2021
It looks like the missing functionality was merged matthew-brett/delocate#123
ipmb added a commit to lincolnloop/pyuwsgi-wheels that referenced this pull request Nov 11, 2021
It looks like the missing functionality was merged matthew-brett/delocate#123
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment