Add install-emscripten CLI command #243

bulenty584 · 2025-10-15T22:44:33Z

Add new 'pyodide xbuildenv install-emscripten' command
Add clone_emscripten() method to clone emsdk repository
Add install_emscripten() method to install and activate Emscripten SDK
Add tests for new functionality

Message echoed after installation complete for user to show the path to the emsdk_env.sh so that user can activate it manually in their shell.

- Add new 'pyodide xbuildenv install-emscripten' command - Add clone_emscripten() method to clone emsdk repository - Add install_emscripten() method to install and activate Emscripten SDK - Add tests for new functionality

for more information, see https://pre-commit.ci

hoodmane · 2025-10-16T01:03:08Z

pyodide_build/xbuildenv.py

+                "--embedded",
+                "--build=Release",


What's --embedded and --build=Release do?

I believe the --embedded flag tells emsdk to not modify your global shell startup files or system env. Essentially it places env variables into emsdk_env.sh so the user can call source on it later. The --build=Release flag causes emsdk to build or fetch pre-built tools in release mode instead of debug mode. Not quite sure if either is necessary for pyodide's build but I kept the commands identical to @ryanking13's shared script just to be safe.

I believe the --embedded flag tells emsdk to not modify your global shell startup files or system env.

This sounds like the default behavior. I think release build is also the default? Anyways we can leave it as is, but I normally would do this without either of these flags.

hoodmane · 2025-10-16T01:03:31Z

pyodide_build/xbuildenv.py

+        )
+
+        # Apply patches from xbuildenv/emsdk/patches directory to upstream/emscripten
+        # Patches are applied after install but before activate


It doesn't matter if they are applied before or after activate.

Noted and removed!

pyodide_build/tests/test_install_emscripten.py

hoodmane · 2025-10-16T01:04:34Z

Thanks for working on this @bulenty584!

…in test_install_emscripten.py removed as well

for more information, see https://pre-commit.ci

pyodide_build/tests/test_install_emscripten.py

…nd some basic reformatting

ryanking13

Thanks for working on this. Could you also update the integration test?

Currently we are installing Emscripten in CI for integration test, but I think we can replace this with this new command to test if this new command works correctly.

pyodide_build/cli/xbuildenv.py

ryanking13 · 2025-10-17T05:02:07Z

pyodide_build/cli/xbuildenv.py

+    emsdk_dir = manager.install_emscripten(version)
+
+    print("Installing emsdk complete.")
+    print(f"Use `source {emsdk_dir}/emsdk_env.sh` to set up the environment.")


Instead of requiring people to manually source the directory, it would be nice to detect the emscripten installation directory and use it during the build.

Let's do that in a separate PR to reduce the diff, for now, it looks good.

pyodide_build/xbuildenv.py

ryanking13 · 2025-10-17T05:09:55Z

pyodide_build/xbuildenv.py

+
+        # Apply patches from xbuildenv/emsdk/patches directory to upstream/emscripten
+        subprocess.run(
+            f"cat {patches_dir}/*.patch | patch -p1 --verbose",


Note that the patch might fail is that emscripten version passed by the user is different from the version that the patch is generated. I think we ned to gracefully catch those errors and show proper error message.

Ah I must have overlooked that. I added some error handling for this case as well.

pyodide_build/tests/test_install_emscripten.py

pyodide_build/xbuildenv.py

ryanking13 · 2025-10-17T05:19:13Z

pyodide_build/tests/test_cli_install_emscripten.py

+    # Verify subprocess calls
+    # install_emscripten makes 4 calls: clone + install + patch + activate


As these are already tested in test_install_emscripten.py, I think these are unnecessary. Testing the outputs should be enough. Same for all other functions in this file.

Yes after re-examining them I agree. Removed as well.

1. Changed default version to `get_build_flag("PYODIDE_EMSCRIPTEN_VERSION")` 2. Updated `clone_emscripten` to `_clone_emscripten` 3. Removed and edited redundant tests in both new test files 4. Updated patch path in `xbuildenv.py` and added new command to CI workflow

for more information, see https://pre-commit.ci

ryanking13

Thanks! code wise looks good to me overall. I left some minor comments. Also, I added integration label to trigger integration tests.

pyodide_build/cli/xbuildenv.py

.github/workflows/main.yml

Removed the get_build_flag command from xbuildenv cli and added default behavior as none

pyodide_build/cli/xbuildenv.py

ryanking13 · 2025-10-20T09:53:42Z

.github/workflows/main.yml

+
+      - name: Activate emsdk
+        run: |
+          source ${{ env.EMSDK_CACHE_FOLDER }}/emsdk_env.sh


The emscripten will not be installed in env.EMSDK_CACHE_FOLDER, as env.EMSDK_CACHE_FOLDER is used for mymindstorm/setup-emsdk not for pyodide xbuildenv command.

You can get the pyodide root directory inside the xbuildenv using

pyodide config get pyodide-root

then, the path to the emsdk_env.sh would be.

$(pyodide config get pyodide-root)/../emsdk/emsdk_env.sh

Ah ok I see. Still wrapping my head around the env as a whole. Edited in the latest commit!

Oops. sorry it should be pyodide_root not pyodide-root (underscore)

Ah, sorry. Fixed now!

ryanking13 · 2025-10-20T09:57:46Z

.github/workflows/main.yml

        uses: actions/cache@v4
        with:
          path: ${{ env.EMSDK_CACHE_FOLDER }}
          key: ${{ env.EMSDK_CACHE_NUMBER }}-${{ env.EMSCRIPTEN_VERSION }}-${{ runner.os }}


Let's remove this emsdk caching job too. We can add back caching when we find a good way to cache pyodide-build.

Ok, removed from my latest commit.

2. emsdk caching job removed in main.yml 3. install-emscripten help message also edited

ryanking13 · 2025-10-21T13:20:43Z

Also there is a failure in the unittest

'NoneType' object has no attribute 'splitlines'

2. Fixed pyodide-root to pyodide_root in integration tests

for more information, see https://pre-commit.ci

bulenty584 and others added 2 commits October 15, 2025 15:39

Add install-emscripten CLI command

29ae702

- Add new 'pyodide xbuildenv install-emscripten' command - Add clone_emscripten() method to clone emsdk repository - Add install_emscripten() method to install and activate Emscripten SDK - Add tests for new functionality

[pre-commit.ci] auto fixes from pre-commit.com hooks

d71ae08

for more information, see https://pre-commit.ci

hoodmane reviewed Oct 16, 2025

View reviewed changes

ryanking13 self-requested a review October 16, 2025 12:56

bulenty584 and others added 2 commits October 16, 2025 09:41

Emsdk installation comments fixed in xbuildenv.py and unneeded tests …

ea08570

…in test_install_emscripten.py removed as well

[pre-commit.ci] auto fixes from pre-commit.com hooks

d86a11d

for more information, see https://pre-commit.ci

hoodmane reviewed Oct 16, 2025

View reviewed changes

pyodide_build/tests/test_install_emscripten.py Outdated Show resolved Hide resolved

TestInstallEmscripten class removed from test_install_emscripten.py a…

8f20c44

…nd some basic reformatting

ryanking13 reviewed Oct 17, 2025

View reviewed changes

bulenty584 and others added 2 commits October 17, 2025 11:16

[pre-commit.ci] auto fixes from pre-commit.com hooks

709ef84

for more information, see https://pre-commit.ci

ryanking13 added the integration This PR will run the integration tests. This label can be used as a persistent marker to do so. label Oct 19, 2025

ryanking13 reviewed Oct 19, 2025

View reviewed changes

pyodide_build/cli/xbuildenv.py Outdated Show resolved Hide resolved

.github/workflows/main.yml Outdated Show resolved Hide resolved

Fixed integration test command to use xbuildenv and activate emsdk

7b4736b

Removed the get_build_flag command from xbuildenv cli and added default behavior as none

ryanking13 reviewed Oct 20, 2025

View reviewed changes

bulenty584 and others added 2 commits October 20, 2025 08:49

1. emsdk activation directory changed in main.yml

83fddb9

2. emsdk caching job removed in main.yml 3. install-emscripten help message also edited

Merge branch 'main' into feat/add-install-emscripten-cli

41ffece

bulenty584 and others added 2 commits October 21, 2025 09:31

1. Fixed unittests to follow new code changes

abfe731

2. Fixed pyodide-root to pyodide_root in integration tests

[pre-commit.ci] auto fixes from pre-commit.com hooks

1fd6457

for more information, see https://pre-commit.ci

		# Verify subprocess calls
		# install_emscripten makes 4 calls: clone + install + patch + activate

Add install-emscripten CLI command #243

Are you sure you want to change the base?

Add install-emscripten CLI command #243

Conversation

bulenty584 commented Oct 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoodmane commented Oct 16, 2025

Uh oh!

Uh oh!

ryanking13 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanking13 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanking13 commented Oct 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants