Skip to content
This repository has been archived by the owner on Feb 15, 2023. It is now read-only.

Update multibuild to a version that strips binaries in wheels #25

Closed
xhochy opened this issue Feb 11, 2018 · 20 comments
Closed

Update multibuild to a version that strips binaries in wheels #25

xhochy opened this issue Feb 11, 2018 · 20 comments

Comments

@xhochy
Copy link
Contributor

xhochy commented Feb 11, 2018

On the next release, please update the multibuild submodule to matthew-brett/multibuild@b2748e5 or newer. This should reduce the size of the scipy wheels from 44M to 30M by excluding unnecessary debug symbols. Also this updates to a newer version of auditwheel and patchelf that produce wheels with binaries that are actually strippable, i.e. fixes things like pypa/manylinux#119

To make this work for scipy you need to export FFLAGS="$FFLAGS -fPIC -Wl,-strip-all" as the first line in function build_wheel in config.sh. For a successful Travis build see https://travis-ci.org/xhochy/scipy-wheels/builds/337663919

@pv
Copy link
Contributor

pv commented Feb 11, 2018

Do you want to make a PR against the master branch of this repository, if you have it already tested?

@xhochy
Copy link
Contributor Author

xhochy commented Feb 11, 2018

@pv Yes, I can do that. I just refrained from that as these repositories are not ones that are typically PR'ed to but I'll do that now.

@tkelman
Copy link

tkelman commented Feb 20, 2018

Also this updates to a newer version of auditwheel and patchelf that produce wheels with binaries that are actually strippable, i.e. fixes things like pypa/manylinux#119

What changed in auditwheel and/or patchelf to fix that? When I did something roughly equivalent to this myself, I found trying to strip the binaries another time after auditwheel was run on them does still break them.

@xhochy
Copy link
Contributor Author

xhochy commented Feb 20, 2018

NixOS/patchelf#117 should be the commit to auditwheel that fixes the stripping problem. It is now included in the latest manylinux1 docker image.

@tkelman
Copy link

tkelman commented Feb 20, 2018

That looks like it was well over a month ago when I was last able to reproduce this. Do you have test wheels somewhere that you built with latest everything? I can try those out and see if calling strip on them again results in binaries that fail to import.

@pv
Copy link
Contributor

pv commented Feb 20, 2018 via email

@tkelman
Copy link

tkelman commented Feb 20, 2018

Ah perfect. So no, pypa/manylinux#119 is not fixed AFAICT. This was in a docker container of amazonlinux:latest but shouldn't be specific to that distro, I don't think (edit: same basic thing reproduces in debian:testing with py36):

$ yum install -y python27-pip binutils findutils
...
$ pip install https://7933911d6844c6c53a7d-47bd50c35cd79bd838daf386af554a83.ssl.cf2.rackcdn.com/scipy-1.1.0.dev0+20180217170714_d8a2deb-cp27-cp27mu-manylinux1_x86_64.whl#sha256=9a94da28d0868a3fa608ab7ae4a0d62b6cf6c2025baee1df6417601710a39fda
...
$ python -c 'from scipy.special import logit'
$ find /usr/local/lib64/python2.7/site-packages/scipy -name "*.so" | xargs strip
$ python -c 'from scipy.special import logit'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/lib64/python2.7/site-packages/scipy/special/__init__.py", line 639, in <module>
    from ._ufuncs import *
ImportError: /usr/local/lib64/python2.7/site-packages/scipy/special/_ufuncs.so:
ELF load command address/offset not properly aligned

@pv pv reopened this Feb 20, 2018
@tkelman
Copy link

tkelman commented Feb 21, 2018

Well this issue is fixed. It removes the need to strip after downloading because it does it for you as part of the wheel-building process. If you happen to try to strip after downloading, it'll still break things - but that's probably the patchelf fix being incomplete. Maybe something like NixOS/patchelf#127 is worth testing, though that would require building a test copy of the manylinux docker image with extra patches applied to patchelf.

@pv
Copy link
Contributor

pv commented Feb 21, 2018

Yes #26 and https://github.com/matthew-brett/multibuild/pull/140 added flags to strip binaries on build, but I guess for auditwheel we have whatever is in the current pypa manylinux docker image.

@pv pv closed this as completed Feb 21, 2018
@charris
Copy link
Contributor

charris commented Feb 21, 2018

The NumPy 1.14.1 release wheels are now up on PyPI and built with the latest recommended multibuild if someone wants to take a look.

@charris
Copy link
Contributor

charris commented Feb 21, 2018

The manylinux1 wheels for 1.14.1 are about 4MB smaller than the 1.14.0 release, so something has been stripped.

@tkelman
Copy link

tkelman commented Feb 21, 2018

I was able to get pypa/manylinux#119 to happen on scipy or h5py post-stripping, but never with numpy IIRC.

@tkelman
Copy link

tkelman commented Mar 27, 2018

It doesn't look like the 1.0.1 linux wheels were stripped. How can we ensure https://github.com/matthew-brett/multibuild/pull/140 is included when the next scipy release wheels get built?

@pv
Copy link
Contributor

pv commented Mar 27, 2018

It may have been is intentional that bugfix releases don't change the build config unless needed, and changes are manually merged.

It may have been intentional to ignore this particular change --- I remembered it, but did not bring it up.

Nothing needs to be done to have it included in the next major release.

@matthew-brett
Copy link
Contributor

I think this happened with #27

@tkelman
Copy link

tkelman commented Mar 27, 2018

The manylinux wheels for https://pypi.python.org/pypi/scipy/1.0.1#downloads look to be the same size as https://pypi.python.org/pypi/scipy/1.0.0#downloads, so not yet. If it'll be in 1.1.0, and that's somewhere less than a year away, guess that's good enough for now.

@pv
Copy link
Contributor

pv commented Mar 27, 2018

@matthew-brett: the 1.0.x releases come from the v1.0.x branch, stripping debug symbols is only on the master branch. Bugfix builds use the same build config as the initial release (in order to avoid issues such as those seen with numpy). Granted, this does not guarantee fully reproducible environment, as configuration may change on the Travis-CI side as seen in #27, but at least is a bit more controlled.

@matthew-brett
Copy link
Contributor

Yes, sure, I should have been more specific what I meant. In #27 we see Ralf tried updating to multibuild master in the 1.0.x branch, but it broke because Travis-CI was coincidentally not working - and then you took us back to an earlier multibuild version, that was working for the daily builds, but did not include the commits implementing stripping.

In general it should be safe to upgrade multibuild, but it will do things like updating default library versions. Not that that applies to Scipy.

@pv
Copy link
Contributor

pv commented Apr 29, 2018

It seems the wheel for 1.1.x, which does include the multibuild changes, are still not really stripped:

$ wget wget https://files.pythonhosted.org/packages/9c/0b/5deb712a9ea5bb0a1de837d04ef7625c5f3ee44efe7ed0765ceda681d7f1/scipy-1.0.1-cp27-cp27mu-manylinux1_x86_64.whl
$ unzip scipy-1.0.1-cp27-cp27mu-manylinux1_x86_64.whl
$ cd scipy/sparse
$ du -csh _sparsetools.so 
39M	_sparsetools.so
39M	yhteensä
$ strip _sparsetools.so 
$ du -csh _sparsetools.so 
3,7M	_sparsetools.so
3,7M	yhteensä

Is this because the -Wl,-strip-all flag has no effect, or because the flag did not in the end get passed to the compiler? Or maybe -Wl,-strip-all strips some, but not all, debug infos?

The multibuild build log is not shown by travis, so I don't know which one it is.

@pv pv reopened this Apr 29, 2018
@pv
Copy link
Contributor

pv commented Apr 29, 2018

Sorry for the noise, I now see I was looking at 1.0.1 not 1.1.0 files...
The 1.1.0 ones are of course stripped properly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants