Skip to content

Add Alpine 3.22 (remove Alpine 3.20) #1580

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 2 commits into from
Jun 10, 2025

Conversation

tianon
Copy link
Member

@tianon tianon commented May 30, 2025

No description provided.

@yosifkit
Copy link
Member

😞 😮

In file included from /usr/src/php/ext/opcache/jit/zend_jit.c:48:
ext/opcache/jit/zend_jit_trace.c: In function 'zend_jit_trace':
ext/opcache/jit/zend_jit_trace.c:6337:70: warning: 'gen_handler' may be used uninitialized [-Wmaybe-uninitialized]
 6337 |                                                         (gen_handler || type == IS_UNKNOWN || !ra || !ra[ssa_op->result_def]));
ext/opcache/jit/zend_jit_internal.h:552:21: note: in definition of macro 'SET_STACK_TYPE'
  552 |                 if (_set_mem_type) { \
      |                     ^~~~~~~~~~~~~
In file included from /usr/src/php/ext/opcache/jit/zend_jit.c:4460:
ext/opcache/jit/zend_jit_trace.c:4288:30: note: 'gen_handler' was declared here
 4288 |                         bool gen_handler;
      |                              ^~~~~~~~~~~
/usr/src/php/ext/opcache/jit/zend_jit_x86.dasc: Assembler messages:
/usr/src/php/ext/opcache/jit/zend_jit_x86.dasc:2976: Error: @TLSGD operator requires `%rdi' as dest register
make: *** [Makefile:1303: ext/opcache/jit/zend_jit.lo] Error 1

@tianon
Copy link
Member Author

tianon commented May 30, 2025

(in which I yet again consider dropping zts variants entirely)

@yosifkit
Copy link
Member

🤔 👀 ❓ I found where the error is coming from, I think, but I don't know why or how to fix it.
https://sourceware.org/pipermail/binutils-cvs/2024-September/065790.html

@arnaud-lb
Copy link

See PHP issue: php/php-src#18743 (comment)

@nielsdos
Copy link

nielsdos commented Jun 9, 2025

See PHP issue: php/php-src#18743 (comment)

Follow-up on this: the fix got merged upstream in php-src, and the fix will be released with PHP 8.3.23 / PHP 8.4.9.

@yosifkit
Copy link
Member

yosifkit commented Jun 9, 2025

Awesome, thanks for the quick fix @nielsdos! ❤️ 🙏

@tianon, should we backport the fix? (php/php-src@b3c8afe)

@tianon
Copy link
Member Author

tianon commented Jun 9, 2025

(that's just a rebase for starters - we also need to decide what to do with 8.1 and 8.2 that didn't get a backported fix as far as I can see)

@tianon
Copy link
Member Author

tianon commented Jun 9, 2025

Oh, and we have the patching issue that the code sticks around -- I'm guessing patching this during the build of php itself is enough, but if this codepath ever ends up being compiled as part of a module build then we have to figure out a clean way to inject a patch into those docker-php-* scripts (which isn't something I'm too keen on doing).

@nielsdos
Copy link

nielsdos commented Jun 9, 2025

we also need to decide what to do with 8.1 and 8.2 that didn't get a backported fix as far as I can see

8.1 & 8.2 are out of bugfix support. If the patch doesn't apply/work there, then I can provide a patchfile for those versions that you can apply (ping me in that case).

@yosifkit
Copy link
Member

yosifkit commented Jun 9, 2025

Since 8.1 and 8.2 are out of bugfix support, we'll have to apply the backport ourselves until their end of life. As for extension time in docker-php-ext-* scripts, we can just leave the patches out (like the current patches on 8.1) and see what to do if there is a problem where any of them are needed for an extension that we don't build in.

@tianon
Copy link
Member Author

tianon commented Jun 9, 2025

@nielsdos looks like the upstream patch will work as-is for 8.2 and 8.3 (and the merge commit version in 8.4 - I'll push that change soon), but 8.1 needs something slightly different; would php/php-src@f171d5f be correct? did I miss anything?

@tianon
Copy link
Member Author

tianon commented Jun 9, 2025

FWIW, that patch does appear to build successfully 😅

@tianon tianon force-pushed the alpine3.22 branch 2 times, most recently from 8efe17a to 696a9fa Compare June 9, 2025 22:48
@nielsdos
Copy link

The patch to make it build on 8.1 seems right.
The index multiplier used is 16 but I believe it should be 8. Probably the index is 0 which would explain why it works anyway. I'm on my phone now so I can't double check.

@tianon
Copy link
Member Author

tianon commented Jun 10, 2025

Interesting, yeah -- in 8.2+ it appears that this was split specifically in part to set that value to 8 for __MUSL__ and __FreeBSD__, but still 16 for everything else?

https://github.com/php/php-src/blob/b3c8afe272a6919248986c703c2e1defc73ff707/ext/opcache/jit/zend_jit_x86.dasc#L2908-L2933

Should I perhaps update my patch to backport that __MUSL__ block too? (I'll openly admit that I don't know what any of this means/does, just that the syntax is pretty easy to read and intuit some amount of intent from. 😄)

@tianon
Copy link
Member Author

tianon commented Jun 10, 2025

php/php-src@6b105d4 is what that looks like -- I copied the #else block and updated 8 to 16 and then staged that and copied in the newer block from 8.2+ just to be absolutely sure I wasn't letting my eyes trick me on the block similarity and it was identical. 😄

(The #if above this already includes && !defined(__MUSL__) 👍)

@tianon
Copy link
Member Author

tianon commented Jun 10, 2025

I've verified locally that using that new patch also appears to work fine and compile successfully, so if it's more technically correct I'm happy to commit that swap. 👍 ❤️

@tianon
Copy link
Member Author

tianon commented Jun 10, 2025

in 8.2+ it appears that this was split specifically in part to set that value to 8 for __MUSL__

php/php-src@94ba883 is where this happened, specifically 👍 👀

@tianon
Copy link
Member Author

tianon commented Jun 10, 2025

(I'm now convinced enough that this is correct -- happy to hear dissent/feedback, though! ❤️)

@nielsdos
Copy link

php/php-src@6b105d4 is what that looks like -- I copied the #else block and updated 8 to 16 and then staged that and copied in the newer block from 8.2+ just to be absolutely sure I wasn't letting my eyes trick me on the block similarity and it was identical. 😄

(The #if above this already includes && !defined(__MUSL__) 👍)

I checked this and it is right. It's indeed technically more correct.

@tianon
Copy link
Member Author

tianon commented Jun 10, 2025

Perfect, thank you so much, @nielsdos! ❤️

@tianon tianon requested a review from yosifkit June 10, 2025 17:39
@yosifkit yosifkit merged commit 98d7f09 into docker-library:master Jun 10, 2025
58 checks passed
@yosifkit yosifkit deleted the alpine3.22 branch June 10, 2025 23:06
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Jun 10, 2025
Changes:

- docker-library/php@98d7f098: Merge pull request docker-library/php#1580 from infosiftr/alpine3.22
- docker-library/php@9c90483c: Backport patches for Alpine 3.22 + ZTS
- docker-library/php@904a84a1: Add Alpine 3.22 (remove Alpine 3.20)
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.

4 participants