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

Use patched version of zlib-ng to fix install name #8743

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

freakboy3742
Copy link
Contributor

Fixes #8671. Alternative to #8672 and #8673

When zlib-ng is compiled on macOS, it sets the install name of the libz.1.dylib to @rpath/libz.1.dylib. This means that any subsequent load requires a valid DYLD_LIBRARY_PATH to resolve the link.

Recent versions of macOS have a feature called System Integrity Protection (SIP) which (amongst other things) prevents DYLD_LIBRARY_PATH from being passed into subshells. As the build's dependencies aren't on the default library path on macOS, delocate-wheel is unable to resolve the libz library.

SIP is disabled on GitHub Actions configurations, so this problem isn't seen in CI; but is enabled by default on macOS machines, so individual developers building Pillow dependencies will get an error from cibuildwheel when the wheel is repaired.

This PR uses the PATCH_DIR mechanism provided by multibuild to provide a patch for zlib-ng (authored by @radarhere, submitted upstream as zlib-ng/zlib-ng#1867) to fix the issue with zlib-ng's install dir during the original build.

This is an alternative to:

I'd argue this is the best of the three approaches, as it fixes the root problem at the source, in a way that will (eventually) require no patch at all.

@radarhere radarhere changed the title Use patched version of zlib-ng to fix install name. Use patched version of zlib-ng to fix install name Feb 10, 2025
@radarhere
Copy link
Member

You may or may not have realised that the Windows build has seven "patches" of its own (they're not true patches at the moment, but rather find and replace) - https://github.com/python-pillow/Pillow/blob/main/winbuild/build_prepare.py#L139

Do you think these should be moved to be part of a "patches" directory as well?

@freakboy3742
Copy link
Contributor Author

I wasn't aware of the Windows patching strategy - I haven't really spent any time looking into the Windows build, since there's no overlap with iOS or macOS.

I agree it looks like those patches would be at least a candidate for migrating to a common patches folder - but I'd suggest such a refactor is orthogonal to this change. The macOS and Linux wheel builds are almost completely independent to the Windows builds at present; I don't know if there's much to functionally be gained by merging in this way.

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

Successfully merging this pull request may close these issues.

Delocate error when building Pillow on macOS
2 participants