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

fix GH-8533: dynamic libphp linking on Mac #17183

Open
wants to merge 3 commits into
base: PHP-8.3
Choose a base branch
from

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Dec 16, 2024

Closes #8533.

Tested as functional on macOS 15.2 (Sequoia), but this should work on older versions too.

@dunglas dunglas force-pushed the fix/8533 branch 2 times, most recently from 744c5d7 to 34cb1f6 Compare December 16, 2024 21:20
@dunglas dunglas changed the title fix: dynamic libphp linking on Mac fix GH-8533: dynamic libphp linking on Mac Dec 16, 2024
@devnexen
Copy link
Member

Note that, normally, only security fixes go in PHP 8.2 branch now.

@dunglas dunglas changed the base branch from PHP-8.2 to PHP-8.3 December 17, 2024 09:06
@dunglas
Copy link
Contributor Author

dunglas commented Dec 17, 2024

@devnexen rebased against 8.3 (I thought active support for 8.2 ended at the end of this year, sorry)

@devnexen
Copy link
Member

No need to be sorry :) thanks for fixing this btw !

@bukka bukka requested a review from petk December 22, 2024 11:34
@bukka
Copy link
Member

bukka commented Dec 22, 2024

Is this specific only to libphp? Shouldn't this be applied on shared extensions as well?

@bukka
Copy link
Member

bukka commented Dec 22, 2024

What I mean is that if it would be possibly better handled in PHP_BUILD_SHARED instead.

@dunglas
Copy link
Contributor Author

dunglas commented Dec 22, 2024

I thought about that but I fear that changing the extension name from foo.so to foo.dylib will break many existing setups.
That's why I made the changes minimal.

Also, the issue seems to happen only when linking libphp.

@bukka
Copy link
Member

bukka commented Dec 22, 2024

Ok it might be cleaner to add new type for PHP_SELECT_SAPI (e.g. shared-dynamic - probably needs a better name) and then handle it in PHP_BUILD_SHARED which sets the OVERALL_TARGET. It could check the platform and make the difference on darwin. That seems slightly cleaner to me.

@dunglas
Copy link
Contributor Author

dunglas commented Dec 23, 2024

@bukka do you prefer something like that? I'm not sure it's cleaner but it's your call. OS detection must happen in embed/config.m4 because we must also change INSTALL_IT.

@bukka
Copy link
Member

bukka commented Dec 29, 2024

Ah ok, forgot about INSTALL_IT. It's probably still better than before so looks good to me. Let's wait for @petk for a bit to see if he has got any comments.

@petk
Copy link
Member

petk commented Dec 30, 2024

Sorry for late reply. I'm not ignoring the PRs, just too busy elsewhere. 😃 If it works that's great. I'm not sure I'll have time to check this so soon. I'll see what I can do in the following days. Probably, there will be some minor adjustments needed for 8.4 and master branches as there is refactored library path.

@dunglas
Copy link
Contributor Author

dunglas commented Feb 17, 2025

Any chance to get this merged @petk @bukka? This would help the Homebrew package for FrankenPHP.

Copy link
Member

@petk petk left a comment

Choose a reason for hiding this comment

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

Builds and seems to work fine. Tested this a bit on macOS. Thanks @dunglas

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.

4 participants