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

Update to Emscripten 3.1.64 #589

Merged
merged 2 commits into from
Jul 29, 2024
Merged

Update to Emscripten 3.1.64 #589

merged 2 commits into from
Jul 29, 2024

Conversation

cxcorp
Copy link
Contributor

@cxcorp cxcorp commented Jul 29, 2024

This PR upgrades Emscripten to 3.1.64 and fixes things caused by breaking Emscripten changes.

  • 3.1.52 no longer allows using .bc, so use .o instead in Makefile. Using .a throws "Unknown format, not a static library!".
  • 3.1.55 no longer supports --memory-init-file so removed from Makefile

I don't have the capacity to do further testing other than just checking that the repo's npm run test passed, and don't have enough context about the repo to know what's breaking in the changelog so I'll just leave this work here in case it's useful.

Closes #588

3.1.52 no longer allows using .bc, so use .o instead. Using .a throws
"Unknown format, not a static library!".
v3.1.55 removed the --memory-init-file option, and this caused the build
to fail with "emcc: error: --memory-init-file is no longer supported".

Fixed by removing the option from Makefile.
@cxcorp cxcorp changed the title Update to Emscripten 3.1.52 Update to Emscripten 3.1.64 Jul 29, 2024
@cxcorp
Copy link
Contributor Author

cxcorp commented Jul 29, 2024

While testing this with the devcontainer, I bumped into puppeteer/puppeteer#12833 which I workarounded with

diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json
index ce6189c..072317b 100644
--- a/.devcontainer/devcontainer.json
+++ b/.devcontainer/devcontainer.json
@@ -20,7 +20,7 @@
        // Use 'postCreateCommand' to run commands after the container is created.
        // We use `npm ci` instead of `npm install` because we want to respect the lockfile and ONLY the lockfile.
        // That way, our devcontainer is more reproducible. --Taytay
-       "postCreateCommand": "npm ci",
+       "postCreateCommand": "PUPPETEER_DOWNLOAD_BASE_URL=https://storage.googleapis.com/chrome-for-testing-public npm ci",
        // Comment out connect as root instead. More info: https://aka.ms/vscode-remote/containers/non-root.
        "remoteUser": "node"
 }
\ No newline at end of file

with the proper fix being upgrading Puppeteer. Puppeteer's postinstall step still tries to download the correct version of Chrome so might as well delete the chrome installation from the devcontainer (although the fonts-freefont-ttf package is needed to render fonts though, I think).

@cxcorp
Copy link
Contributor Author

cxcorp commented Jul 29, 2024

Some warnings in CI:

warning: JS library symbol '$ALLOC_NORMAL' is deprecated. Please open a bug if you have a continuing need for this symbol [-Wdeprecated]
warning: JS library symbol '$ALLOC_STACK' is deprecated. Please open a bug if you have a continuing need for this symbol [-Wdeprecated]
warning: JS library symbol '$allocate' is deprecated. Please open a bug if you have a continuing need for this symbol [-Wdeprecated]
warning: JS library symbol '$allocateUTF8OnStack' is deprecated. Please open a bug if you have a continuing need for this symbol [-Wdeprecated]
emcc: warning: warnings in JS library compilation [-Wjs-compiler]

I think these come from e.g.: https://github.com/emscripten-core/emscripten/blob/8c81cac1bbae378bc7dde0c21c99602cbaf452d0/src/library_legacy.js#L12-L16

will probably become an upgrade blocker at some point but not right now

@lovasoa lovasoa merged commit 1eb07a3 into sql-js:master Jul 29, 2024
1 check passed
@lovasoa
Copy link
Member

lovasoa commented Jul 29, 2024

Thank you!

@cxcorp cxcorp deleted the upgrade-emscripten branch July 29, 2024 19:43
@vwh
Copy link

vwh commented Jul 29, 2024

Thank you!

image

This pull request broke the https://sql.js.org/dist/sql-wasm.wasm build
I had to use older WASM build to fix it

@cxcorp
Copy link
Contributor Author

cxcorp commented Jul 29, 2024

It looks like the main branch's CI deploys the dist to GitHub Pages, so the sql-wasm.wasm hosted there is from the main branch which is probably not backwards compatible with the sql-wasm.js from npm, as emscripten makes no ABI compatibility guarantees across versions. Using https://sql.js.org/dist/sql-wasm.js works at least with the docs' web demo.

@lovasoa
Copy link
Member

lovasoa commented Jul 30, 2024

Ok, I'll make a new release on npm

@lovasoa
Copy link
Member

lovasoa commented Jul 30, 2024

I updated puppeteer in 158dfb4

@lovasoa
Copy link
Member

lovasoa commented Jul 30, 2024

I just pushed v1.11.0 which is compatible with the wasm file from sql.js.org

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.

Emscripten 3.1.52 incompatible with build
3 participants