PHP 8.4 #541

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

Jump to bottom

Merged

stesie merged 2 commits into phpv8:php8 from mykiwi-forks:php84-and-tooling

Jan 9, 2025

Merged

PHP 8.4 #541

stesie merged 2 commits into phpv8:php8 from mykiwi-forks:php84-and-tooling

Jan 9, 2025

Conversation

mykiwi

Copy link

Contributor

@mykiwi mykiwi commented Jan 3, 2025 •

edited

Loading

PHP 8.4

TLDR: 179/180 (+2 skip) tests passed (99.4%)

php 8.4.2 (on x86_64) test report:

PHP 8.3

TLDR: 179/180 (+2 skip) tests passed (99.4%)

php 8.3.15 (on x86_64) test report:

PHP 8.2

TLDR: 179/180 (+2 skip) tests passed (99.4%)

php 8.2.27 (on x86_64) test report:

PHP 8.1

TLDR: 179/180 (+2 skip) tests passed (99.4%)

php 8.1.31 (on x86_64) test report:

=====================================================================
TIME END 2025年01月09日 09:12:46
=====================================================================
TEST RESULT SUMMARY
---------------------------------------------------------------------
Exts skipped : 0
Exts tested : 33
---------------------------------------------------------------------
Number of tests : 182 180
Tests skipped : 2 ( 1.1%) --------
Tests warned : 0 ( 0.0%) ( 0.0%)
Tests failed : 1 ( 0.5%) ( 0.6%)
Tests passed : 179 ( 98.4%) ( 99.4%)
---------------------------------------------------------------------
Time taken : 9 seconds
=====================================================================
=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Test V8::executeString() : Time limit [tests/time_limit.phpt]
=====================================================================

@mykiwi mykiwi force-pushed the php84-and-tooling branch from 0719d54 to 86f373e Compare

January 3, 2025 18:38

@bastien-wink

Copy link

bastien-wink commented Jan 4, 2025

Interesting test run with docker and bake.
Do you consider also updating .github/workflows/build-test.yml ?

Most important is to fix those 11 tests so we can use PHP 8.4.

@JoshuaBehrens

Copy link

Contributor

JoshuaBehrens commented Jan 4, 2025 •

edited

Loading

@mykiwi there does not seem to be a reason for the version check. In #539 I was able to solve it without php version check

Why do we need a dockerfile here?

@JoshuaBehrens

Copy link

Contributor

JoshuaBehrens commented Jan 4, 2025

Time limit is also failing for me on MacOS with php8.3. Unsure right now how to solve it though. There are also issue right now for me as some things are missing around the DateTime timezone name lookup :/

@stesie

Copy link

Member

stesie commented Jan 4, 2025

I'd also say ignore the time limit ones for now. Locally on my Mac they fail as well. On the Linux machine (and CI) they pass just fine. TZ one likewise.

The others seem "just" to be caused by different stack traces (at least those I've checked)

If you compare

root@e48d4f358c52:/tmp/php-v8js# diff -u tests/commonjs_exception_001.{exp,out}
--- tests/commonjs_exception_001.exp	2025年01月04日 18:36:44.627206918 +0000
+++ tests/commonjs_exception_001.out	2025年01月04日 18:36:44.628206860 +0000
@@ -1,15 +1,15 @@
-Fatal error: Uncaught Exception: some exception in %s%ecommonjs_exception_001.php:9
+Fatal error: Uncaught Exception: some exception in /tmp/php-v8js/tests/commonjs_exception_001.php:9
 Stack trace:
-#0 [internal function]: {closure}('test')
-#1 %s%ecommonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
+#0 [internal function]: {closure:/tmp/php-v8js/tests/commonjs_exception_001.php:8}('test')
+#1 /tmp/php-v8js/tests/commonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
 #2 {main}
 
-Next V8JsScriptException: module.js:1: Exception: some exception in %s%ecommonjs_exception_001.php:9
+Next V8JsScriptException: module.js:1: Exception: some exception in /tmp/php-v8js/tests/commonjs_exception_001.php:9
 Stack trace:
-#0 [internal function]: {closure}('test')
-#1 %s%ecommonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
-#2 {main} in %s%ecommonjs_exception_001.php:12
+#0 [internal function]: {closure:/tmp/php-v8js/tests/commonjs_exception_001.php:8}('test')
+#1 /tmp/php-v8js/tests/commonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
+#2 {main} in /tmp/php-v8js/tests/commonjs_exception_001.php:12
 Stack trace:
-#0 %s%ecommonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
+#0 /tmp/php-v8js/tests/commonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
 #1 {main}
- thrown in %s%ecommonjs_exception_001.php on line 12
\ No newline at end of file
+ thrown in /tmp/php-v8js/tests/commonjs_exception_001.php on line 12
\ No newline at end of file

The expectation has just {closure} while the actual output on php 8.4 is {closure:/tmp/php-v8js/tests/commonjs_exception_001.php:8}
... therefore if you replace the expectation by {closure%s}, then it should match both, php 8.4 and the older ones

root@e48d4f358c52:/tmp/php-v8js# diff -u tests/generators_from_v8_008.{exp,out}
--- tests/generators_from_v8_008.exp	2025年01月04日 18:36:46.819080332 +0000
+++ tests/generators_from_v8_008.out	2025年01月04日 18:36:46.819080332 +0000
@@ -1,9 +1,9 @@
 int(23)
 
-Fatal error: Uncaught Exception: this shall not work in %s
+Fatal error: Uncaught Exception: this shall not work in /tmp/php-v8js/tests/generators_from_v8_008.php:14
 Stack trace:
-#0 [internal function]: {closure}()
+#0 [internal function]: {closure:/tmp/php-v8js/tests/generators_from_v8_008.php:13}()
 #1 [internal function]: Closure->__invoke()
-#2 %s: V8Generator->next()
+#2 /tmp/php-v8js/tests/generators_from_v8_008.php(18): V8Generator->next()
 #3 {main}
- thrown in %s
\ No newline at end of file
+ thrown in /tmp/php-v8js/tests/generators_from_v8_008.php on line 14
\ No newline at end of file

... here it now also says {closure:/tmp/php-v8js/tests/generators_from_v8_008.php:13}

@stesie

Copy link

Member

stesie commented Jan 4, 2025

and maybe one more thought: I think it might be confusing to users, that a Dockerfile shipped with a project doesn't yield a useable binary, but just test results.

What do you think of naming it e.g. Dockerfile.test or so ?

PS: is there some neat way to run the buildx bake stuff with the local repo? Am I missing something obvious?

@mykiwi

Copy link

Contributor Author

mykiwi commented Jan 5, 2025 •

edited

Loading

@bastien-wink : Do you consider also updating .github/workflows/build-test.yml ?

If maintainers want it, I can.

@bastien-wink : Most important is to fix those 11 tests so we can use PHP 8.4.

Yes, that where I needed help 🙌

@JoshuaBehrens : Why do we need a dockerfile here?

For me it is to be able to test it easily on your machine, with all benefits of docker: easy to tests with many php versions/distros/platforms, having cache, reproducibility.

@stesie : is there some neat way to run the buildx bake stuff with the local repo?

Sure, like this: docker buildx bake --set main.contexts.php-v8js-source-code=.
I forgot to push my Makefile with all thoses commands.

@stesie I think it might be confusing to users, that a Dockerfile shipped with a project doesn't yield a useable binary, but just test results. What do you think of naming it e.g. Dockerfile.test or so ?

What about creating a folder name devtools (or something else than "docker" to avoid confusion) and put files like the docker-bake.hcl, foobar.Dockerfile, ... ?

@JoshuaBehrens

Copy link

Contributor

JoshuaBehrens commented Jan 6, 2025

I 👍 a devtools folder. I am on devenv to switch easily between php versions (I test with brew though as it is common macOS installation way). I also 👍 the Dockerfile as dev utility as this would be a predefined linux environment to test for on any dev machine.

For me it is to be able to test it easily on your machine, with all benefits of docker: easy to tests with many php versions/distros/platforms, having cache, reproducibility

I see the default docker pitch but for this scenario I see it as a big 👍 . I see downsides but this would not be applicable just for the sake of it as a devtool 😁

The makefile looks pretty standard with the default help and clean target and should be therefore picked up easily by people who are used to work with make. Likely I would not move it into the devtools folder though as it would be the command set to control the devtools. So everything in the devtools folder and the makefile as the single entrypoint in root is plausible to me.

If maintainers want it, I can

I would be happy if it is updated as this is the part which makes sure, that all "common" scenarios should be tested. Building v8 though takes a lot of time and therefore one really needs to make sure, that the steps, that compile v8 themselves are cached properly.

Eventually I don't want to express a leading opinion here though ^^' @stesie is the one to decide.

@stesie

Copy link

Member

stesie commented Jan 6, 2025

Eventually I don't want to express a leading opinion here though ^^' @stesie is the one to decide.

mmmh, after all it has to meet all our requirements, and I don't want this to be up to my taste only.

Generally I like the idea of having a subfolder for all that fluff. And I agree that the Makefile should be top-level so it's easily available.

Regarding the CI stuff, if it easily fits there, why not re-use it. Much of it is different anyways ...
On the other hand, feel free to keep it off this PR to keep it small and then maybe follow-up with another one.

Most important is to fix those 11 tests so we can use PHP 8.4.
Yes, that where I need help 🙌

mmmh, I'm a bit confused ... do you still need help or are you good to go? Have you tried changing the {closure} expectation in the .phpt files to {closure%s}? Did it work? further errors?

TBH I haven't tried. So feel free to ask for more support, then I'll have a deeper look and try it myself. But I'm pretty sure that's the only issue you've got

@mykiwi

Copy link

Contributor Author

mykiwi commented Jan 6, 2025

do you still need help or are you good to go? Have you tried changing the {closure} expectation in the .phpt files to {closure%s}? Did it work? further errors?

I don't think, thanks! Not yet, but I will try.

@mykiwi

Copy link

Contributor Author

mykiwi commented Jan 7, 2025 •

edited

Loading

For information, I did {closur%s} instead of {closure%s} to keep tests pass on previous versions

I have 2 last errors:

Test V8::executeString() : Time limit [tests/time_limit.phpt]
Test V8::executeString() : var_dump [tests/var_dump.phpt]

If you have any idea on how to fix them, feel free to guide me

@mykiwi mykiwi force-pushed the php84-and-tooling branch 3 times, most recently from d5c2a24 to f697ed2 Compare

January 7, 2025 09:42

@mykiwi mykiwi marked this pull request as ready for review

January 7, 2025 09:47

@stesie

Copy link

Member

stesie commented Jan 8, 2025

First of all, and out of interest, why do we mix PHP 8.4 with changes in tooling here? TBH I think discussions would be way cleaner, if we'd sort out that stuff separately.

That said, I'm still not totally sold to the new build stuff, what's the advantage of it, etc. ... but maybe that's also just because I'm doing it manually/differently for ten years now :) ... so I'm totally up to adding this, but to me it would be helpful if there would be some guiding words (read: documentation), which problem it solves, how it's suppose to be used, and importantly how to debug it.

So yes, if I'm running it, I get the final result in tests-outputs/php8.4.2_x86_64__1-std (took me a while to find it), but that's only the console output. How am I supposed to access the exp/out files of the failed tests? To access these I have to somehow copy that stuff from the docker images, right ?

what's worse, this now adds a Makefile in toplevel dir (yes, sorry, I even asked for that, but have overseen stuff) ... however, if you run the ./configure script, then it overrides that Makefile. And since it's under version control then wants to commit that.
Which is bad IMHO ... so maybe move that file to devtools as well, and then document that recommended approach is to run make -f devtools/Makefile php84 or so

regarding php 8.4 changes

with the new docker build both tests, you've mentioned, fail for me as well.

If I build it the "classic" way (on Ubuntu 24.04, amd64 machine) with ppa:ondrej/php active and php8.4-dev/cli installed from there

./configure --with-v8js=/opt/v8-13.1.63 LDFLAGS=-lstdc++ 'CPPFLAGS=-DV8_COMPRESS_POINTERS -DV8_ENABLE_SANDBOX -Wno-invalid-offsetof -Wno-deprecated-declarations -Wno-unused-result' --with-php-config=/usr/bin/php-config8.4
make clean
make -j4
make test

The time_limit one passes just fine ... that one is "known" to be flaky ... I've seen it failing on MacOS as well. So I wouldn't care about that one.

regarding the var_dump.phpt ... if you have a look at the .out file

the first var_dump is producing the following output for the "function" key

 ["function"]=>
 object(Closure)#4 (4) {
 ["name"]=>
 string(58) "{closure:/home/stesie/Projekte/v8js/tests/var_dump.php:51}"
 ["file"]=>
 string(45) "/home/stesie/Projekte/v8js/tests/var_dump.php"
 ["line"]=> 
 int(51)
 ["parameter"]=>
 array(1) {
 ["$x"]=>
 string(10) "<required>"
 }
 }

however the expectation looks like this

 \["function"\] \=\>
 object\(Closure\)\#\d+ \{
 function id\(x\) \{ return x; \}
 \}

... or to put it differently, php 8.4 completely changed the output of var_dump here :)

Also for example the DateTime object, which is also printed, now has more methods.

I think having dedicated tests, one testing the closure/datetime printing on php < 8.4 and one for php >= 8.4 would be better, even though it would be a bit more redundant.

And while being at that test file, you also might want to consider (partialy) reverting my commit

commit fcd78f830ac29fdc4e48834e313383e3d15665e1
Author: Stefan Siegl <stesie@brokenpipe.de>
Date: Sun Jun 23 19:09:12 2019 +0200
 use EXPECTREGEX to handle different PHP version's output well

... which introduced that EXPECTREGEX usage, which is really hard to maintain.

Or to put it differently, I'd suggest splitting the var_dump.phpt test into a var_dump_php83_and_older.phpt and a var_dump_php84_and_newer.phpt ... that share the same implementation, but have (slightly) different test expectations (and you use the SKIPIF blocks to skip either this or that; see above commit on how that can be done)

mykiwi added 2 commits

January 9, 2025 09:36

@mykiwi


 adapt code to handle php 8.4 too (based on @JoshuaBehrens & @rbro wor...

6386e9d

...k) and fix some tests
Signed-off-by: Romain Gautier <mail@romain.sh>

@mykiwi


 update var_dump test to handle php84 and previous versions

c2614b5

Signed-off-by: Romain Gautier <mail@romain.sh>

@mykiwi mykiwi force-pushed the php84-and-tooling branch from d3c99d3 to c2614b5 Compare

January 9, 2025 09:11

@mykiwi mykiwi changed the title ~~(削除) PHP 8.4 (and docker tooling/documentation) (削除ここまで)~~ (追記) PHP 8.4 (追記ここまで)

Jan 9, 2025

@mykiwi

Copy link

Contributor Author

mykiwi commented Jan 9, 2025 •

edited

Loading

@stesie First of all, and out of interest, why do we mix PHP 8.4 with changes in tooling here? TBH I think discussions would be way cleaner, if we'd sort out that stuff separately.

No problem. I updated the PR to only contains what is useful for php8.4

So yes, if I'm running it, I get the final result in tests-outputs/php8.4.2_x86_64__1-std (took me a while to find it), but that's only the console output.

If you don't have tests-outputs/php8.4.2_x86_64__2-report, that means something wrong append and maybe there are some details why in tests-outputs/php8.4.2_x86_64__1-std

I'd suggest splitting the var_dump.phpt test into a var_dump_php83_and_older.phpt and a var_dump_php84_and_newer.phpt ... that share the same implementation, but have (slightly) different test expectations (and you use the SKIPIF blocks to skip either this or that; see above commit on how that can be done)

Done

JoshuaBehrens

JoshuaBehrens approved these changes

Jan 9, 2025

View reviewed changes

stesie added a commit that referenced this pull request

Jan 9, 2025

@stesie


 Fix tests/var_dump.phpt to not require certain build path, refs #541

38c0d6b

@stesie stesie merged commit c2614b5 into phpv8:php8

Jan 9, 2025

2 of 3 checks passed

@stesie

Copy link

Member

stesie commented Jan 9, 2025

@mykiwi thank you very much for your contribution!

I've pushed one more minor fix, the test fixture was asserting for the build location, which obviously isn't ideal :)

diff --git a/tests/var_dump.phpt b/tests/var_dump.phpt
index 9097ca9..13ca64e 100644
--- a/tests/var_dump.phpt
+++ b/tests/var_dump.phpt
@@ -114,9 +114,9 @@ array(11) {
 ["function"]=>
 object(Closure)#4 (4) {
 ["name"]=>
- string(45) "{closure:/tmp/php-v8js/tests/var_dump.php:51}"
+ string(%d) "{closure:%s/tests/var_dump.php:51}"
 ["file"]=>
- string(32) "/tmp/php-v8js/tests/var_dump.php"
+ string(%d) "%s/tests/var_dump.php"
 ["line"]=>
 int(51)
 ["parameter"]=>

apart from that, nice work, thanks :)

@stesie stesie mentioned this pull request

Jan 9, 2025

php 8.4 compile error - php_strtolower not declared #540

Closed

Labels

None yet

4 participants

@mykiwi @bastien-wink @JoshuaBehrens @stesie

PHP 8.4 #541

PHP 8.4 #541

Uh oh!

Conversation

@mykiwi mykiwi commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PHP 8.4

PHP 8.3

PHP 8.2

PHP 8.1

Uh oh!

bastien-wink commented Jan 4, 2025

Uh oh!

JoshuaBehrens commented Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshuaBehrens commented Jan 4, 2025

Uh oh!

stesie commented Jan 4, 2025

Uh oh!

stesie commented Jan 4, 2025

Uh oh!

mykiwi commented Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshuaBehrens commented Jan 6, 2025

Uh oh!

stesie commented Jan 6, 2025

Uh oh!

mykiwi commented Jan 6, 2025

Uh oh!

mykiwi commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stesie commented Jan 8, 2025

Uh oh!

mykiwi commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

stesie commented Jan 9, 2025

Uh oh!

Uh oh!

@mykiwi mykiwi commented Jan 3, 2025 •

edited

Loading

JoshuaBehrens commented Jan 4, 2025 •

edited

Loading

mykiwi commented Jan 5, 2025 •

edited

Loading

mykiwi commented Jan 7, 2025 •

edited

Loading

mykiwi commented Jan 9, 2025 •

edited

Loading