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

[Chore] Officially support musl the same way glibc is supported #13877

Closed
dkarlovi opened this issue Apr 3, 2024 · 16 comments
Closed

[Chore] Officially support musl the same way glibc is supported #13877

dkarlovi opened this issue Apr 3, 2024 · 16 comments

Comments

@dkarlovi
Copy link

dkarlovi commented Apr 3, 2024

Description

From what I've understood talking to people closer to the project, musl is seen as an afterthought in PHP, it mostly works, but isn't seen as a priority.

This creates issues like performance issues and weird bugs for musl users:

dunglas/symfony-docker#555
#11678

While musl is not as used as glibc, it does have a strong presence when using with containers (Alpine based images) and also new directions PHP can take, like with FrankenPHP standalone binaries using static-php (which uses musl)

All this means musl usage is not insignificant and is quite likely to become larger with time, it would make sense to include running correctly on musl as a requisite for a release, include musl for performance regression tests, etc.

@dkarlovi
Copy link
Author

dkarlovi commented Apr 3, 2024

/cc @dunglas you might be interested here.

@dunglas
Copy link
Contributor

dunglas commented Apr 3, 2024

While I agree that having musl as a first-class citizen would be nice, the performance problems seem to be related to musl itself, and there's probably nothing the PHP project can do on its own. See for example: dunglas/frankenphp#666

@devnexen
Copy link
Member

devnexen commented Apr 3, 2024

using alternative allocators such as jemalloc helps noticeably with performance issues. @iluuu1994 might have an opinion about it from a CI's perspective.

@dkarlovi
Copy link
Author

dkarlovi commented Apr 3, 2024

@dunglas even this information would be good for the PHP project to have and propagate. For example, Alpine PHP packages might do the same thing you did (change allocator) if on musl. Musl bugs could be reported and hopefully fixed upstream. Overall, targeting musl would be beneficial overall IMO, even if just to make the issues better known.

@arnaud-lb
Copy link
Member

arnaud-lb commented Apr 3, 2024

Maybe a first step would be to add an Musl/Alpine build to CI, if only to find regressions. This PR tried to do so: #8621. IIRC tests were green except for those affected by one of the issues mentioned in the description, and the PR itself needed refactoring, but it was not far from completing.

@andypost
Copy link
Contributor

andypost commented Apr 3, 2024

To enable build on musl needs to solve a blocker #11678

@andypost
Copy link
Contributor

andypost commented Apr 3, 2024

Moreover it probably will reveal #12125

@dkarlovi
Copy link
Author

dkarlovi commented Apr 4, 2024

@andypost there's a bit of a chicken and egg situation here, yes: the build on musl will fail because of the blocker you've linked, but had the build on musl been there when the offending change happened, it wouldn't have allowed the change to be made in the first place. 😄

IMO a good start would be to add a musl build like @arnaud-lb suggested, even allow it to fail for the time being. Then it can be followed up by a series of PRs to fix the build and only then make it mandatory to merge, from then on musl becomes a first class citizen.

@dkarlovi
Copy link
Author

dkarlovi commented Apr 4, 2024

Thinking about this further, if this is the common POV with musl

using alternative allocators such as jemalloc helps noticeably with performance issues

maybe the musl build actually uses an alternative allocator by default (if the official stance of PHP is musl's is too slow), this then allows packagers to replicate it when building, producing the most performant version of PHP on musl and still knowing it will work as expected, because it's actually tested against that allocator.

@dunglas
Copy link
Contributor

dunglas commented Apr 4, 2024

Replacing the default allocator - at least in static context - is hacky (see https://github.com/dunglas/frankenphp/pull/666/files#diff-548dba7efda4c2f29f4eeb946db1da090e10308817056d55077d71146f8ad0ebR190-R198). Also, while musl now officially uses and supports the standard malloc() API (to allow replacing the allocator), bugs coming from the interactions with the 3rd-party allocators will likely not be supported upstream. It's also very unlikely that major projects using musl such as Alpine Linux switch to a third-party allocator.

In my opinion, as a first step, we should focus on supporting musl with its official allocator (mallocng). In the future, we may add new CI builds testing PHP with other allocators such as https://github.com/microsoft/mimalloc, but we have to keep in mind that it's an extra maintenance burden.

@andypost
Copy link
Contributor

andypost commented Apr 4, 2024

Checked history

Probably it makes sense to use 1.2.4+ in CI

@arnaud-lb
Copy link
Member

We now have a nightly job running on Alpine/Musl, targeting 8.4 and future versions. It was easier than anticipated and the amount of changes required was small (they are listed on #13925).

All tests pass when building and running against the packages listed in https://github.com/php/php-src/blob/5b482b706e9d3e8e185928cf5838ac7e80442877/.github/actions/apk/action.yml.

Most notably, gnu-libiconv and icu-data-full are required. I would recommend that Alpine packages for PHP depend on these to ensure consistent behavior on Alpine and other distros.

@andypost
Copy link
Contributor

Thank you! Just a nitpick - I bet krb5-dev is not needed but I see some mention in CURL even after https://github.com/php/php-src/blob/php-8.4.0beta4/UPGRADING.INTERNALS#L128

@dunglas
Copy link
Contributor

dunglas commented Aug 28, 2024

Excellent work @arnaud-lb, thank you very much! To officially support musl (at least in the ZTS/FrankenPHP context), it would be nice to fix or at least find a workaround for this crash: #13648

This bug is annoying because it affects most (all?) Laravel users. Laravel uses OpenSSL by default and on every request.

@nielsdos
Copy link
Member

@dunglas See also #14734 which is likely related

@dkarlovi
Copy link
Author

@arnaud-lb

We now have a nightly job running on Alpine/Musl, targeting 8.4 and future versions.

Should we close here, the original intent of the issue is solved by doing this IMO.

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

No branches or pull requests

7 participants