-
Notifications
You must be signed in to change notification settings - Fork 33
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
Initial support for setjmp/longjmp for PHP v7.4.32 #28
base: main
Are you sure you want to change the base?
Conversation
@fd, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great step forward.
We will definitely have to revise it more if we end up trying to merge this effort upstream into the PHP source base, but it is a huge improvement already!
There are some nits in the code, which again I will consider revisiting only later when we try going upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @fd. This is a great contribution. Some comments from my side.
As for the TODO list, I think we can iterate on this. For other PHP versions, I would aim for the 8.1 and 8.2 versions.
@@ -0,0 +1,465 @@ | |||
From 6c33996f4ec3cad08e6bbd670f788b2ecebc846a Mon Sep 17 00:00:00 2001 | |||
From: "[email protected]" <Wasm Labs Team> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be great to keep your authorship here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to do proper attribution, would you mind to update the patch with the information you prefer to be shown as your identity?
logStatus "Preparing artifacts... " | ||
mkdir -p ${WASMLABS_OUTPUT}/bin 2>/dev/null || exit 1 | ||
|
||
cp sapi/cgi/php-cgi ${WASMLABS_OUTPUT}/bin/php-cgi${WASMLABS_RUNTIME:+-$WASMLABS_RUNTIME} || exit 1 | ||
cp sapi/cgi/php-cgi-asyncify ${WASMLABS_OUTPUT}/bin/php-cgi${WASMLABS_RUNTIME:+-$WASMLABS_RUNTIME} || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cp sapi/cgi/php-cgi-asyncify ${WASMLABS_OUTPUT}/bin/php-cgi${WASMLABS_RUNTIME:+-$WASMLABS_RUNTIME} || exit 1 | |
mv sapi/cgi/php-cgi-asyncify ${WASMLABS_OUTPUT}/bin/php-cgi${WASMLABS_RUNTIME:+-$WASMLABS_RUNTIME} || exit 1 |
@fd, VMware has approved your signed contributor license agreement. |
Now after merging #33, this will need a rebase. |
@fd: we are thinking in merging your code rebasing it on top of current main, fixing merging conflicts. Would that be fine for you? |
Hello @fd, Sorry for the long time to reply and thank you for your contribution. After some long discussions, this project is currently in the process of being archived. However, the development of this project will continue as an independent community fork present at https://github.com/webassemblylabs/webassembly-language-runtimes. We would be grateful if you could recreate your contribution under that fork and we will work on applying it. |
This patch takes the setjmp polyfill from Ruby and integrates it with PHP v7.4.32.
Missing bits:
wasm-opt -O --asyncify -g --pass-arg=asyncify-ignore-imports -o [OUTPUT] [INPUT]