Skip to content

Add back 8-zts-alpine, take 2 #1076

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

Merged
merged 1 commit into from
May 4, 2022

Conversation

tianon
Copy link
Member

@tianon tianon commented Oct 26, 2020

#1075, but this time with proper gusto

Closes #1074

@tianon
Copy link
Member Author

tianon commented Oct 26, 2020

blargh

@tianon tianon force-pushed the 8-zts-alpine-take-2 branch from 6724d14 to 025d39d Compare November 25, 2020 21:56
@WyriHaximus
Copy link

The error I keep coming back to is:

_tsrm_ls_cache: initial-exec TLS resolves to dynamic definition

Some Googling seems to suggest a musl issue, but I've also tried building with Alpine 3.11 and got the same error, as with Alpine 3.10.

Full error:

Warning: PHP Startup: Unable to load dynamic library 'pdo_mysql.so' (tried: /usr/local/lib/php/extensions/no-debug-zts-20200930/pdo_mysql.so (Error relocating /usr/local/lib/php/extensions/no-debug-zts-2
0200930/pdo_mysql.so: _tsrm_ls_cache: initial-exec TLS resolves to dynamic definition in /usr/local/lib/php/extensions/no-debug-zts-20200930/pdo_mysql.so), /usr/local/lib/php/extensions/no-debug-zts-2020
0930/pdo_mysql.so.so (Error loading shared library /usr/local/lib/php/extensions/no-debug-zts-20200930/pdo_mysql.so.so: No such file or directory)) in Unknown on line 0

@tianon tianon mentioned this pull request Nov 25, 2020
@WyriHaximus
Copy link

Also just tried edge which comes with musl 1.2 (where 3.12 comes with 1.1) and still getting the same error. Getting the feeling, this issue purely comes from PHP 8 itself :(

@WyriHaximus
Copy link

After reading https://wiki.php.net/zts-improvement and not having a clue what it means I also found https://bugs.freedesktop.org/show_bug.cgi?id=35268 which seems to suggest this needs to change in PHP itself. (As far as I understand.)

@WyriHaximus
Copy link

@carusogabriel If you have any clue about this your help would be appreciated.

@WyriHaximus
Copy link

And the issue is also present in 8.0.0 final

@tianon tianon force-pushed the 8-zts-alpine-take-2 branch from 025d39d to b3bbb9b Compare November 30, 2020 18:11
@tianon tianon force-pushed the 8-zts-alpine-take-2 branch from b3bbb9b to 6dd757e Compare January 4, 2021 18:11
@tianon tianon force-pushed the 8-zts-alpine-take-2 branch 2 times, most recently from 50d86f2 to ba537dd Compare April 14, 2021 20:07
@J0WI J0WI mentioned this pull request Jun 16, 2021
@mvorisek
Copy link
Contributor

Any progress on this PR so TS PHP needed for threaded apps is available?

@tianon tianon force-pushed the 8-zts-alpine-take-2 branch from ba537dd to d26bb98 Compare June 30, 2021 16:53
@tianon
Copy link
Member Author

tianon commented Jun 30, 2021

To be clear -- any actual progress on this PR is likely to happen in PHP upstream, not in the image (however, Alpine is not exactly well-supported, so I wouldn't suggest holding your breath).

I've rebased and added the 8.1 variants too so we can see whether anything has progressed.

@mvorisek
Copy link
Contributor

Is the issue still present /w Alpine latest/3.15?

@tianon tianon force-pushed the 8-zts-alpine-take-2 branch from d26bb98 to 427cfd9 Compare February 28, 2022 20:42
@tianon
Copy link
Member Author

tianon commented Feb 28, 2022

Thanks for the reminder! I've just rebased so we can find out. 😄

@mvorisek
Copy link
Contributor

mvorisek commented Mar 1, 2022

I have opened php/php-src#8160 in php-src. I do not know the issue directly and the CI output seems very limited, so if anyone knows the problem, please describe in the php-src issue, preferably with minimalistic Dockerfile to reproduce.

@cmb69
Copy link

cmb69 commented Mar 1, 2022

Can you please try with the following patch:

 ext/pdo/config.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/pdo/config.m4 b/ext/pdo/config.m4
index 9b9a3e36df..63a2f7ed0f 100644
--- a/ext/pdo/config.m4
+++ b/ext/pdo/config.m4
@@ -9,7 +9,7 @@ if test "$PHP_PDO" != "no"; then
   dnl Make sure $PHP_PDO is 'yes' when it's not 'no' :)
   PHP_PDO=yes
 
-  PHP_NEW_EXTENSION(pdo, pdo.c pdo_dbh.c pdo_stmt.c pdo_sql_parser.c pdo_sqlstate.c, $ext_shared)
+  PHP_NEW_EXTENSION(pdo, pdo.c pdo_dbh.c pdo_stmt.c pdo_sql_parser.c pdo_sqlstate.c, $ext_shared,,-DZEND_ENABLE_STATIC_TSRMLS_CACHE=1)
   PHP_ADD_EXTENSION_DEP(pdo, spl, true)
   PHP_INSTALL_HEADERS(ext/pdo, [php_pdo.h php_pdo_driver.h php_pdo_error.h])
 

@tianon tianon force-pushed the 8-zts-alpine-take-2 branch from a02c99a to 0979638 Compare March 1, 2022 21:51
@tianon
Copy link
Member Author

tianon commented Mar 1, 2022

Here's the updated warning on 8.1 with that patch:

Warning: Failed loading Zend extension 'opcache' (tried: /usr/local/lib/php/extensions/no-debug-zts-20210902/opcache (Error loading shared library /usr/local/lib/php/extensions/no-debug-zts-20210902/opcache: No such file or directory), /usr/local/lib/php/extensions/no-debug-zts-20210902/opcache.so (Error relocating /usr/local/lib/php/extensions/no-debug-zts-20210902/opcache.so: std_object_handlers: initial-exec TLS resolves to dynamic definition in /usr/local/lib/php/extensions/no-debug-zts-20210902/opcache.so)) in Unknown on line 0

@tianon
Copy link
Member Author

tianon commented Mar 1, 2022

Something similar (but on pdo_mysql.so instead of opcache.so) on 8.0:

Warning: PHP Startup: Unable to load dynamic library 'pdo_mysql.so' (tried: /usr/local/lib/php/extensions/no-debug-zts-20200930/pdo_mysql.so (Error relocating /usr/local/lib/php/extensions/no-debug-zts-20200930/pdo_mysql.so: _tsrm_ls_cache: initial-exec TLS resolves to dynamic definition in /usr/local/lib/php/extensions/no-debug-zts-20200930/pdo_mysql.so), /usr/local/lib/php/extensions/no-debug-zts-20200930/pdo_mysql.so.so (Error loading shared library /usr/local/lib/php/extensions/no-debug-zts-20200930/pdo_mysql.so.so: No such file or directory)) in Unknown on line 0

(Maybe that fix wasn't quite enough / in the right place for 8.0?)

@tianon
Copy link
Member Author

tianon commented Mar 1, 2022

(I've also just pushed docker-library/official-images@ce73036 which should make the next run of these tests in CI actually show the error/warning correctly, which will be a lot more useful. 🤦)

Edit: success (at getting the test to print the warning/error): https://github.com/docker-library/php/runs/5382861733?check_suite_focus=true#step:7:9 🤘

@cmb69
Copy link

cmb69 commented Mar 2, 2022

Thanks for checking, @tianon! I'm a bit surprised due to
https://github.com/php/php-src/blob/PHP-8.0/TSRM/TSRM.h#L150-L156. So for __MUSL__, tls_model("initial-exec") shouldn't be used at all.

@cmb69
Copy link

cmb69 commented Mar 2, 2022

See also php/php-src@804420b. Since @derickr committed this, maybe he has some ideas.

@mvorisek
Copy link
Contributor

mvorisek commented Mar 7, 2022

@tianon please test patching PHP 8.0+ versions with

php/php-src@PHP-8.0...mvorisek:abd95290272087285dc2827d0fa0fbe141db6ff7 (with both commits applied)

@tianon tianon force-pushed the 8-zts-alpine-take-2 branch 3 times, most recently from fde89dc to aa8104e Compare March 7, 2022 19:20
@tianon
Copy link
Member Author

tianon commented Mar 7, 2022

Looks like that still did the trick for 8.1, but not 8.0. 😅

@mvorisek
Copy link
Contributor

mvorisek commented Mar 7, 2022

Looks like that still did the trick for 8.1, but not 8.0. 😅

weird, very weird, my build is passing even for PHP 8.0

see mvorisek/image-php@29fdca7

here is a test: https://github.com/atk4/data/runs/5456421886?check_suite_focus=true#step:4:5 , the CI confirms ZTS

here is the full patching code: mvorisek/image-php@29fdca7#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR55-R108 , it is based on 7.4/alpine3.15/zts/ source from this repo

@mvorisek
Copy link
Contributor

mvorisek commented Mar 7, 2022

I build from source mvorisek/image-php@29fdca7#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR81

I belive that is the issue here, as the patching in this PR does not change the prebuilt configure file in any way (but my fix/patch changes configure.ac).

@tianon
Copy link
Member Author

tianon commented Mar 8, 2022

Ahhh good catch -- we build from the released source tarballs, so indeed, regenerating configure is not something we do (especially since it can often have negative effects depending on our version of autoconf). I'll throw that into my hack here, since I verified that your build does indeed pass the offending test.

@tianon tianon force-pushed the 8-zts-alpine-take-2 branch from aa8104e to ed03ce8 Compare March 8, 2022 00:40
@tianon
Copy link
Member Author

tianon commented Mar 8, 2022

There we go; much better. 😄

Edit: specifically, php/php-src@php:8b15858...mvorisek:abd9529 applied on 8.0 and 8.1 fixes it 😄

@mvorisek
Copy link
Contributor

php/php-src#8180 merged, so ZTS build should pass starting with upcoming PHP 8.0.18 and 8.1.5 releases.

@tianon tianon force-pushed the 8-zts-alpine-take-2 branch from ed03ce8 to 2027b5d Compare March 18, 2022 17:00
@mvorisek
Copy link
Contributor

The releases are out, can you please rebase/retest?

@tianon tianon force-pushed the 8-zts-alpine-take-2 branch from 2027b5d to 83dea60 Compare April 18, 2022 18:22
@WyriHaximus
Copy link

🟢 😱 🎉 !

@mvorisek
Copy link
Contributor

mvorisek commented May 4, 2022

image

@tianon
Copy link
Member Author

tianon commented May 4, 2022

Oops, forgot to come back to this -- thanks for the reminder! 👍

@tianon tianon merged commit 16b3a9d into docker-library:master May 4, 2022
@tianon tianon deleted the 8-zts-alpine-take-2 branch May 4, 2022 20:15
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request May 5, 2022
Changes:

- docker-library/php@4aac9a2: Update 8.1-rc
- docker-library/php@2a12c7c: Update 8.0-rc
- docker-library/php@16b3a9d: Merge pull request docker-library/php#1076 from infosiftr/8-zts-alpine-take-2
BaurzhanSakhariev pushed a commit to crate/official-images that referenced this pull request Jul 18, 2022
Changes:

- docker-library/php@4aac9a2: Update 8.1-rc
- docker-library/php@2a12c7c: Update 8.0-rc
- docker-library/php@16b3a9d: Merge pull request docker-library/php#1076 from infosiftr/8-zts-alpine-take-2
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.

ZTS for PHP 8 Alpine images
4 participants