Skip to content

Remove SetupEmscripten C++ code#6658

Merged
hoodmane merged 1 commit intomainfrom
hoodmane/remove-emscripten-setup
Apr 28, 2026
Merged

Remove SetupEmscripten C++ code#6658
hoodmane merged 1 commit intomainfrom
hoodmane/remove-emscripten-setup

Conversation

@hoodmane
Copy link
Copy Markdown
Contributor

Instead import setupEmscripten.ts directly. As requested by @jasnell because SetupEmscripten does not work with new module registry.

@hoodmane hoodmane requested review from a team as code owners April 24, 2026 00:42
@hoodmane hoodmane force-pushed the hoodmane/remove-emscripten-setup branch from 650f0e3 to 1fd9e45 Compare April 24, 2026 00:43
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

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

I'm Bonk, and I've done a quick review of your PR.

This PR removes the SetupEmscripten C++ code path and instead imports setupEmscripten.ts directly as an internal module, simplifying the Emscripten initialization pipeline.

  1. [HIGH] Debug print() statement left in helpers.bzl — this will produce output on every Bazel load of this file, polluting build logs.
  2. [LOW] python_stdlib.zip.d.ts is now a dead file — the declare module 'pyodide-internal:generated/python_stdlib.zip' was removed but the file remains with bare top-level declarations that nothing references. Consider deleting it entirely since modules.d.ts already declares the new pyodideRuntime-internal:python_stdlib.zip module.

This review was generated by an AI assistant and may contain inaccuracies.

Comment thread src/pyodide/helpers.bzl Outdated
Comment thread src/pyodide/types/python_stdlib.zip.d.ts Outdated
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 24, 2026

Review posted on PR #6658 with two findings:

  1. [HIGH] Debug print() left in src/pyodide/helpers.bzl:284 — this will produce output on every Bazel load, polluting build logs. Posted a suggestion to remove it.

  2. [LOW] src/pyodide/types/python_stdlib.zip.d.ts is now a dead file — the module declaration was removed but the file remains with bare top-level declarations that nothing references. Suggested deleting it since modules.d.ts already declares the new pyodideRuntime-internal:python_stdlib.zip module.

The rest of the PR looks clean: all SetupEmscripten/EmscriptenRuntime references are fully removed, the C++ deletion is complete, the Bazel module type reclassifications (data -> internal/wasm) are correct, and the TypeScript signature change from sync to async loadPyodide is properly awaited at the call site.

github run

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 24, 2026

Merging this PR will not alter performance

✅ 72 untouched benchmarks
⏩ 129 skipped benchmarks1


Comparing hoodmane/remove-emscripten-setup (98ad560) with main (f75a8ae)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@danlapid
Copy link
Copy Markdown
Collaborator

Goodbye old friend - you will not be missed

Copy link
Copy Markdown
Contributor

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

I don't have the exact context of why it was existed in C++ originally, but the change looks good overall.

Comment thread src/pyodide/internal/pool/sentinel.ts Outdated
Comment thread src/pyodide/internal/python.ts Outdated
Comment thread src/pyodide/internal/python.ts
Comment thread src/pyodide/internal/pool/builtin_wrappers.ts Outdated
Copy link
Copy Markdown
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

I wonder if there are other things we can remove now that SetupEmscripten is gone.

Comment thread src/pyodide/internal/python.ts
Comment thread src/pyodide/internal/pool/sentinel.ts Outdated
@hoodmane hoodmane force-pushed the hoodmane/remove-emscripten-setup branch 4 times, most recently from 3341241 to 421fb9d Compare April 28, 2026 00:51
Comment thread src/pyodide/internal/pool/builtin_wrappers.ts
@hoodmane hoodmane force-pushed the hoodmane/remove-emscripten-setup branch from 421fb9d to 6071578 Compare April 28, 2026 17:43
Instead import `setupEmscripten.ts` directly. As requested by jasnell because
SetupEmscripten does not work with new module registry.
@hoodmane hoodmane force-pushed the hoodmane/remove-emscripten-setup branch from 6071578 to 98ad560 Compare April 28, 2026 18:43
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.56%. Comparing base (b2e22f4) to head (98ad560).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6658      +/-   ##
==========================================
- Coverage   66.57%   66.56%   -0.02%     
==========================================
  Files         403      402       -1     
  Lines      115958   115889      -69     
  Branches    19416    19404      -12     
==========================================
- Hits        77198    77137      -61     
+ Misses      27182    27180       -2     
+ Partials    11578    11572       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hoodmane hoodmane merged commit 3fa4f91 into main Apr 28, 2026
22 of 23 checks passed
@hoodmane hoodmane deleted the hoodmane/remove-emscripten-setup branch April 28, 2026 19:37
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.

5 participants