php / php-src

The PHP Interpreter
https://www.php.net
Other
37.95k stars 7.73k forks source link

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

Open dkarlovi opened 5 months ago

dkarlovi commented 5 months ago

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:

https://github.com/dunglas/symfony-docker/issues/555 https://github.com/php/php-src/issues/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 commented 5 months ago

/cc @dunglas you might be interested here.

dunglas commented 5 months ago

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: https://github.com/dunglas/frankenphp/pull/666

devnexen commented 5 months ago

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

dkarlovi commented 5 months ago

@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 commented 5 months ago

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: https://github.com/php/php-src/pull/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 commented 5 months ago

To enable build on musl needs to solve a blocker https://github.com/php/php-src/issues/11678

andypost commented 5 months ago

Moreover it probably will reveal https://github.com/php/php-src/issues/12125

dkarlovi commented 5 months ago

@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. :smile:

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Checked history

Probably it makes sense to use 1.2.4+ in CI

arnaud-lb commented 2 weeks ago

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 https://github.com/php/php-src/pull/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 commented 2 weeks ago

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 commented 2 weeks ago

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: https://github.com/php/php-src/issues/13648

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

nielsdos commented 2 weeks ago

@dunglas See also https://github.com/php/php-src/issues/14734 which is likely related