-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
11e1dc5
to
6724d14
Compare
blargh |
6724d14
to
025d39d
Compare
The error I keep coming back to is:
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:
|
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 :( |
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.) |
@carusogabriel If you have any clue about this your help would be appreciated. |
And the issue is also present in 8.0.0 final |
025d39d
to
b3bbb9b
Compare
b3bbb9b
to
6dd757e
Compare
50d86f2
to
ba537dd
Compare
Any progress on this PR so TS PHP needed for threaded apps is available? |
ba537dd
to
d26bb98
Compare
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. |
Is the issue still present /w Alpine latest/3.15? |
d26bb98
to
427cfd9
Compare
Thanks for the reminder! I've just rebased so we can find out. 😄 |
427cfd9
to
7706f84
Compare
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. |
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])
|
a02c99a
to
0979638
Compare
Here's the updated warning on 8.1 with that patch:
|
Something similar (but on
(Maybe that fix wasn't quite enough / in the right place for 8.0?) |
(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 🤘 |
Thanks for checking, @tianon! I'm a bit surprised due to |
See also php/php-src@804420b. Since @derickr committed this, maybe he has some ideas. |
@tianon please test patching PHP 8.0+ versions with php/php-src@PHP-8.0...mvorisek:abd95290272087285dc2827d0fa0fbe141db6ff7 (with both commits applied) |
fde89dc
to
aa8104e
Compare
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 |
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 |
Ahhh good catch -- we build from the released source tarballs, so indeed, regenerating |
aa8104e
to
ed03ce8
Compare
There we go; much better. 😄 Edit: specifically, php/php-src@php:8b15858...mvorisek:abd9529 applied on 8.0 and 8.1 fixes it 😄 |
php/php-src#8180 merged, so ZTS build should pass starting with upcoming PHP 8.0.18 and 8.1.5 releases. |
ed03ce8
to
2027b5d
Compare
The releases are out, can you please rebase/retest? |
2027b5d
to
83dea60
Compare
🟢 😱 🎉 ! |
Oops, forgot to come back to this -- thanks for the reminder! 👍 |
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
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
#1075, but this time with proper gusto
Closes #1074