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

Upgrade Emscripten version #2116

Draft
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Draft

Conversation

brandonpayton
Copy link
Member

Motivation for the change, related issues

There are a couple of reasons for this upgrade:

  1. I am having trouble rebuilding php-wasm on my current system. References to older ubuntu images and emscripten versions has given me trouble.
  2. We want this Emscripten fix from @jeroenpf and @bgrgicak which was first included in Emscripten 3.1.73.

Implementation details

This PR:

  • Bumps the ubuntu docker image version due to build errors related to missing image. Referencing a newer ubuntu image seemed to fix the "missing image" errors.
  • Bumps Emscripten version
  • Adjusts Playground-specific patches to Emscripten output since the latest breaks that patching.

Testing Instructions (or ideally a Blueprint)

  • CI

@brandonpayton
Copy link
Member Author

Quoting from this earlier comment:

there is an issue due to a change in the Emscripten-generated JS. With Emscripten 3.1.74, our ExitStatus patch conflicts because the output changed from declaring ExitStatus as a function to declaring it as a class. Functions declared anywhere in a scope are available to all statements within a scope, even if the statements precede the function declaration. The same is not true for classes, so I started running into errors that were something like "ExitStatus cannot be referenced before initialized".

I wrote a small node script to identify the insertion point and perform the insertion but forgot it needed to run within a Docker container if we patch in the same place we patch other Emscripten things. By default, node and npm are not installed there. And this approach deviates from the current approach with replace.sh which uses a lot of hardcoded source code strings. I don't think the replace.sh approach is very readable, but maybe it would make sense to just add a similar replacement for that problem.

@bgrgicak, I am AFK for the rest of the week and do not plan to look at this again until next Monday. Please feel free to pick this up in the meantime if you want to fix this earlier.

@adamziel
Copy link
Collaborator

adamziel commented Jan 9, 2025

Good explorations here! I agree, the replace.sh approach is not readable at all. We could contribute some of these to the Emscripten repo but not all – e.g. I wouldn't expect the websocket decorator logic to make it past the review. What if we forked Emscripten, applied these changes, and then just kept our fork up to date with the upstream?

@brandonpayton
Copy link
Member Author

What if we forked Emscripten, applied these changes, and then just kept our fork up to date with the upstream?

This sounds like a good idea and definitely better than hand-patching various aspects of the build output.

I don't know if this means we will need to build emscripten itself on the fly or produce our own builds for each release version, but we can see...

Maybe we can keep a branch of customizations that is rebased and tagged whenever we upgrade, so our changes are always latest in the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Inbox
Development

Successfully merging this pull request may close these issues.

2 participants