-
Notifications
You must be signed in to change notification settings - Fork 142
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
Rename libraries on graft, fixes #24 #25
Conversation
Note it requires patchelf 0.9, which our code should probably check for (calling patchelf --version), but doesn't currently. |
I just tested it and got the following error:
It seems to me that
The version of patchelf available on the centos 5.11 based manylinux docker image is probably too old. |
Also, I think @njsmith suggested |
I had not read @rmcgibbo's #25 (comment) that this requires patchelf 0.9... It's a pain to build under centos 5.11 though:
|
Ok I managed to build patchelf 0.9 and it works. I also checked that the original problem with f2py and libgfortran as reported by @matthew-brett on the wheel-builders mailing list is fixed. I will try to do a PR to build patchelf 0.9 in the docker images. |
Actually the release tarball from nixos.org is already bootstrapped (hence easy to build on the old centos) contrary to the release tarball fetched from github.com. |
+1 for merge on my side once we agree on the desired form for the vendored lib filenames as pointed in #25 (comment). |
There is something fishy in the output of
The repairing process seems correct but the show command still thinks that the wheel has dependencies on system BLAS and fortran:
I checked manually by unzipping the wheel and indeed there is a problem: the compiled extensions link to both the old system libs and the new vendored libs at the same time:
|
Oh, right. I know the issue. Thanks for testing. There's still a couple more calls to Also, I'm 99% sure soname format doesn't matter. |
shorthash = hashfile(f)[:8] | ||
|
||
old_soname = elf_read_soname(src_path) | ||
new_soname = '%s.%s' % (os.path.split(src_path)[1], shorthash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following can implement @njsmith proposed naming scheme:
base, ext = os.path.basename(src_path).split('.', 1)
new_soname = '%s-%s.%s' % (base, shorthash, ext)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could even make the naming scheme idempotent so that calling repair twice on a manylinux wheel would not change it. For instance (untested):
src_name = os.path.basename(src_path)
base, ext = src_name.split('.', 1)
if not base.endswith('-%s' % shorthash):
new_soname = '%s-%s.%s' % (base, shorthash, ext)
else:
new_soname = src_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't repair just be a noop if the lib is already tagged manylinux_1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this already the case? If so I am fine with that.
Okay, @ogrisel, I think this PR is doing the right thing now.
|
|
m = re.match('patchelf\s+(\d+(.\d+)?)', version) | ||
if m: | ||
if float(m.group(1)) >= 0.9: | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can never return anything else, right? I that case I think a simple return
(without True
) would be enough.
Sure. I'll make those changes. Did this work on the numpy test case you were using before though? |
(it did for me, but I'm not sure if my numpy setup was set up exactly the same as yours and @matthew-brett's, because I haven't been in the wheel-builder loop so much for the past month or so.) |
All chaos broke loose for me. Build generated here and uploaded to here using the last commit on this PR. Install on an actual machine (CentOS 7):
More informative message (maybe) on buildbot on Debian sid:
|
Oh no...... Sent from my iPhone
|
Oh god damn it. This is a bug in patchelf: it turns out that when you link against a library like
Notice that the first part references |
Well that's potentially not that hard to fix then. I was thinking the diagnosis would be the hardest part. Sent from my iPhone
|
In the mean time I am working on a docker-based integration test for auditwheel (just to let you know). |
380e5fd
to
7ac8994
Compare
Sure. Rebased. |
@njsmith I took a quick look at the code in your patchelf PR as compared to pyelftools. I think it's still missing jumping into some "aux" table, if you look at the implementation of the methods required for
when running the master branch of patchelf with --debug against _ufuncs.so, it doesn't properly find the filenames (https://github.com/njsmith/patchelf/blob/907c02009e9f4e6ad122fa940fdb59572fddfc87/src/patchelf.cc#L1337)
Note here that libc.so.6 shows up 3 times, which isn't right. |
@rmcgibbo: oh thanks! Looking at elftools I think I see the problem: I was interpreting the |
No problem. The aux table was just a guess but pyelftools is probably useful to debug/document Sent from my iPhone
|
@rmcgibbo here is the PR to your PR: rmcgibbo#1 |
Improvements to integration tests
@matthew-brett: https://github.com/njsmith/patchelf master should now work correctly for scipy, if you'd like to test (cf NixOS/patchelf#86) |
Builds up at usual URL : http://ccdd0ebb5a931e58c7c5-aae005c4999d7244ac63632f8b80e089.r77.cf2.rackcdn.com/ Numpy passes tests on local machine, scipy does not crash on import, just set full test runs going at: |
Looks good. Would it be possible to publish a bootstrapped tarball for patchelf to integrate it in the manylinux1 docker image while waiting for the patchelf 0.9.1 or 0.10 release? |
Thanks! |
Hum apparently there are undefined symbols in https://travis-ci.org/matthew-brett/manylinux-testing/jobs/119960887#L803 |
Ah no - sorry - I am also running tests there against ATLAS - which we know is broken - so you can ignore that one - look only at the openblas tests. Elsewhere, all looking good so far: |
Looks like @matthew-brett's tests passed \o/ In any case I think this auditwheel PR can be merged? And then I guess we'll need an auditwheel release, and then a rebuild of the docker images with the new auditwheel + the new patchelf? |
All tests on buildbot passing (Python 2.7 on Fedora 19, Pythons 2.7 3.4 3.5 on Debian Sid). manylinux-builds all passed on Python 2.7, a couple on 3.4, the rest pending, but it would be surprising if any of those failed. Very nicely done, congratulations all. |
One manylinux-testing test timed out, but nearly done and with no errors. |
Rebuilding the docker images to pick up auditwheel 1.3.0 should complete the changes needed to fix the problems @matthew-brett found with vendoring libgfortran (see: pypa/auditwheel#24, pypa/auditwheel#25, pypa#43, pypa#44, etc.).
Okay, I think this works. I didn't get to check it with the numpy/f2py example yet though, and I have to go to bed now.