Support emscripten (pygbag + pyodide) in the meson buildconfig #3588
Conversation
📝 WalkthroughWalkthroughAdds Pyodide CI alongside Pygbag, updates the WASM SDK and build commands, makes dev.py WASM-aware with a Meson cross-file generator and WASM path changes, adds Meson emscripten variants and option, consolidates emscripten C extensions, renames/init-symbols in C sources, updates freetype include, and bumps build-tool constraints. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Actions as GitHub Actions
participant Repo as Repository
participant SDK as Wasm SDK
participant DevPy as dev.py
participant Meson as Meson/Ninja
participant Art as Artifacts
rect rgb(236,248,255)
note over Actions: build-pygbag (emscripten-pygbag)
Actions->>Repo: checkout
Actions->>SDK: setup SDK (python3.13-wasm)
Actions->>DevPy: `${SDKROOT}/python3-wasm dev.py build --wheel`
DevPy->>DevPy: detect wasm, write wasm.cross
DevPy->>Meson: configure with --cross-file=wasm.cross -> build (emscripten-pygbag)
Meson-->>DevPy: wheel produced
Actions->>Art: upload emsdk-wheels
end
rect rgb(245,236,255)
note over Actions: build-pyodide (cibuildwheel)
Actions->>Repo: checkout
Actions->>Actions: run cibuildwheel (pyodide cp313)
Actions->>Actions: run before-build (embuilder + settings.js)
Actions->>Art: upload pyodide-wheels
end
sequenceDiagram
autonumber
participant DevPy as dev.py
participant Sys as sysconfig
participant FS as SDKROOT
participant Meson as Meson
DevPy->>Sys: read HOST_GNU_TYPE
alt platform is emscripten / wasm
DevPy->>DevPy: wasm set, host_gnu_type assigned
DevPy->>FS: get_wasm_cross_file(SDKROOT) -> write wasm.cross
DevPy->>Meson: configure with --cross-file=wasm.cross (skip deps)
Meson-->>DevPy: build single extension module (emscripten path)
else native build
DevPy->>Meson: normal configure (install deps)
Meson-->>DevPy: build per-module targets (native path)
end
note right of DevPy: editable builds abort early on wasm
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ankith26
force-pushed
the
ankith26-meson-wasm
branch
2 times, most recently
from
eedb48b to
febe9a1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
dev.py (4)
39-51: Guard WASM cross-file/args and handle unsupported WASI explicitlyComputing
wasm_cross_fileandwasm_argsunconditionally risks pointing to a non-existent file (e.g.,meson-cross-.iniwhen not WASM, ormeson-cross-wasi.iniwhich doesn’t exist here). Build will fail silently later.
- Only compute/use these when
wasm == 'emscripten'.- Fail fast for
wasm == 'wasi'(or allow selecting type via a flag/env).Apply this diff:
-host_gnu_type = sysconfig.get_config_var("HOST_GNU_TYPE") -if isinstance(host_gnu_type, str) and "wasm" in host_gnu_type: - wasm = "wasi" if "wasi" in host_gnu_type else "emscripten" -else: - wasm = "" - -wasm_cross_file = (source_tree / "buildconfig" / f"meson-cross-{wasm}.ini").resolve() -wasm_args = [ - f"-Csetup-args=--cross-file={wasm_cross_file}", - "-Csetup-args=-Demscripten_type=pygbag", -] +host_gnu_type = sysconfig.get_config_var("HOST_GNU_TYPE") +if isinstance(host_gnu_type, str) and "wasm" in host_gnu_type: + wasm = "wasi" if "wasi" in host_gnu_type else "emscripten" +else: + wasm = "" + +wasm_args: list[str] = [] +if wasm == "emscripten": + wasm_cross_file = (source_tree / "buildconfig" / "meson-cross-emscripten.ini").resolve() + wasm_args = [ + f"-Csetup-args=--cross-file={wasm_cross_file}", + "-Csetup-args=-Demscripten_type=pygbag", + ] +elif wasm == "wasi": + pprint("WASI is not supported by this buildconfig yet. Please use emscripten.", Colors.RED) + sys.exit(1)
206-211: Fail gracefully when SDKROOT is missing in WASM modeAccessing
os.environ["SDKROOT"]will KeyError if the environment isn’t set correctly, leading to a cryptic crash.Apply this diff:
- self.py: Path = ( - Path(os.environ["SDKROOT"]) / "python3-wasm" - if wasm - else Path(sys.executable) - ) + if wasm: + sdkroot = os.environ.get("SDKROOT") + if not sdkroot: + pprint("SDKROOT is not set but WASM was detected. Set SDKROOT to your python-wasm SDK root.", Colors.RED) + sys.exit(1) + self.py = Path(sdkroot) / "python3-wasm" + else: + self.py = Path(sys.executable)
516-522: PATH injection: skip non-existent dirs and preserve orderPrepending blindly may add junk paths on custom layouts. Also, prefer emscripten upstream dir before devices bin.
Apply this diff:
- if wasm: - for wasm_path in [ - self.py.parent / "devices" / "emsdk" / "usr" / "bin", - self.py.parent / "emsdk" / "upstream" / "emscripten", - ]: - os.environ["PATH"] = f"{wasm_path}{os.pathsep}{os.environ['PATH']}" + if wasm: + for wasm_path in [ + self.py.parent / "emsdk" / "upstream" / "emscripten", + self.py.parent / "devices" / "emsdk" / "usr" / "bin", + ]: + if wasm_path.exists(): + os.environ["PATH"] = f"{wasm_path}{os.pathsep}{os.environ['PATH']}"
535-538: Skip pip version check/upgrade on WASMIn SDK environments this can be slow or fail; since you don’t install deps on WASM, you can skip the pip check/upgrade too.
Apply this diff:
- pprint("Checking pip version") - pip_v = cmd_run([self.py, "-m", "pip", "-V"], capture_output=True) + if not wasm: + pprint("Checking pip version") + pip_v = cmd_run([self.py, "-m", "pip", "-V"], capture_output=True)And guard the subsequent pip upgrade block under
if not wasm:.src_c/meson.build (3)
1-22: Emscripten static lib path: good direction; confirm .pyx in static_library works across Meson versionsBuilding a single static
pygameis correct for pygbag. Two follow-ups:
- Meson cython support with
.pyxinsidestatic_library()varies by Meson/Cython versions; please confirm CI uses a Meson that treatscythonas a first-class language for non-py.extension_moduletargets.- Consider moving
'pgcompat.c'out ofcython_filestobase_filesto keep typed lists clean.Proposed tidy:
-base_files = ['base.c', 'bitmask.c', 'SDL_gfx/SDL_gfxPrimitives.c'] -cython_files = [ +base_files = ['base.c', 'bitmask.c', 'SDL_gfx/SDL_gfxPrimitives.c', 'pgcompat.c'] +cython_files = [ 'cython/pygame/_sdl2/audio.pyx', 'cython/pygame/_sdl2/mixer.pyx', 'cython/pygame/_sdl2/sdl2.pyx', 'cython/pygame/_sdl2/video.pyx', - 'pgcompat.c' ]
12-21: Optional deps: tie inclusion to options to avoid link-time surprisesYou always add
sdl_image_dep,sdl_mixer_dep,sdl_ttf_dep,freetype_dep. When options are disabled or libs absent, some toolchains produce empty/“disabler” deps that still end up in the list. Safer to filter byget_option(...)or*_dep.found().Apply this diff:
- dependencies: pg_base_deps + [sdl_image_dep, sdl_mixer_dep, sdl_ttf_dep, freetype_dep], + dependencies: pg_base_deps + + (get_option('image') ? [sdl_image_dep] : []) + + (get_option('mixer') ? [sdl_mixer_dep] : []) + + (get_option('font') ? [sdl_ttf_dep, freetype_dep] : []),
23-24: Style: place comment on its own lineMeson allows trailing comments, but
else # regular buildis brittle in diffs. Non-blocking nit.-else # regular build +else + # regular build
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/build-emsdk.yml(1 hunks)buildconfig/meson-cross-emscripten.ini(1 hunks)dev.py(7 hunks)meson.build(6 hunks)meson_options.txt(1 hunks)src_c/meson.build(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- meson_options.txt
- buildconfig/meson-cross-emscripten.ini
- .github/workflows/build-emsdk.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: arm64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
🔇 Additional comments (8)
dev.py (2)
253-261: Nice: clear error for editable installs on WASMGood user-facing guard; avoids a confusing partial install.
288-290: Plumb WASM args only when setThis is safe now, but after guarding
wasm_argsabove, ensure we only extend when non-empty.Suggested tiny tweak:
- if wasm: + if wasm_args: install_args.extend(wasm_args)src_c/meson.build (1)
480-481: LGTM: explicit endif commentHelpful for long files.
meson.build (5)
36-38: Good: make emscripten build selectable via option
plat = 'emscripten-@0@'sets the ground for multiple flavors. LGTM.
79-80: Mixer variant name for pygbag: confirm library nameUsing
SDL2_mixer_oggis reasonable for pygbag SDKs; ensure the PC/lib name matches the shipped artifact to avoid fallback failures.If the actual lib is
SDL2_mixerwith built-in OGG, consider using a cross property or option to switch names rather than hard-coding.
255-262: Nice: include wasm-specific lib dirsIncluding
lib/wasm32-emscriptenand/picimproves resolution across SDK variants. LGTM.
323-333: Good: skip portmidi resolution on emscriptenAvoids unnecessary lookups and simplifies the wasm build.
448-464: Docs/tests/stubs gating for emscripten is appropriatePrevents cross-environment build steps from running where they can’t execute. LGTM.
|
PyScript maintainer here, we were surprised by latest Pyodide inability to use pygame-ce so we do hope this can be merged sooner than later and see this wonderful package usable again as our users got confused (as much as ourselves) too, thank you! |
ankith26
force-pushed
the
ankith26-meson-wasm
branch
4 times, most recently
from
08a7efb to
bb249a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
buildconfig/meson-cross-emscripten.ini (1)
1-12: Add cross properties for more robust Meson behaviorExplicit properties improve portability and avoid Meson trying to run build-time code with the target. Consider:
- needs_exe_wrapper = true (explicit)
- cmake, strip fallbacks (optional)
[host_machine] system = 'emscripten' cpu_family = 'wasm32' cpu = 'wasm' endian = 'little' +[properties] +needs_exe_wrapper = true + [binaries] c = 'emcc' cpp = 'em++' ar = 'emar' strip = 'emstrip' exe_wrapper = 'node'Do you want to also expose an
sdkrootproperty here (to be consumed by meson.build) if you need to reference SDK paths in future?meson.build (1)
37-37: Guard SDL API on emscriptenEmscripten ports are SDL2-first. Building with
-Dsdl_api=3likely breaks. Add a fast-fail until SDL3 support is verified.elif host_machine.system() == 'emscripten' plat = 'emscripten-@0@'.format(get_option('emscripten_type')) + if get_option('sdl_api') != 2 + error('Emscripten builds currently require -Dsdl_api=2') + endifIf SDL3 on Emscripten is intended, please point to the tested Emscripten ports settings and I’ll adapt the guards accordingly.
pyproject.toml (1)
100-106: Pyodide block: good start; add minimal smoke test or skip rationaleAn empty
test-commandcan mask regressions. If tests aren’t feasible yet, add a short smoke (e.g., import pygame; print(version)) or document intentional skip.Example:
-[tool.cibuildwheel.pyodide] -build = "cp313-*" # build only for the latest python version. -# taken from pyodide recipe at https://github.com/pyodide/pyodide-recipes/blob/main/packages/pygame-ce/meta.yaml -before-build = "embuilder build sdl2 sdl2_ttf sdl2_image sdl2_mixer sdl2_gfx libjpeg libpng giflib harfbuzz vorbis mpg123 libmodplug freetype libhtml5 --pic" -test-command = "" # TODO: fix runtime issues and then figure out how to test +[tool.cibuildwheel.pyodide] +build = "cp313-*" +# taken from pyodide recipe at https://github.com/pyodide/pyodide-recipes/blob/main/packages/pygame-ce/meta.yaml +before-build = "embuilder build sdl2 sdl2_ttf sdl2_image sdl2_mixer sdl2_gfx libjpeg libpng giflib harfbuzz vorbis mpg123 libmodplug freetype libhtml5 --pic" +# Minimal smoke test; replace with real tests when possible. +test-command = "python - <<'PY'\nimport pygame, sys; print(pygame.__version__); sys.exit(0)\nPY"Confirm cibuildwheel version in CI supports the
pyodideplatform block you’re using.dev.py (3)
39-51: Build WASM args lazily to avoid invalid cross-file when not on WASMAvoid computing a bogus meson-cross-.ini path when wasm is false; initialize only when needed.
Apply:
-# we will assume dev.py wasm builds are made for pygbag. -host_gnu_type = sysconfig.get_config_var("HOST_GNU_TYPE") -if isinstance(host_gnu_type, str) and "wasm" in host_gnu_type: - wasm = "wasi" if "wasi" in host_gnu_type else "emscripten" -else: - wasm = "" - -wasm_cross_file = (source_tree / "buildconfig" / f"meson-cross-{wasm}.ini").resolve() -wasm_args = [ - f"-Csetup-args=--cross-file={wasm_cross_file}", - "-Csetup-args=-Demscripten_type=pygbag", -] +# we will assume dev.py wasm builds are made for pygbag. +wasm: str = "" +host_gnu_type = sysconfig.get_config_var("HOST_GNU_TYPE") +if isinstance(host_gnu_type, str) and "wasm" in host_gnu_type: + wasm = "wasi" if "wasi" in host_gnu_type else "emscripten" + +wasm_cross_file: Path | None = None +wasm_args: list[str] = [] +if wasm: + wasm_cross_file = (source_tree / "buildconfig" / f"meson-cross-{wasm}.ini").resolve() + wasm_args = [ + f"-Csetup-args=--cross-file={wasm_cross_file}", + "-Csetup-args=-Demscripten_type=pygbag", + ]
206-211: Avoid hard dependency on SDKROOT; use current interpreter pathRelying on SDKROOT can KeyError. Using sys.executable is robust (and equals python3-wasm in CI).
- self.py: Path = ( - Path(os.environ["SDKROOT"]) / "python3-wasm" - if wasm - else Path(sys.executable) - ) + self.py: Path = Path(sys.executable)
516-522: Add existence checks before PATH injectionGuard against non-existent directories to keep PATH clean.
- if wasm: - for wasm_path in [ - self.py.parent / "emsdk" / "upstream" / "emscripten", - self.py.parent / "devices" / "emsdk" / "usr" / "bin", - ]: - os.environ["PATH"] = f"{wasm_path}{os.pathsep}{os.environ['PATH']}" + if wasm: + for wasm_path in [ + self.py.parent / "emsdk" / "upstream" / "emscripten", + self.py.parent / "devices" / "emsdk" / "usr" / "bin", + ]: + if wasm_path.exists(): + os.environ["PATH"] = f"{wasm_path}{os.pathsep}{os.environ['PATH']}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src_c/freetype/ft_freetype.his excluded by!src_c/freetype/*src_c/freetype/ft_wrap.his excluded by!src_c/freetype/*
📒 Files selected for processing (10)
.github/workflows/build-emsdk.yml(1 hunks)buildconfig/meson-cross-emscripten.ini(1 hunks)dev.py(7 hunks)meson.build(4 hunks)meson_options.txt(1 hunks)pyproject.toml(1 hunks)src_c/_freetype.c(1 hunks)src_c/meson.build(4 hunks)src_c/mixer.c(1 hunks)src_py/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src_c/_freetype.c
- src_c/mixer.c
- meson_options.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/meson.build
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: x86_64
- GitHub Check: arm64
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: Pyodide build
- GitHub Check: dev-check
🔇 Additional comments (12)
meson.build (2)
458-474: Nice: skip docs/tests on emscripten buildsAvoids long-running steps and native execution requirements under cross builds.
37-37: Optionemscripten_typeis defined — meson_options.txt declares this combo (choices ['pygbag','generic'], default 'generic'), soget_option('emscripten_type')is valid.src_c/meson.build (2)
23-25: Keep else branch intactNo changes needed here; regular build path remains consistent.
354-385: Good: exclude camera on emscriptenAvoids platform-only code under WASM.
src_py/__init__.py (1)
118-131: Safer WASM guard looks goodAutomated symbol and code scans didn’t locate
import_cython; manually verify that your WASM static lib’spygame.baseactually exportsimport_cython..github/workflows/build-emsdk.yml (3)
54-54: LGTM: unified build entrypointSwitching to
${SDKROOT}/python3-wasm dev.py build --wheelkeeps the build logic single-sourced in dev.py.
63-76: Pyodide cibuildwheel configuration verified pyproject.toml includes a [tool.cibuildwheel.pyodide] section and the workflow uploads wheelhouse/*.whl.
39-42: Approve SDK bumpAsset is available (HTTP 200).
dev.py (4)
14-14: LGTM: sysconfig importNeeded for HOST_GNU_TYPE detection.
254-261: Good guard: block editable installs on WASMClear error path; avoids an unsupported mode.
288-290: LGTM: meson cross and emscripten args inclusionOnly applied under WASM; safe for native builds.
535-538: LGTM: skip Python deps on WASMPrevents accidental native dependency resolution.
|
If things work out I would also like to get this in by our next release 2.5.6 which should be happening soon. My goal for this PR is to lay down the buildconfig support so that both pygbag and pyodide can benefit from this. I just updated this PR and got compilation to work with pyodide locally. I am facing some runtime issues with the generated wheels on pyodide, and I don't know much about emscripten/pyodide to fix those errors. The current error is I have also started work on adding this to our CI though there is some compilation error I don't see locally: @ryanking13 hello! I believe you maintain pygame-ce for pyodide. I would appreciate your help with moving this PR ahead. |
|
Some more explanations about the changes in this PR: All |
|
Actually pygbag since 0.9.0 uses a shared library in a wheel, but the intermediate fully statically linked is important in CI to avoid any runtime link errors that were quite common in emscripten early days when enabling various size optimisations / DCE. It is quite easy to make a shared lib from the full static one without recompiling anything. Afaik the major difference beetween python-wasm / pyodide wheels is that pygbag link SDL2/3 and OpenGL/ES into the pygame dynamic library directly and force JS glue code of pymain to embed full support for SDL/GL/GLES/OpenAL/GLFW calls without any sort of code elimination. Pyodide main must be quite different since it cannot load eg panda3d / raylib wheels without missing some symbols. @ankith26 I will test that PR coming week. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: build (macos-14)
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Pyodide build
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: dev-check
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: build (ubuntu-22.04)
|
Currently the pyodide wheel in the CI artifact has dynamic linking issue. The |
|
The same issue exists in the |
ankith26
force-pushed
the
ankith26-meson-wasm
branch
from
b57cb89 to
5e2eb3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
meson_options.txt (1)
44-45: Consider documenting the emscripten_type options.While the option is well-structured, consider adding a comment above it explaining the difference between 'pygbag' and 'generic' variants to help maintainers understand when to use each.
Add documentation like this:
+# Specify the type of emscripten build being done. +# 'pygbag': For pygame-web/python-wasm-sdk builds (uses static linking) +# 'generic': For generic emscripten/pyodide builds (uses WASM exceptions) option('emscripten_type', type: 'combo', choices: ['pygbag', 'generic'], value: 'generic')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/build-emsdk.yml(1 hunks)dev.py(6 hunks)meson.build(4 hunks)meson_options.txt(1 hunks)pyproject.toml(1 hunks)src_c/meson.build(4 hunks)src_c/mixer.c(1 hunks)src_py/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src_c/mixer.c
- pyproject.toml
- src_c/meson.build
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: arm64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: Pyodide build
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: dev-check
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: x86
- GitHub Check: AMD64
🔇 Additional comments (7)
src_py/__init__.py (1)
118-118: LGTM! Proper attribute check for static builds.The addition of
hasattr(pygame_static, 'import_cython')is a good defensive check that ensures the static module has the required method before attempting to call it. This prevents potential AttributeError exceptions.meson.build (6)
37-37: LGTM! Dynamic platform variant for emscripten.The dynamic platform value using
emscripten_typeoption properly enables different emscripten build configurations.
90-107: Consider extracting hardcoded paths to configuration.The
/opt/python-wasm-sdkpaths are hardcoded. While this works for the current CI setup, it reduces portability for other build environments.Based on the previous review comment, consider making these paths configurable via cross properties:
if plat == 'emscripten-pygbag' + # Get SDK root from cross file, with fallback to default location + sdkroot = meson.get_cross_property('sdkroot', '/opt/python-wasm-sdk') add_global_arguments('-DBUILD_STATIC=1', language: 'c') sdl_dep = declare_dependency( link_args: ['-lSDL2'], )
302-358: Generic dependency fallback overwrites emscripten-specific dependencies.These generic SDL and freetype dependency lookups will execute even for emscripten builds, potentially overwriting the carefully configured emscripten-specific dependencies defined earlier.
Wrap this entire block in a guard:
+if not plat.startswith('emscripten') # TODO: add version constraints? sdl_dep = dependency(sdl, required: false) if not sdl_dep.found() sdl_dep = declare_dependency( include_directories: pg_inc_dirs, dependencies: cc.find_library(sdl, dirs: pg_lib_dirs), ) endif # ... rest of the dependency declarations ... freetype_dep = dependency('freetype2', required: false) if not freetype_dep.found() freetype_dep = declare_dependency( include_directories: pg_inc_dirs, dependencies: cc.find_library( 'freetype', dirs: pg_lib_dirs, required: get_option('freetype'), ), ) endif +endif # not emscripten
362-362: LGTM! Proper emscripten exclusion for portmidi.The condition correctly excludes emscripten platforms from portmidi dependency resolution, which is appropriate since emscripten doesn't support MIDI.
486-486: LGTM! Documentation build correctly skipped for emscripten.The condition properly excludes emscripten platforms from documentation and test builds, which is appropriate for WASM targets.
136-136: Critical: Missing dependency declarations for non-emscripten platforms.The
elseblock starting here doesn't contain any dependency declarations. This means that for non-emscripten platforms, the SDL and freetype dependencies won't be properly declared before the generic fallback block at lines 302-358.The structure should be:
-else +endif # emscripten if plat == 'win' and host_machine.cpu_family().startswith('x86')Then move the
endif # emscriptento after line 358 to properly guard the generic fallback block.
|
Can you retry with the latest CI artifact? I think those linker errors should be gone now. |
|
Thanks. The linking errors are gone, but now I am experiencing I'll try to debug with the wheel and the code changes in this PR more. |
|
So, I did some debugging, and here is what I found so far. The problem happens in resolving the global SDL uses a static global variable I think it happens because of three different reasons combined:
Because of this, the following situation happens.
... So, how to fix? Here are some possible options that I can think of.
1.a. removing - static SDL_VideoDevice *_this = NULL;
+ SDL_VideoDevice *_this = NULL;This seems to make the memory address shared by multiple modules. 1.b. hiding probablematic symbols [Not verified] Hiding symbols such as
Building SDL as a shared library will avoid exporting symbols from multiple binaries. (probably related issue: pyodide/pyodide#5887) |
ankith26
force-pushed
the
ankith26-meson-wasm
branch
from
5e2eb3a to
30a4ca0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build-emsdk.yml (1)
53-61: CI step looks good; consider caching the SDK archive to speed up runsOptional: cache /opt/python-wasm-sdk by keying on $SDK_VERSION/$SDK_ARCHIVE to avoid repeated downloads.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/build-emsdk.yml(1 hunks)dev.py(6 hunks)meson.build(4 hunks)meson_options.txt(1 hunks)pyproject.toml(2 hunks)src_c/base.c(4 hunks)src_c/meson.build(4 hunks)src_c/mixer.c(2 hunks)src_py/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- meson_options.txt
- src_c/mixer.c
- src_py/init.py
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/meson.build
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: dev-check
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-14)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: aarch64
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: x86_64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: Pyodide build
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
🔇 Additional comments (9)
dev.py (4)
288-295: Good guard: editable install is not supported on WASMClear error and early exit LGTM.
322-329: Nice: write cross file into build dir and pass via --cross-fileThis integrates cleanly with Meson’s setup-args flow.
568-571: Early-return from dependency install on WASMPragmatic choice to avoid accidental host installs. LGTM.
198-234: Cross file is missing link flags and has a brittle Node path
- Without c_link_args/cpp_link_args, emcc won’t retain SDL/FreeType ports; only include paths are passed. Add the Emscripten link flags here so linkers pull the right archives.
- Hard-coding node/22.16.0_64bit is fragile. Resolve the first node/*/bin dynamically or fall back to plain 'node' if present in PATH.
- Optional: expose sdkroot via cross properties for future Meson use.
Apply:
def get_wasm_cross_file(sdkroot: Path): emsdk_dir = sdkroot / "emsdk" bin_dir = emsdk_dir / "upstream" / "emscripten" - node_bin_dir = emsdk_dir / "node" / "22.16.0_64bit" / "bin" + # pick first available node/*/bin, else fallback to 'node' + node_candidates = sorted((emsdk_dir / "node").glob("*/bin/node")) + node_path = node_candidates[0] if node_candidates else "node" sysroot_dir = bin_dir / "cache" / "sysroot" @@ lib_dirs = [lib_dir] c_args = [f"-I{x}" for x in inc_dirs] + [f"-L{x}" for x in lib_dirs] + link_args = [ + "-sUSE_SDL=2", + "-sUSE_SDL_TTF=2", + "-sUSE_SDL_MIXER=2", + "-sUSE_FREETYPE=1", + "-lSDL2", + "-lhtml5", + ] return f""" [host_machine] @@ [binaries] c = {str(bin_dir / 'emcc')!r} cpp = {str(bin_dir / 'em++')!r} ar = {str(bin_dir / 'emar')!r} strip = {str(bin_dir / 'emstrip')!r} -exe_wrapper = {str(node_bin_dir / 'node')!r} +exe_wrapper = {str(node_path)!r} [project options] emscripten_type = 'pygbag' [built-in options] c_args = {c_args!r} +c_link_args = {link_args!r} +cpp_link_args = {link_args!r} + +[properties] +sdkroot = {str(sdkroot)!r} """src_c/meson.build (1)
3-9: Skip pygame._sdl2 Cython modules when building against SDL3Only include the _sdl2 cython modules if sdl_api != 3 to avoid mismatches under SDL3.
-cython_files = [ - 'cython/pygame/_sdl2/audio.pyx', - 'cython/pygame/_sdl2/mixer.pyx', - 'cython/pygame/_sdl2/sdl2.pyx', - 'cython/pygame/_sdl2/video.pyx', -] +cython_files = ['pgcompat.c'] +if sdl_api != 3 + cython_files += [ + 'cython/pygame/_sdl2/audio.pyx', + 'cython/pygame/_sdl2/mixer.pyx', + 'cython/pygame/_sdl2/sdl2.pyx', + 'cython/pygame/_sdl2/video.pyx', + ] +endifmeson.build (2)
300-356: Guard the generic SDL/FreeType fallback to avoid clobbering Emscripten settingsCurrently this block runs for emscripten too and can override the emscripten-specific declare_dependency you set above.
-# TODO: add version constraints? -sdl_dep = dependency(sdl, required: false) +if not plat.startswith('emscripten') + # TODO: add version constraints? + sdl_dep = dependency(sdl, required: false) if not sdl_dep.found() sdl_dep = declare_dependency( include_directories: pg_inc_dirs, dependencies: cc.find_library(sdl, dirs: pg_lib_dirs), ) endif @@ -endif +endif # not emscriptenRepeat the same wrap for sdl_image_dep, sdl_mixer_dep, sdl_ttf_dep, and freetype_dep in this block.
485-501: Docs/tests install correctly skipped on EmscriptenThis avoids unnecessary work and reduces wheel size. LGTM.
.github/workflows/build-emsdk.yml (1)
63-76: Pyodide job: minimal and fineUsing cibuildwheel with CIBW_PLATFORM=pyodide is aligned with the new meson path. LGTM.
src_c/base.c (1)
2423-2468: Verify new PyInit symbols and module names
- Confirm that mixer.c defines PyInit_pg_mixer and that importing pygame.mixer still resolves correctly.
- Confirm that the SDL2 Cython extensions export PyInit_audio and PyInit_video matching their module filenames.
|
Thanks for investigating and finding out the root cause of that issue. Well, I suppose the easiest solution is to go back to doing a single (shared library) build on our side. That way there is only one I tested I suppose the only thing left to do here is to fix the png loading issue. For now libpng is skipped: when I tried adding it there were some issues related to its setjmp/longjmp usage. This PR currently links against |
Yes, that's also fine as there will not be the same symbols exported from multiplie modules. Thanks for handling it!
That's awesome. In pyodide, we've been removing the docs directory as well to reduce the size further. |
Passing |
|
But EDIT: I got a fix for it |
ankith26
force-pushed
the
ankith26-meson-wasm
branch
from
30a4ca0 to
5c6d82d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
dev.py (1)
198-238: Missing critical Emscripten link flags in the cross-file.The cross-file only sets include paths via
c_argsand library search paths viac_link_args. However, it's missing the essential-sUSE_*Emscripten flags that instruct emcc to pull in the SDL/FreeType static ports. Without these, the linker will drop all SDL/FreeType symbols, causing import-time failures (as reported in PR comments).Apply this diff to add the required link flags:
c_args = [ f"-I{x}" for x in [ inc_dir / "SDL2", inc_dir / "freetype2", sdkroot / "devices" / "emsdk" / "usr" / "include" / "SDL2", ] ] - c_link_args = [f"-L{lib_dir}"] + c_link_args = [ + f"-L{lib_dir}", + "-sUSE_SDL=2", + "-sUSE_FREETYPE=1", + "-sUSE_SDL_MIXER=2", + "-sUSE_SDL_TTF=2", + ] return f"""Based on past review comments.
src_c/meson.build (1)
1-19: Emscripten single-build consolidation looks correct, but missing SDL3 guard.The condition
plat.startswith('emscripten')correctly covers both pygbag and pyodide variants. The single shared extension module approach aligns with the PR's goal to resolve SDL global state issues by consolidating all C extensions into one module.However, the
cython_fileslist includes SDL2-specific modules (_sdl2/*.pyx) without checking ifsdl_api == 3. According to past review comments and the regular build path (lines 322-349), SDL2 modules should be skipped when building for SDL3.Apply this diff to add the SDL3 guard:
base_files = ['base.c', 'bitmask.c', 'rotozoom.c', 'SDL_gfx/SDL_gfxPrimitives.c'] -cython_files = [ - 'cython/pygame/_sdl2/audio.pyx', - 'cython/pygame/_sdl2/mixer.pyx', - 'cython/pygame/_sdl2/sdl2.pyx', - 'cython/pygame/_sdl2/video.pyx', -] +cython_files = [] +if sdl_api != 3 + cython_files += [ + 'cython/pygame/_sdl2/audio.pyx', + 'cython/pygame/_sdl2/mixer.pyx', + 'cython/pygame/_sdl2/sdl2.pyx', + 'cython/pygame/_sdl2/video.pyx', + ] +endifBased on past review comments.
🧹 Nitpick comments (3)
dev.py (1)
202-203: Use a glob pattern for the node path to handle SDK version changes.The current code assumes a single node version directory exists. As noted by pmp-p in past comments, emsdk may use different custom node builds for each SDK version.
The current
sorted(...)[0]approach works but could fail silently if multiple node versions exist. Consider adding a fallback or validation:node_matches = sorted(emsdk_dir.glob("node/*/bin/node")) - node_path = node_matches[0] if node_matches else Path("node") + node_path = node_matches[-1] if node_matches else Path("node") # use latestBased on past review comments.
meson.build (1)
107-143: Pyodide dependencies include critical flags but use SDK-specific library names.The pyodide variant correctly enables wasm exceptions and longjmp support (line 110), and uses
-sUSE_SDL=2and-sUSE_FREETYPE=1flags to pull in Emscripten ports. However, line 124 uses-lpng-legacysjljand line 138 uses-lfreetype-legacysjlj, which are SDK-specific library names that may not be portable across different pyodide/emscripten SDK versions.As noted in past review comments, prefer the standard
-lpngand-lfreetypenames, relying on the-sUSE_FREETYPE=1flag to handle the correct variant:sdl_image_dep = declare_dependency( link_args: [ '-lSDL2_image-bmp-gif-jpg-lbm-pcx-png-pnm-qoi-svg-tga-xcf-xpm-xv-wasm-sjlj', '-ljpeg', - '-lpng-legacysjlj', + '-lpng', ], )freetype_dep = declare_dependency( compile_args: freetype_flags, - link_args: freetype_flags + ['-lfreetype-legacysjlj', '-lharfbuzz'] + link_args: freetype_flags + ['-lfreetype', '-lharfbuzz'] )Based on past review comments.
.github/workflows/build-emsdk.yml (1)
35-42: Remove unused PYBUILD environment variable
ThePYBUILDenv var isn’t referenced anywhere in the build scripts or config and should be removed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/build-emsdk.yml(1 hunks)dev.py(6 hunks)meson.build(4 hunks)meson_options.txt(1 hunks)pyproject.toml(2 hunks)src_c/meson.build(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- meson_options.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: x86_64
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: dev-check
- GitHub Check: build (windows-latest)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: build (macos-14)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (13)
dev.py (5)
39-44: WASM detection looks correct.The logic correctly detects both emscripten and wasi variants from
HOST_GNU_TYPEand provides a fallback to an empty string for non-WASM builds.
243-247: Python path switching logic is sound.When
wasmis detected, the script correctly switches to the WASM-wrapped Python interpreter fromSDKROOT. Otherwise, it uses the system Python.
293-299: Appropriate guard for editable WASM builds.Editable installs are not currently supported on WASM, and the early exit with a clear error message guides users to use
--wheelinstead.
327-333: Cross-file generation and usage in build is correct.The build directory is created, the cross-file is written, and the
--cross-fileargument is correctly passed to Meson via setup-args.
573-575: Early return for WASM dependency installation is appropriate.Since WASM builds use pre-built SDK dependencies, skipping the normal pip dependency installation is correct.
src_c/meson.build (1)
21-478: Regular build structure is preserved correctly.The explicit
elsebranch and closingendif # regular buildcomment clearly separate the emscripten and regular build paths, maintaining all existing per-module builds for non-emscripten targets.meson.build (5)
37-37: Dynamic platform assignment for emscripten is correct.The
platvariable correctly incorporates theemscripten_typeoption (pygbag/pyodide) to create distinct platform identifiers (emscripten-pygbag,emscripten-pyodide), enabling variant-specific logic downstream.
144-367: Non-emscripten fallback block is correctly guarded.The
elsebranch (line 144) and correspondingendif # emscripten(line 367) properly isolate the generic SDL/FreeType dependency resolution logic from the emscripten-specific declarations, preventing the overwrites flagged in past reviews.
370-370: Portmidi guard for emscripten is correct.Adding
and not plat.startswith('emscripten')ensures portmidi dependencies are not declared on emscripten builds, which don't support MIDI.
494-494: Docs and stripped build guard for emscripten is appropriate.Skipping documentation and example installation on emscripten builds reduces wheel size and avoids unnecessary overhead, aligning with the stripped build strategy.
90-106: Please set SDKROOT and verify pygbag SDK libraries
Re-run the library-check script with SDKROOT pointing to your pygbag SDK root so that$SDKROOT/emsdk/upstream/emscripten/cache/sysroot/lib/wasm32-emscripten/picis inspected for libSDL2.a, libSDL2_image.a, libSDL2_mixer_ogg.a (or libSDL2_mixer.a), libogg.a, libvorbis.a, libfreetype.a, libharfbuzz.a, and libSDL2_ttf.a. If any are missing or named differently, adjust thelink_argsor add explicit-Lsearch paths..github/workflows/build-emsdk.yml (2)
54-54: Build command simplification is a good improvement.The consolidated single-command build (
${SDKROOT}/python3-wasm dev.py build --wheel) simplifies the workflow by moving build logic into dev.py. This aligns with the PR's goal to streamline emscripten/pygbag support.
63-76: Confirm runner selection for Pyodide jobWe’ve verified the
[tool.cibuildwheel.pyodide]section exists in pyproject.toml. The only remaining question is that the Pyodide job usesruns-on: ubuntu-latestwhile the Pygbag job usesubuntu-22.04. Please confirm this difference is intentional and align the runners if not.
ankith26
force-pushed
the
ankith26-meson-wasm
branch
2 times, most recently
from
29b9a8f to
62ed850
Compare
ankith26
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
dev.py (1)
209-217: Add Emscripten link flags to pull in SDL and FreeType static portsThe cross file currently only provides include and library search paths but omits the
-sUSE_*link flags that tell Emscripten to pull in the static SDL/FreeType ports. Without these, the linker will drop SDL/FreeType dependencies at link time, causing runtime import failures (as noted in PR comments aboutFT_Init_FreeTypeand SDL_mixer missing symbols).Apply this diff to add the required link flags:
c_args = [ f"-I{x}" for x in [ inc_dir / "SDL2", inc_dir / "freetype2", sdkroot / "devices" / "emsdk" / "usr" / "include" / "SDL2", ] ] - c_link_args = [f"-L{lib_dir}"] + c_link_args = [ + f"-L{lib_dir}", + "-sUSE_SDL=2", + "-sUSE_FREETYPE=1", + "-sUSE_SDL_MIXER=2", + "-sUSE_SDL_TTF=2", + ]Also add
cpp_link_args = {c_link_args!r}to the returned string (line 237) to ensure C++ link steps also receive these flags.src_c/meson.build (1)
4-9: Gate SDL2 Cython modules when building for SDL3The
cython_fileslist unconditionally includes_sdl2modules, but these should be excluded whensdl_api == 3(as done in the regular build at lines 346-349). The emscripten path currently lacks this guard.Apply this diff to conditionally include SDL2 modules:
base_files = ['base.c', 'bitmask.c', 'rotozoom.c', 'SDL_gfx/SDL_gfxPrimitives.c'] -cython_files = [ - 'cython/pygame/_sdl2/audio.pyx', - 'cython/pygame/_sdl2/mixer.pyx', - 'cython/pygame/_sdl2/sdl2.pyx', - 'cython/pygame/_sdl2/video.pyx', -] +cython_files = [] +if sdl_api != 3 + cython_files += [ + 'cython/pygame/_sdl2/audio.pyx', + 'cython/pygame/_sdl2/mixer.pyx', + 'cython/pygame/_sdl2/sdl2.pyx', + 'cython/pygame/_sdl2/video.pyx', + ] +endif
🧹 Nitpick comments (1)
meson.build (1)
124-124: Document or replace Emscripten-specific –legacysjlj library variantsmeson.build (lines 124, 138): the flags
-lpng-legacysjljand-lfreetype-legacysjljare toolchain-specific variants generated for legacy SJLJ exception mode. For broader portability, switch to-lpngand-lfreetypewith-sSUPPORT_LONGJMP=wasm, or add a comment explaining why these specific builds are required.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src_c/freetype/ft_freetype.his excluded by!src_c/freetype/*src_c/freetype/ft_wrap.his excluded by!src_c/freetype/*
📒 Files selected for processing (9)
.github/workflows/build-emsdk.yml(1 hunks)dev.py(6 hunks)meson.build(4 hunks)meson_options.txt(1 hunks)pyproject.toml(2 hunks)src_c/_freetype.c(1 hunks)src_c/base.c(4 hunks)src_c/meson.build(2 hunks)src_c/mixer.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- .github/workflows/build-emsdk.yml
- meson_options.txt
- src_c/_freetype.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Pyodide build
- GitHub Check: dev-check
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (14)
dev.py (4)
39-44: LGTM! Clean WASM detection logicThe WASM detection using
sysconfig.HOST_GNU_TYPEis appropriate and handles both WASI and Emscripten variants correctly.
295-301: LGTM! Appropriate guard for WASM editable installsBlocking editable installs on WASM with a clear error message is the right approach, as WASM builds require full wheel packaging.
329-335: LGTM! Clean cross-file integrationThe cross-file generation and integration with Meson setup args is well-structured. The path handling correctly derives the SDK root from
self.py.parent.
575-577: LGTM! Correct dependency handling for WASMSkipping standard dependency installation for WASM is correct, as WASM builds rely on SDK-provided dependencies rather than PyPI packages.
meson.build (5)
36-37: LGTM! Clean platform determination for Emscripten variantsThe platform string construction correctly incorporates the
emscripten_typeoption to create variant-specific identifiers (emscripten-pygbag, emscripten-pyodide).
90-106: LGTM! Clean pygbag dependency declarationsThe pygbag variant correctly declares minimal static library dependencies with appropriate link arguments. The dependency chain (sdl_ttf_dep → freetype_dep) is properly expressed.
144-367: LGTM! Correct guard structure for dependency fallbacksThe dependency fallback block is properly guarded within the
elsebranch (lines 144-367), ensuring it only executes for non-Emscripten builds. The Emscripten-specific dependencies (lines 90-143) are kept separate and won't be overwritten.
370-370: LGTM! Correct portmidi guard for EmscriptenExcluding portmidi dependency discovery for Emscripten builds is appropriate, as MIDI is not supported in the browser environment.
494-494: LGTM! Appropriate docs/tests exclusion for EmscriptenSkipping docs, tests, and examples for Emscripten builds is correct. This reduces wheel size (critical for WASM) and avoids potential issues with doc generation in the WASM environment.
src_c/mixer.c (1)
2052-2057: LGTM! Conditional module initialization symbol handles static build requirements.The conditional definition of the module initialization symbol correctly prevents name collisions in static builds. When
BUILD_STATICis defined,pg_mixeris used as the entry point; otherwise,mixeris used. This aligns with the corresponding changes insrc_c/base.c(line 2425) wherePyInit_pg_mixer()is declared and referenced (line 2620).src_c/base.c (4)
2425-2425: LGTM! Entry point renamed to avoid collision in static builds.The renaming of
PyInit_mixertoPyInit_pg_mixeris consistent with the conditional initialization insrc_c/mixer.c(lines 2052-2057), wherepg_mixeris used as the module init symbol whenBUILD_STATICis defined.
2467-2473: LGTM! Entry points renamed for _sdl2 submodules.The new declarations reflect the reorganization of module initialization:
- Line 2467:
PyInit_mixernow refers to_sdl2.mixer(distinct from the mainpg_mixerat line 2425).- Line 2470:
PyInit_sdl2_audiorenamed toPyInit_audiofor_sdl2.audio.- Line 2473:
PyInit_sdl2_videorenamed toPyInit_videofor_sdl2.video.These changes prevent symbol collisions and align with the static build requirements for emscripten/pygbag.
2552-2554: LGTM! Submodule loading calls updated with renamed entry points.The
load_submodule_mphasecalls now use the renamed initialization functions:
- Line 2552:
PyInit_mixer()for_sdl2.mixer.- Line 2553:
PyInit_audio()for_sdl2.audio.- Line 2554:
PyInit_video()for_sdl2.video.These are consistent with the declarations at lines 2467, 2470, and 2473.
2620-2620: LGTM! Main mixer module loading updated to usePyInit_pg_mixer.The call to
load_submodulenow usesPyInit_pg_mixer(), which matches the renamed declaration at line 2425 and the conditional initialization insrc_c/mixer.c(lines 2052-2057).
Oh yeah, that's a good point. Probably it should be reported to Emscripten so we can pass those flags to embuilder. The current patch looks okay to me. Another workaround that I use in that situation is to build a dummy source file with all the ports enabled. It will trigger building all the ports. # create dummy c file
touch temp.c
# build dummy file, which ports enabled.
emcc temp.c -sSUPPORT_LONGJMP="wasm" -fpic --use-port=sdl2_image |
ankith26
force-pushed
the
ankith26-meson-wasm
branch
from
62ed850 to
5d59620
Compare
|
My fix is also pretty hacky but I guess it'll do for now... Anyways, I should be done with this PR from my end. I have decided to push running tests on our CI to a future PR in the interest of getting this PR merged quicker (+ I see the infra to run tests is already available on pyodide recipe repo). |
|
I can confirm that the current Pyodide wheel works well with Pyodide 0.28.3. Thanks! There seems like a little bit of performance issue in Chrome, but I think it is a problem in Pyodide side, not Pygame, let me handle that. Thanks for fixing the Emscripten build! |
This is my take on @pmp-p 's PR at pmp-p#12
dev.pyupdated to handle this. It usespython-wasm-sdkand makes a custom wheel with a single shared library build ofsrc_cand the pure python contents ofsrc_py.pyodide buildnow works out of box (provided dependencies are installed as done inbefore-buildattribute in[tool.cibuildwheel.pyodide]section ofpyproject.toml). Added CI to test building, however running unit tests on CI is still out of scope of this PR, I may look at it in a future PR.@pmp-p I would appreciate your review on this PR + help with testing it. If this PR works out, pygbag would need to be updated accordingly to handle the wheel file this PR can build.