Skip to content

wasm: Update EMCC_OPTIONS default behavior #491

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

Merged
1 commit merged into from
Apr 2, 2025

Conversation

shesek
Copy link
Contributor

@shesek shesek commented Apr 2, 2025

The existing build_wasm.sh script always enables WASM_BIGINT, which is necessary for the NPM WASM package but has to be disabled for the esplora build.

This commit changes the default behavior to enable all options necessary for the NPM package (WASM_BIGINT plus EXPORT_ES6 too), while allowing the esplora build to override it with a separate set of options (MODULARIZE and EXPORT_NAME only).

The esplora build process already sets the appropriate EMCC_OPTIONS, so no changes necessary there. With this PR merged, we can update the esplora build to use the latest libwally (it currently uses an older version which did not set WASM_BIGINT, and would not work against libwally's current master).

This also updates the libwally gitlab CI build to use the correct EMCC_OPTIONS for esplora.

However I'm not quite sure why these options are duplicated in this repo. Wouldn't it be better to let the esplora build process control them? So for example, if a new EXPORTED_FUNCTION is needed for esplora the changes would be contained in the esplora repo and won't require sending PRs to two separate repos.

- If no EMCC_OPTIONS is specified, enable all options necessary for the
  NPM WASM package build by default.

- If it is specified, use it as-is with no added defaults. This enables full
  control over the options, e.g. for esplora, which has to disable some.

This also updates the gitlab CI build to use the correct EMCC_OPTIONS
for the esplora build.
@shesek shesek force-pushed the 202504-emcc-options branch from bf46242 to ed4d512 Compare April 2, 2025 11:28
@shesek
Copy link
Contributor Author

shesek commented Apr 2, 2025

With this PR merged, we can update the esplora build to use the latest libwally

I tested esplora against this PR branch (using the Docker build) and it appears to work, however one more minor change is necessary (the build target directory was changed from wally_dist to just dist). I'll open a PR on esplora once this PR is merged.

@jgriffiths
Copy link
Contributor

However I'm not quite sure why these options are duplicated in this repo. Wouldn't it be better to let the esplora build process control them?

The esplora build process changes (which are either in-house, in-flight or have landed, I don't follow esplora commits) moved to split-arch, x86 and arm64. In that process, rebuilding the wally wasm was happening with different emcc versions since arm64 release lag quite a bit. Since the resulting wasm should be consistent no matter where esplora is built, it was decided to make esplora wasm release part of the wally release process, so esplora can just download/sha256sum check the build binary and include it directly instead of rebuilding it pointlessly every time.

The use of wally for unblinding in esplora hasn't changed in 4+ years and there are no plans for further changes AFAIU. The only reason its being changed now and the current b0rkage was noticed is the move to supporting arm64. With usable binary releases and no need to rebuild wally, the build in esplora can be greatly simplified, and wally will take responsibility for ensuring the file remains compatible.

@jgriffiths jgriffiths closed this pull request by merging all changes into ElementsProject:master in f394af8 Apr 2, 2025
@jgriffiths
Copy link
Contributor

@shesek Merged, thanks. Would you be able to have a look at #470, I have no idea whats going on in JS land tbh...

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.

2 participants