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

manylinux1: Install libxcrypt #324

Merged
merged 1 commit into from
Jul 22, 2019
Merged

Conversation

lkollar
Copy link
Contributor

@lkollar lkollar commented Jul 19, 2019

This will make libcrypt.so.2 available in the image. The GLIBC variants of the header and libraries are deleted in order to force applications to pick up the libxcrypt version.

See #305.

@zackw
Copy link

zackw commented Jul 19, 2019

The new binary file named docker/build_scripts/v4.4.6 appears to be a tarball of the libxcrypt source code. Should this file really be checked into version control?

@lkollar
Copy link
Contributor Author

lkollar commented Jul 19, 2019

Good catch. Removed the v4.4.6 file.

This will make `libcrypt.so.2` available in the image. The GLIBC
variants of the header and libraries are deleted in order to force
applications to pick up the `libxcrypt` version.
@lkollar lkollar force-pushed the manylinux1-libxcrypt branch from c63773a to c7b1c68 Compare July 19, 2019 15:48
@zackw
Copy link

zackw commented Jul 19, 2019

Revised patch LGTM regarding how it handles lib(x)crypt, except that, as mentioned in the other thread, you may want to add --disable-werror to the configure arguments. I can't comment on integration with the rest of manylinux, though.

@lkollar
Copy link
Contributor Author

lkollar commented Jul 19, 2019

This one doesn't need the --disable-werror flag as it compiles fine with the older GCC version. I'm happy to add it to this PR as well but I didn't think it makes sense.

@takluyver
Copy link
Member

Thanks for getting on these so quickly. If you've got time, can you try building pyphash using the new images, and check that libcrypt.so.2 gets correctly bundled? And that it's not finding any way to link against libcrypt.so.1.

@lkollar
Copy link
Contributor Author

lkollar commented Jul 19, 2019

Sure, I'll give it a try. Not sure I'll have more time for this today though, maybe over the weekend.

@lkollar
Copy link
Contributor Author

lkollar commented Jul 21, 2019

I just compiled pyphash on the new manylinux1 image and it picked up libcrypt.so.2 correctly.

[root@df988c2676ee /]# /opt/python/cp37-cp37m/bin/python -c "import pyphash"
[root@df988c2676ee /]# ldd /opt/python/cp37-cp37m/lib/python3.7/site-packages/pyphash.cpython-37m-x86_64-linux-gnu.so
	linux-vdso.so.1 =>  (0x00007fff55866000)
	libcrypt.so.2 => /usr/local/lib/libcrypt.so.2 (0x00007f879ad03000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f879aae7000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f879a78e000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f879b13c000)
[root@df988c2676ee /]#

Copy link
Member

@takluyver takluyver left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me, then. Did you check with building on manylinux2010 (#325) as well?

I'm not quite confident enough in my understanding to press the merge button myself, but I don't think anything else needs doing. Hopefully someone more familiar with this stuff will press the button. If it's not pressed when I get back from holiday, I can do it.

@lkollar
Copy link
Contributor Author

lkollar commented Jul 22, 2019

Re-ran the same test on manylinux2010 as well: #325 (comment).

I tried doing an auditwheel repair on the pyphash wheel I built in the new image and auditwheel correctly vendored libcrypt.so.2 into the resulting package. The only issue I can see is if people try to use these images to audit wheels they have built on older images: audithweel will report the package as non-compliant as it will pick up the dependency on libcrypt.so.2 which will have to be vendored into the wheel to make it manylinux-compliant. Running audithweel repair on such a package works correctly though, so I think we should be OK.

@takluyver
Copy link
Member

Wouldn't wheels built on older images link against libcrypt.so.1, so they wouldn't work at all on the new images? The new images are effectively like Fedora 30 in that respect.

Either way, I think it's OK. I don't see an obvious use case for building wheels on one version of the images and then auditing them on a newer version.

@zackw
Copy link

zackw commented Jul 22, 2019

Wheels needing to vendor libcrypt.so.2 is the intended outcome, I believe.

@njsmith
Copy link
Member

njsmith commented Jul 22, 2019

The only issue I can see is if people try to use these images to audit wheels they have built on older images: audithweel will report the package as non-compliant

I think that's fine. The underlying reality is that those wheels used to be compliant, but then things changed, and now they're not compliant anymore. Auditwheel would just be reporting that fact.

@lkollar
Copy link
Contributor Author

lkollar commented Jul 22, 2019

Given that there are no outstanding concerns around this, I'm going to go ahead and land the two PRs.

@lkollar lkollar merged commit 7e9299a into pypa:manylinux1 Jul 22, 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