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

Check compatibility after repairing wheel #148

Closed
wants to merge 3 commits into from

Conversation

takluyver
Copy link
Member

Partially addresses #146.

This checks the output wheel from auditwheel repair for compatibility with the specified tag. This is not an ideal fix, because the output wheel is produced even if it's invalid. But producing an error means that it will fail in testing & CI environments, which probably mitigates the problem in many situations.

A better fix would involve either checking the libraries as they are grafted in, or producing the wheel initially into a temporary folder, and only copying it to the output directory if it passes the check.

This builds on the branch in #147.

@takluyver
Copy link
Member Author

The test failure on 3.5 might be related to something I just fixed in my other PR:
46619cd

I think the test with numpy is currently sensitive to order - it will succeed if it runs with manylinux1 first, and fail if it runs with manylinux2010 first. Dictionary ordering is deterministic from Python 3.6, but random in 3.5.

@ehashman
Copy link
Member

Please rebase this PR.

@takluyver
Copy link
Member Author

Rebased; thanks for merging my other PR. 🙂

auvipy
auvipy previously approved these changes Apr 12, 2019
Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

Build is green! thanks

@ehashman
Copy link
Member

Looks good @takluyver; is it possible to add a test case to verify this?

@takluyver
Copy link
Member Author

takluyver commented Apr 15, 2019

I've just been trying to write a test for it, but unfortunately I can't find a good way to do it.

I ran into this with wheels of pygame, but pygame has a number of extra dependencies which need to be built before it, which don't seem reasonable to deal with as part of the test process here. Pygame actually has its own build images adding these dependencies to the standard PyPA images, but there's no published manylinux2010 version yet.

The other packages I've tried to build to test this either fail the check before repairing (i.e. their compiled libraries already need newer symbols than manylinux1) or they don't need newer symbols at all, so they really are eligible for a manylinux1 tag. It seems I hit a fairly specific corner case.

I've tried so far with numpy, pygame, tornado and pyzmq.

@ehashman
Copy link
Member

We could make a minimal test case that does exactly what the error message says: have a simple wheel depend on a prebuilt shared object that has more recent symbols than the platform tag? I don't know if it's worth the marginal effort but I'm always worried about preventing regressions :)

@takluyver
Copy link
Member Author

I certainly understand the desire for decent tests to avoid regressions. But I don't really know this domain well enough to put together a minimal test case: I don't know how I'd make an extension module only using the older symbols, a separate library using newer symbols, and get them appropriately linked so auditwheel bundles the library in.

@ehashman
Copy link
Member

If you are interested in learning how to do this, this test is a good example:

def test_build_wheel_with_binary_executable(docker_container):
# Test building a wheel that contains a binary executable (e.g., a program)

The source code for the test package lives here: https://github.com/pypa/auditwheel/tree/b0fde3d6a365aa3f942c9e04ef5bca9014c788ed/tests/testpackage

To get a library with newer symbols, you could just commit a known policy-breaking .so in the git tree and depend on it (rather than building the C code in the manylinux image).

Anyways, if that's too much work, I can look into merging this as is.

@takluyver
Copy link
Member Author

Thanks! I'll try to look into this, but I can't guarantee when I'll have time.

@mayeut
Copy link
Member

mayeut commented Apr 16, 2019

@takluyver,
I worked on a test case for the best guess behavior in #146
With dependencies on manylinux2010 symbols in the "parent"/"main" module, it's rejected when trying to repair it targeting manylinux1.
With dependencies on manylinux2010 symbols only in the "child" module, it's not rejected when trying to repair it targeting manylinux1.
You can cherry-pick the commit in my branch in order to add a test in this PR if you find it to fit your needs: mayeut@b3d1ea6
You can see the corresponding failed test here: https://travis-ci.com/mayeut/auditwheel/builds/108599087

@auvipy auvipy dismissed their stale review May 7, 2019 18:48

I want to take another look on the recent changes

@takluyver
Copy link
Member Author

This was superseded by #150.

@takluyver takluyver closed this May 29, 2019
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.

4 participants