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

Add an fpm-zts build #1344

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

RulerOf
Copy link

@RulerOf RulerOf commented Nov 14, 2022

Resolves #1317, #249

To echo this statement from @okdewit, I have a Symfony application using the PHP Parallel API that I cannot run correctly under FPM without ZTS.

It's not clear if there's a formal testing procedure here, but I can validate that I'm able to install the parallel extension in the php 7.4 container I've built:

╰─❯ docker run -it --rm php:7.4-fpm-zts bash
root@5c46acc8317c:/var/www/html# pecl install parallel-1.1.4
downloading parallel-1.1.4.tgz ...
Starting to download parallel-1.1.4.tgz (58,783 bytes)
..............done: 58,783 bytes
42 source files, building
<snip>
Build process completed successfully
Installing '/usr/local/lib/php/extensions/no-debug-zts-20190902/parallel.so'
install ok: channel://pecl.php.net/parallel-1.1.4
configuration option "php_ini" is not set to php.ini location
You should add "extension=parallel.so" to php.ini

@RulerOf
Copy link
Author

RulerOf commented Nov 29, 2022

@tianon my apologies if this is beating a dead horse, but would appreciate your thoughts here.

@DracoBlue
Copy link

please execute apply-templates.sh once and commit the resulting changes to the other files

@RulerOf
Copy link
Author

RulerOf commented Dec 12, 2022

@DracoBlue I ran that, but it just changes some file modes in a way that doesn't appear to make a lot of sense, might be some kind of platform issue since I'm on Mac OS running inside of a docker container using a bind mount. Deletes a couple files and effectively does a chmod -x on the others:

root@66938b17e1c5:/app# git diff
diff --git a/8.0-rc/alpine3.16/fpm/docker-php-entrypoint b/8.0-rc/alpine3.16/fpm/docker-php-entrypoint
old mode 100755
new mode 100644
<snip>
diff --git a/8.0/bullseye/apache/docker-php-entrypoint b/8.0/bullseye/apache/docker-php-entrypoint
old mode 100755
<snip>
diff --git a/8.1/buster/apache/docker-php-entrypoint b/8.1/buster/apache/docker-php-entrypoint
old mode 100755
new mode 100644
diff --git a/8.1/buster/fpm/docker-php-entrypoint b/8.1/buster/fpm/docker-php-entrypoint

I had to do a considerable amount of reverse engineering of the entire CI process to really understand what was going on in the build here, and thought that this step would be done there. Could you tell me if this git diff output looks right before I commit it to the branch or if I should try to set up a more accurate runtime environment first?

@DracoBlue
Copy link

It seems like the versions.json needs to be modified (see d461611#diff-2b39d33506bc7a34cef4b9ebf4cf8b1e3a5532f2131ceb37011b94261cec5f8cR53 as reference) to make clear which php version you want to use with fpm-zts.

Previous invocation of `sed -i` would cause broken behavior on Mac OS.
BSD sed requires a backup file for in-place editing.
See https://stackoverflow.com/a/22084103/7969681 for details.
@RulerOf
Copy link
Author

RulerOf commented Dec 13, 2022

@DracoBlue my problem was with Docker's VirtioFS file sharing for Mac OS. Seems to break the behavior of cp -a which was screwing all of my files up. The latest changes should have all of the missing pieces.

While I hadn't figured it out yet, the behavior that brought me into docker to run the scripts in the first place is that apply-templates.sh relies on behavior specific to GNU sed:

sed -i -e 's! php ! '"$cmd"' !g' "$version/$dir/docker-php-entrypoint"

I applied this cross-platform workaround as it's the shortest fix to that problem.

@yosifkit
Copy link
Member

We unfortunately do not have build server bandwidth to support another variation of PHP builds, so we cannot currently do fpm-zts.

I have a similar open issue about adding ZTS to the apache-based images (#742), but the conclusion is that it was not really supported usage and had performance implications. I would like to approve just enabling ZTS in the fpm images, but I don't know if there are still performance issues and the bench.php in PHP's source is not indicative of common PHP usage nor very exhaustive (it only takes 0.5-0.6 seconds on a fairly modern system).

@lfamorim
Copy link

lfamorim commented Feb 27, 2023

@yosifkit, I understand the performance implications of adding this feature, however due to its importance, I would like to discuss the possibility of enabling ZTS in the FPM images, as FPM-ZTS. Are there any sponsors who could provide the necessary resources to make this happen?

@RulerOf
Copy link
Author

RulerOf commented Mar 3, 2023

If the problem is a build slot availability and enabling ZTS on base FPM image isn't feasible, what about something like replacing plain zts with fpm-zts in the standard image lineup, then building zts from fpm-zts as a base image to cut down on build time?

It'd have some unused files laying around, but might be a good compromise.

@mikk150
Copy link
Contributor

mikk150 commented Apr 10, 2023

If the problem is a build slot availability and enabling ZTS on base FPM image isn't feasible, what about something like replacing plain zts with fpm-zts in the standard image lineup, then building zts from fpm-zts as a base image to cut down on build time?

It'd have some unused files laying around, but might be a good compromise.

now you broke all peoples code who use zts and do not need fpm

@oleg-andreyev
Copy link

it would be great to continue this effort

@tianon
Copy link
Member

tianon commented May 15, 2024

I will re-share https://github.com/krakjoe/pthreads/blob/4d1c2483ceb459ea4284db4eb06646d5715e7154/README.md#sapi-support (#249 (comment)), because I think it is still relevant:

pthreads v3 is restricted to operating in CLI only: I have spent many years trying to explain that threads in a web server just don't make sense, after 1,111 commits to pthreads I have realised that, my advice is going unheeded.

So I'm promoting the advice to hard and fast fact: you can't use pthreads safely and sensibly anywhere but CLI.

Thanks for listening ;)

@oleg-andreyev
Copy link

Personally the reason why I need it is simple - we use common image for web and cli, but threading want to apply only in cli context

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

Successfully merging this pull request may close these issues.

Is that ok if I change the template file to generate a fpm with zts version dockerfile?
7 participants