Remove 'wasm-pack' Build Dependency by rhammonds-teleport · Pull Request #58618 · gravitational/teleport

rhammonds-teleport · 2025-09-02T18:01:10Z

This PR removes our dependency on wasm-pack, which is holding us back from updating to recent releases of rust. This means that we have to roll our own automation to handle the work that wasm-pack was doing for us. Namely:

Installing the wasm32-unknown-unknown toolchain
Ensuring the correct version of wasm-bindgen-cli is installed (must match the version of wasm-bindgen that the wasm library is compiled against)
Ensuring that wasm-opt is installed (in some environments wasm-opt already had to be installed out-of-band)
Building our wasm library
Optimizing the wasm build artifact for size
Executing wasm-bindgen to generate javascript/typescript bindings
Creating a package.json manifest

fheinecke

LGTM provided this builds

zmb3 · 2025-09-04T19:56:05Z

build.assets/Dockerfile

Should we try to pre-seed the dockerfile with these dependencies so that the make command is a no-op if the correct version is already present?

Sure. Are you thinking just the wasm-opt dependency, or should we try to guess the right wasm-bindgen-cli version as well? I say "guess" because we can't actually tell at this stage exactly which version we need, but we can hardcode the current version and try to remember to keep it up to date. Any drift will get fixed at build time.

Why can't we tell what version we need? Couldn't we run make -C .. print-wasm-bindgen-version and pass it in via an ARG?

I got my wires crossed.

Added install automation for both wasm-opt and wasm-bindgen to the buildbox and node images.

camscale · 2025-09-04T23:43:31Z

Makefile

 CARGO_TARGET_linux_arm64 := aarch64-unknown-linux-gnu
 CARGO_TARGET_linux_386 := i686-unknown-linux-gnu
 CARGO_TARGET_linux_amd64 := x86_64-unknown-linux-gnu
+CARGO_TARGET_wasm := wasm32-unknown-unknown


These are meant to be named with OS and ARCH - see how RUST_TARGET_ARCH is defined. You seem to be using this var for different purposes, in which case it should probably be named differently and not be co-located with these vars.

camscale · 2025-09-05T00:22:17Z

Makefile

+print-wasm-bindgen-version: NEED_VERSION = $(WASM_BINDGEN_VERSION)
+print-wasm-bindgen-version:
+	@echo $(NEED_VERSION)


No need for the NEED_VERSION var here, just print directly:

print-wasm-bindgen-version: @echo $(WASM_BINDGEN_VERSION)

camscale · 2025-09-05T06:16:27Z

Makefile

+	cargo build --package ironrdp --lib --target $(CARGO_TARGET_wasm) --release
+	wasm-opt target/$(CARGO_TARGET_wasm)/release/ironrdp.wasm -o target/$(CARGO_TARGET_wasm)/release/ironrdp.wasm -O
+	wasm-bindgen target/$(CARGO_TARGET_wasm)/release/ironrdp.wasm --out-dir $(ironrdp)/pkg --typescript --target web
+	@echo "*" > $(ironrdp)/pkg/.gitignore


Why is this being created in make? Can we just add /web/packages/shared/libs/ironrdp/pkg to the top-level .gitignore (or in web/.gitignore if preferred)?

wasm-pack would write out this same .gitignore, so I included it here for parity.
But you're right, there's no reason to keep it here.

camscale · 2025-09-05T06:22:24Z

Makefile

+	@printf '%s\n' '{' \
+	' "name": "ironrdp",' \
+	' "version": "0.1.0",' \
+	' "module": "ironrdp.js",' \
+	' "types": "ironrdp.d.ts",' \
+	' "files": ["ironrdp_bg.wasm","ironrdp.js","ironrdp.d.ts"],' \
+	' "sideEffects": ["./snippets/*"]' \
+	'}' > $(ironrdp)/pkg/package.json


Another way to do this which may be a bit easier to modify later:

define ironrdp_package_json { "name": "ironrdp", "version": "0.1.0", ... } endef export ironrdp_package_json

and in the recipe:

printenv ironrdp_package_json > $(ironrdp)/pkg/package.json

The source material is a little bit easier to edit as it does not need all those single quotes and backslashes, which are easy to get wrong. It's just bare json which is also easier to cut and paste.

camscale · 2025-09-05T06:32:28Z

build.assets/Dockerfile

Why can't we tell what version we need? Couldn't we run make -C .. print-wasm-bindgen-version and pass it in via an ARG?

* Replace potentially confusing 'CARGO_TARGET_wasm' variable. * Replace oneoff .gitignore with a corresponding entry in the top level .gitignore. * Replace 'printf' used to create package.json file with a cleaner multiline string. * Remove unnecessary 'NEED_VERSION' * Use awk to find wasm-bindgen version without using 'cargo' since it doesn't seem to be available on ARM64 runners

camscale · 2025-09-10T04:02:05Z

build.assets/versions.mk

 # Run lint-rust check locally before merging code after you bump this.
 RUST_VERSION ?= 1.81.0
 WASM_OPT_VERSION ?= 0.116.1
+WASM_BINDGEN_VERSION ?= $(shell $(MAKE) -C .. --no-print-directory  print-wasm-bindgen-version)


I'm not sure I like this here. Prior to this change, this version file could be used directly in other parts of the build when needed such as https://github.com/gravitational/teleport/blob/master/integrations/operator/Dockerfile.gha#L27 - that won't work now as versions.mk now depends on the top-level makefile and its relative location to it.

I would just lift this out into build.assets/Makefile as that already has those dependencies mentioned.

I realise now that Dockerfile will still work as it does not reference WASM_BINDGEN_VERSION, so it will never run that command or need that version. Still a little more complicated than the basic "set var to version string" so I think it's still best located in build.assets/Makefile but I'm not so worried about breaking other things now.

Our makefiles are a bit of a mess and could do with some unification, but things like this dynamic versions specified in other files is always going to be a bit hairy.

camscale · 2025-09-10T06:56:03Z

Makefile

+WASM_BINDGEN_VERSION = $(shell awk ' \
+  $$1 == "name" && $$3 == "\"wasm-bindgen\"" { in_pkg=1; next } \
+  in_pkg && $$1 == "version" { gsub(/"/, "", $$3); print $$3; exit } \
+' Cargo.lock)


I'm curious what was wrong with the last version - when I ran it locally it seemed to work (although it did require cargo be to be installed, the redirected error should have made it silent if it was not).

The reason I ask is that the awk that was previously there (similar to this awk) for some reason did not work on the 32-bit intel release (i386 arch) for inexplicable reasons and I still don't know what was wrong. I'm a little worried this will not work too since we never resolved it before and why we had to hard code the version. See the comment below on the ensure-wasm-bindgen target. If you've verified this awk works in the 386 release build, then great.

I had just assumed there was some issue with it since it was commented out and we were hardcoding 0.2.99. Purely by happenstance, this usage of awk seems to work fine on i386. I've been cutting dev builds to ensure that reviewers only see changes that produce a clean build.

zmb3 · 2025-09-11T16:39:08Z

Makefile

+.PHONY: ensure-wasm-opt
+ensure-wasm-opt: WASM_OPT_VERSION := $(shell $(MAKE) --no-print-directory -C build.assets print-wasm-opt-version)
+ensure-wasm-opt:
+	cargo install wasm-opt@$(WASM_OPT_VERSION)


Do we need --force or --locked here?

Ah yes, I missed the --locked flag here. I don't think we should add --force though since it defeats the purpose of seeding the build images by forcing reinstallation (even when the specified version is already installed).

* Get a Teleport build working without wasm-pack * * seed docker images with wasm-opt, and wasm-bindgen-cli * Replace potentially confusing 'CARGO_TARGET_wasm' variable. * Replace oneoff .gitignore with a corresponding entry in the top level .gitignore. * Replace 'printf' used to create package.json file with a cleaner multiline string. * Remove unnecessary 'NEED_VERSION' * Use awk to find wasm-bindgen version without using 'cargo' since it doesn't seem to be available on ARM64 runners * Move 'WASM_BINDGEN_VERSION' definition out of versions.mk * Pass '--locked' flag to wasm-opt install

rhammonds-teleport force-pushed the rhammonds/remove-webpack branch 4 times, most recently from fa3e667 to f7f427b Compare September 4, 2025 17:21

Get a Teleport build working without wasm-pack

eff99d8

rhammonds-teleport force-pushed the rhammonds/remove-webpack branch from f7f427b to eff99d8 Compare September 4, 2025 17:47

rhammonds-teleport marked this pull request as ready for review September 4, 2025 18:18

rhammonds-teleport requested review from camscale, doggydogworld, fheinecke, klizhentas, r0mant, rosstimothy, russjones and zmb3 as code owners September 4, 2025 18:18

github-actions bot requested review from bl-nero and ravicious September 4, 2025 18:18

github-actions bot added size/sm ui labels Sep 4, 2025

fheinecke approved these changes Sep 4, 2025

View reviewed changes

zmb3 reviewed Sep 4, 2025

View reviewed changes

camscale reviewed Sep 5, 2025

View reviewed changes

bl-nero approved these changes Sep 5, 2025

View reviewed changes

rhammonds-teleport added the no-changelog Indicates that a PR does not require a changelog entry label Sep 10, 2025

camscale reviewed Sep 10, 2025

View reviewed changes

Move 'WASM_BINDGEN_VERSION' definition out of versions.mk

2a58762

camscale approved these changes Sep 10, 2025

View reviewed changes

rhammonds-teleport requested a review from zmb3 September 11, 2025 15:59

zmb3 reviewed Sep 11, 2025

View reviewed changes

Pass '--locked' flag to wasm-opt install

e7f4f8b

zmb3 approved these changes Sep 12, 2025

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from ravicious September 12, 2025 22:22

rhammonds-teleport added this pull request to the merge queue Sep 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 16, 2025

rhammonds-teleport added this pull request to the merge queue Sep 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 16, 2025

Merge branch 'master' into rhammonds/remove-webpack

679a386

rhammonds-teleport enabled auto-merge September 16, 2025 16:43

rhammonds-teleport added this pull request to the merge queue Sep 16, 2025

Merged via the queue into master with commit 09f6f6c Sep 16, 2025
44 checks passed

rhammonds-teleport deleted the rhammonds/remove-webpack branch September 16, 2025 17:21

rosstimothy mentioned this pull request Sep 17, 2025

Remove wasm-pack #57304

Closed

rhammonds-teleport mentioned this pull request Dec 11, 2025

[v18] Remove 'wasm-pack' Build Dependency #62174

Merged

rhammonds-teleport mentioned this pull request Dec 12, 2025

[v17] Remove 'wasm-pack' Build Dependency #62214

Merged

chenrui333 mentioned this pull request Dec 14, 2025

teleport 18.5.1 Homebrew/homebrew-core#258448

Closed

Conversation

rhammonds-teleport commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rhammonds-teleport commented Sep 2, 2025 •

edited

Loading