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

Correctly pass DYLD_LIBRARY_PATH to delocate #8672

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

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Jan 8, 2025

Resolves #8671. Alternative to #8673 and #8674

In #8500, I added

DYLD_LIBRARY_PATH = "$(pwd)/build/deps/darwin/lib"

However, there is a known problem where using DYLD_LIBRARY_PATH with cibuildwheel doesn't propagate to delocate - https://cibuildwheel.pypa.io/en/stable/faq/#macos-passing-dyld_library_path-to-delocate

This isn't a problem in our CI job, but apparently is when running cibuildwheel locally.

This PR uses the suggested fix from cibuildwheel.

@freakboy3742
Copy link
Contributor

For the benefit of reviewers - #8673 implements the @rpath removal approach mentioned in the PR description.

@freakboy3742
Copy link
Contributor

Gentle nudge on this (and/or #8673) - is there something preventing a merge on one of these two fixes for #8671?

@aclark4life
Copy link
Member

@freakboy3742 Presumably either this or #8673 can go in the next release for which the deadline to merge would be end of March.

@radarhere radarhere requested a review from hugovk February 6, 2025 06:47
@hugovk
Copy link
Member

hugovk commented Feb 6, 2025

Thanks for the PRs! Yeah, we'll definitely include one or the other in the next release. I don't see too much difference so will flip a coin unless someone would like to argue for one over the other :)

@hugovk hugovk added this to the 11.2.0 milestone Feb 6, 2025
@freakboy3742
Copy link
Contributor

Thanks for the PRs! Yeah, we'll definitely include one or the other in the next release. I don't see too much difference so will flip a coin unless someone would like to argue for one over the other :)

I'd argue the #8673 is better on the basis that it fixes the problem right next to the place where the problem is created, rather than relying on remembering later that a fix is needed for one specific library case. However, that's a strong opinion, weakly held; both solutions work.

In terms of timing - while it's great this will be in the next release, the bug prevents testing of macOS builds on a local machine, so I'd argue it would be beneficial to merge it sooner rather than later. In order to test my iOS patch, I need to have include a version of this patch.

@freakboy3742
Copy link
Contributor

I've just submitted #8743, which I believe is an even better solution, as it fixes the problem at the source; and, with any luck, eventually the patch used by that PR won't be needed.

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

Successfully merging this pull request may close these issues.

Delocate error when building Pillow on macOS
4 participants