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!

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

pyodide_build/tests/test_install_emscripten.py

+from pyodide_build.xbuildenv import CrossBuildEnvManager
+
+
+class TestInstallEmscripten:


This seems like a lot more tests than we need.

I felt that as well but wanted to be thorough. Do you mean to get rid of all the tests or to keep some?

My latest commit has the tests file stripped of some of the bloated cases, let me know if this works better!

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

hoodmane · 2025-10-16T17:12:03Z

pyodide_build/tests/test_install_emscripten.py

+from pyodide_build.xbuildenv import CrossBuildEnvManager
+
+
+class TestInstallEmscripten:


Remove the class? You don't seem to use it for anything.

Ah yeah, you are completely correct. Removed!

…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.

ryanking13 · 2025-10-17T04:59:45Z

pyodide_build/cli/xbuildenv.py

+@app.command("install-emscripten")
+def _install_emscripten(
+    version: str = typer.Option(
+        "latest", help="Emscripten SDK Version (default: latest)"


The default should be the one in the xbuildenv. Pyodide requires the same Emscripten version that is used to build the Pyodide runtime.

You can call get_build_flag("PYODIDE_EMSCRIPTEN_VERSION") to get the proper version.

Yup, added!

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.

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

pyodide_build/xbuildenv.py

+
+        xbuild_root = self.symlink_dir.resolve()
+        emsdk_dir = xbuild_root / "emsdk"
+        patches_dir = emsdk_dir / "patches"


I think the patches should be under self.pyodide_root / "emsdk" / "patches"

Ah yes, just saw that in the file as well. Edited in the new commit.

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.

ryanking13 · 2025-10-17T05:14:53Z

pyodide_build/tests/test_install_emscripten.py

+    )
+
+
+def test_install_emscripten_patch_application(tmp_path, monkeypatch):


This test looks redundant as the full pipeline is already tested in the previous tests.

Noted and changed!

ryanking13 · 2025-10-17T05:15:21Z

pyodide_build/tests/test_install_emscripten.py

+        manager.install_emscripten()
+
+
+def test_install_emscripten_patch_application_sequence(tmp_path, monkeypatch):


This also looks a bit redundant

ryanking13 · 2025-10-17T05:15:49Z

pyodide_build/xbuildenv.py


        return version

+    def clone_emscripten(self, emsdk_dir: Path | str | None = None) -> Path:


Let's make this function private, to avoid using it directly outside.

Suggested change

def clone_emscripten(self, emsdk_dir: Path | str | None = None) -> Path:

def _clone_emscripten(self, emsdk_dir: Path | str | None = None) -> Path:

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.

ryanking13 · 2025-10-19T03:43:46Z

pyodide_build/cli/xbuildenv.py

+    version: str = typer.Option(
+        get_build_flag("PYODIDE_EMSCRIPTEN_VERSION"),
+        help=f"Emscripten SDK Version (default: ${get_build_flag('PYODIDE_EMSCRIPTEN_VERSION')})",


Let's not call this inside the function's default value. Calling get_build_flag requires installing xbuildenv and it is a heavy operation.

You can set the default to None, and call get_build_flag inside the function explicitly when the version is not given.

Oh ok, I took it out and added the default check in the function.

ryanking13 · 2025-10-19T03:50:13Z

.github/workflows/main.yml

-        with:
-          version: ${{ env.EMSCRIPTEN_VERSION }}
-          actions-cache-folder: ${{env.EMSDK_CACHE_FOLDER}}
+        run: pyodide install-emscripten


This should be pyodide xbuildenv install-emscripten.

Also as the current implementation does not activate the emsdk after running this command. It should be called manually.

Ah yes, totally my bad. Added both in now!

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

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

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

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

		from pyodide_build.xbuildenv import CrossBuildEnvManager


		class TestInstallEmscripten:

		)


		def test_install_emscripten_patch_application(tmp_path, monkeypatch):

		manager.install_emscripten()


		def test_install_emscripten_patch_application_sequence(tmp_path, monkeypatch):


		return version

		def clone_emscripten(self, emsdk_dir: Path \| str \| None = None) -> Path:

	def clone_emscripten(self, emsdk_dir: Path \| str \| None = None) -> Path:
	def _clone_emscripten(self, emsdk_dir: Path \| str \| None = None) -> Path:

		# 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!

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!

hoodmane commented Oct 16, 2025

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

ryanking13 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants