Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fd
Copy link

@fd fd commented Jan 13, 2023

This patch takes the setjmp polyfill from Ruby and integrates it with PHP v7.4.32.

Missing bits:

  • running wasm-opt with the asyncify pass.
    wasm-opt -O --asyncify -g --pass-arg=asyncify-ignore-imports -o [OUTPUT] [INPUT]
  • update READMEs
  • integrate with other PHP versions
  • maybe turn it into a lib so other dependencies can also use it

@vmwclabot
Copy link

@fd, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link

@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.

Copy link
Contributor

@assambar assambar left a 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.

Copy link
Contributor

@ereslibre ereslibre left a 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>
Copy link
Contributor

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 :)

Copy link
Contributor

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?

Dockerfile.wasi-builder Outdated Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

@vmwclabot
Copy link

@fd, VMware has approved your signed contributor license agreement.

@ereslibre
Copy link
Contributor

Now after merging #33, this will need a rebase.

@ereslibre
Copy link
Contributor

@fd: we are thinking in merging your code rebasing it on top of current main, fixing merging conflicts. Would that be fine for you?

@ereslibre
Copy link
Contributor

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.

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