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 to repair other shared libraries bundled in a wheel #28

Closed
wants to merge 5 commits into from

Conversation

anlambert
Copy link
Contributor

First, thanks for the great work on the PEP 513 and the auditwheel tool. Being able to upload binary wheels for Linux platforms was a real missing feature.

I work on an opensource project called Tulip (http://www.tulip-software.org) mainly dedicated to the visualization of large graphs. The project is written in C++ but we also offered Python bindings to our core libraries (https://pypi.python.org/pypi/tulip-python). I already managed to distribute wheels of our modules for Windows and MacOS, so when I heard about the PEP 513, I gave it a try.

So I used the Centos5 docker images to compile our project and its Python bindings. When trying to use the auditwheel tool to repair the wheels we produce, I came into some small issues.

I use a different method to generate the binary wheels through the 'setup.py bdist_wheel' command.
I compile Tulip and its Python bindings through the use of CMake, put all the files required to distribute the modules (Python extension modules + shared libraries related to our project) in a directory, and then use 'setup.py bdist_wheel' command to bundle the whole in a .whl archive. As I need the wheels to get the correct binary tag name (e.g. cp27-cp27m-linux-x86_64) and I don't compile the extension modules trough 'setup.py', I am forced to override the distclass in the setup function (see https://github.com/tulip5/tulip/blob/master/library/tulip-python/bindings/tulip-core/packaging/setup.py.in). Proceeding that way, bundled modules are not located in the root folder of the wheel but in the purelib folder inside the modules data folder (check the wheels I uploaded on PyPi). Consequently, the auditwheel tool fails to find the module root folder. I added some checks in 'repair.py' to handle that special case.

I also distribute other shared libraries in the wheels for the Tulip modules (tulip libraries + some its plugins). The auditwheel tool don't take into account shared libraries bundled into a wheel different from Python extension modules in its repairing tasks. But they must also be repaired in order to be distributed. I slightly modify the 'get_wheel_elfdata' function in wheel_abi.py to also allow those libraries to be fixed through the patchelf tool.

Besides those small issues, the produced wheels works great on every Linux distributions I have tested so far.

@njsmith
Copy link
Member

njsmith commented Apr 15, 2016

I'm a little wary about this idea of properly supporting compiled extension modules in purelib, since the definition of purelib is "like platlib, for packages that don't contain any compiled extension modules":-). ("purelib" = for pure Python, "platlib" = for _plat_form specific compiled code). Possibly it would even make sense for auditwheel to complain if it sees .so files inside purelib...

I do get that practicality beats purity, but how hard would it be to fix your bdist_wheel hack to make compliant platlib-using wheels?

@njsmith
Copy link
Member

njsmith commented Apr 15, 2016

Note also as a general warning -- auditwheel certainly should support the case where there are "hand-bundled" libraries that it didn't put there, but we've found that bundling libraries correctly is rather complex -- e.g. if your tulip libraries could ever find themselves in a situation where there are two different packages linking to them inside the same Python process, and one is using the bundled libraries and one is using an external system version of the tulip libraries, then all kinds of bad things can happen. auditwheel takes countermeasures to make sure that this case works -- you might want to consider checking its techniques, or even just letting auditwheel start taking care of the bundling.

(Sadly auditwheel is currently Linux-only, I guess might be part of the issue. But we really should fix that anyway, since you need exactly the same things on OSX and Windows and they are also rather non-trivial to get right :-).)

@anlambert
Copy link
Contributor Author

Thanks for the clarification between purelib and platlib, it was quite hard to understand it by reading the PEP about binary wheel and the pip documentation. Anyway, I knew that I was doing something wrong but it was more convenient for me to proceed that way.

I think I can manage to make compliant platlib wheels for the tulip modules. Compiling the extension modules trough the 'distutils.extension.Extension' class seems feasible even if the configuration is quite complex (hopefully, CMake will help me to gather required include directories and libraries to link against).

Regarding shared libraries related to tulip bundled in the current wheels (and you're right, bundling those correctly is rather complex ;-)), I will try to link the extension modules against static version of them to avoid the possible issues you describe above.

Thanks again for the clear explanations. I think that PR can be closed now.

@njsmith
Copy link
Member

njsmith commented Apr 15, 2016

If all you're doing BTW is setting up your own file layout and trying to use distutils to bundle it into a wheel, then instead of wasting time trying to trick distutils into doing the right thing then you might find flit easier to work with -- it's a tool specialized for that. I guess it probably doesn't have an option to enable platlib since normally it's used only for pure python modules, but it would be presumably be trivial to just add this as a switch. (Actually looking at your setup.py I don't even understand why your hacked Distribution class doesn't already work.)

Cc @takluyver

Before closing this, can you check whether this part of your pr is still relevant? Even if you're switching to static libs we should still not fail on shared libraries:

The auditwheel tool don't take into account shared libraries bundled into a wheel different from Python extension modules in its repairing tasks. But they must also be repaired in order to be distributed. I slightly modify the 'get_wheel_elfdata' function in wheel_abi.py to also allow those libraries to be fixed through the patchelf tool.

@anlambert
Copy link
Contributor Author

A quick follow up to that PR. I investigated a bit further the reason why the wheel content was installed in install_purelib instead of install_platlib in my case. This was obviously due to the fact that I did not declare any extension module through the 'ext_modules' parameter of the 'setup' function from setuptools. Nevertheless I found a workaround to force the wheel content to be platlib compliant.
I simply add the following lines in setup.cfg:

[install]
install_lib=

By overriding install_lib, wheel files are no more put in the purelib folder and so the auditwheel tool is able to repair the extension modules.

I also investigated if I could switch to static libs instead of bundling shared libraries related to Tulip but it turns out it will be too painful to put in place even if it should be feasible (but a CMake nightmare). So I will stick to the way I proceed actually as it works pretty well. Allowing to repair other shared libraries with the auditwheel will be welcome but I don't want to impose my particular case, I can continue to work with my slightly patched version.

@ogrisel
Copy link
Contributor

ogrisel commented Apr 20, 2016

Thanks for the follow up. We should definitely have auditwheel show and repair respectively diagnose and automatically fix this purelib / platlib info. This is now tracked by #32.

I think we can close this PR.

@ogrisel ogrisel closed this Apr 20, 2016
@njsmith
Copy link
Member

njsmith commented Apr 20, 2016

@ogrisel: I think the other half of this PR, which is to apply rpath/vendor fixups to hand-vendored libraries, is still fixing a legitimate bug? (Re-opening so as not to lose track of this question, feel free to reclose if the answer is "no" :-))

@njsmith njsmith reopened this Apr 20, 2016
@ogrisel
Copy link
Contributor

ogrisel commented Apr 20, 2016

Indeed I had not understood that point. Is there a specific issue for that?

@njsmith
Copy link
Member

njsmith commented Apr 20, 2016

It looks like there's already a reasonable patch here, it's just a bit mixed up with the platlib/purelib stuff.

@anlambert: would you be willing to fix up this PR so that it doesn't try to fix purelib code (or even better, if it's a purelib then check for binaries and issue a warning/error if they're found), but keeping the other fixes?

@anlambert anlambert changed the title fix repairing of wheels where root is not purelib check invalid binary wheel + allow to repair other shared libraries bundled in a wheel Apr 21, 2016
@anlambert
Copy link
Contributor Author

anlambert commented Apr 21, 2016

I have just updated the PR (and its title) in order to check if the input binary wheel is valid (i.e. no shared libraries in purelib). An error will be raised if it is the case. I added the check in the 'get_wheel_elfdata' function in order for the error to be raised with all auditwheel commands.

@ogrisel
Copy link
Contributor

ogrisel commented Apr 21, 2016

I don't understand the travis failure. It seems unrelated to this branch but I cannot reproduce it on the current master. Could you please rebase your branch on top of the current master and force push to github to update this PR?

@ogrisel
Copy link
Contributor

ogrisel commented Apr 21, 2016

Actually please ignore my last comment, I made a mistake when trying to replicate the travis problem.

@ogrisel
Copy link
Contributor

ogrisel commented Apr 22, 2016

I don't get the same errors when I run the tests locally when I rebased it on master. Have you tried to run the tests yourself?

@ogrisel
Copy link
Contributor

ogrisel commented Apr 22, 2016

I tried in #34 and still get the same travis failure. This is weird because I cannot reproduce this locally.

@ogrisel
Copy link
Contributor

ogrisel commented Apr 22, 2016

Actually I was confused: I get the same errors on my local machine, I just did not run the tests in the same order as travis. Sorry for the noise.

if 'purelib' in so_path_split:
raise RuntimeError(('Invalid binary wheel, found shared library "%s" in purelib folder.\n'
'The wheel has to be platlib compliant in order to be repaired by auditwheel.') %
so_path_split[-1])
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue #32 should be addressed in a separate PR. Let's focus this PR on handling pre-bundled shared libraries properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will open a new one adressing #32 and update this one

@anlambert
Copy link
Contributor Author

I just ran the tests locally : I can confirm they pass on master branch but not with that PR, so there is definitely a regression induced by it. Looks like grafted libraries for numpy can not be located anymore, surely a side effect on rpaths.

@anlambert anlambert changed the title check invalid binary wheel + allow to repair other shared libraries bundled in a wheel allow to repair other shared libraries bundled in a wheel Apr 22, 2016
@anlambert
Copy link
Contributor Author

I worked on the test failure that afternoon and I think I manage to fix it. I committed my progress on it but it still needs some improvements and clear explanations. I have to leave my office now and I won't be able to work on it that week end, so I will be back on it starting monday.

…gh CMake, containing multiple extension modules and other shared libraries
@ehashman
Copy link
Member

Hi @anlambert,

Thanks for your hard work on this PR. Since there's a merge conflict and this has been stale for a while, I'm going to close this PR for now. My apologies that the maintainers didn't get a chance to review your fixes in a timely manner. Feel free to reopen this in the future if you get a chance to fix the merge conflicts and the CI failures!

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.

None yet

4 participants