review: address pre-merge feedback on emscripten target

Working through reviewer-flagged items before landing:

* Windows: `emcc` is invoked via the full path resolved by
  `which::which("emcc")` instead of the bare string "emcc". Stashed
  on the Build struct in `step_check_for_emcc` and threaded through
  `emcc_link` and `emcc_post_link`. The Rust process spawner doesn't
  honour PATHEXT for bare names, so `Command::new("emcc")` couldn't
  find `emcc.bat` on Windows; using the full path works since Rust
  1.77's CVE-2024-24576 fix added explicit .bat handling for full paths.

* tests: `prepend_path` (the `EMSCRIPTEN_BIN` companion helper, kept
  as a future debugging hook) now uses `std::env::join_paths` instead
  of a hardcoded `:` separator. Was a trap for the same Windows port.

* tests: `make_node_driver` JSON-encodes the .mjs URL when interpolating
  it into the JS source, so Windows-style paths with backslashes can't
  break out of the JS string literal.

* template: bumped `wasm-bindgen = "0.2"` to `"0.2.122"` so new
  projects scaffolded via `wasm-pack new --emscripten` pick up the
  emscripten output-mode fixes (wasm-bindgen#5156). Older versions
  produce broken JS/wasm under the emscripten target.

* docs: corrected stale comment that referenced an "EmscriptenRuntime"
  interface decoration. The decoration was deliberately scaled back to
  not claim runtime members (`HEAP*`, `FS`, `ccall`/`cwrap`) on
  the factory return; the comment now reflects what the code actually
  does.

* style: collapsed `Target::Bundler`, `Web`, and `Deno` post-link
  settings into a single match arm with an explanatory comment. They
  were three identical struct literals.

Author: guybedford

src/command/build.rs

@@ -45,6 +45,13 @@ pub struct Build {
     pub panic_unwind: bool,
     target_triple: String,
     wasm_path: Option<String>,
+    /// Path to the `emcc` binary as resolved by `step_check_for_emcc`,
+    /// reused by the subsequent link/post-link steps. Resolved via
+    /// `which::which("emcc")` so that on Windows the `PATHEXT` lookup
+    /// finds `emcc.bat` — `Command::new("emcc")` would not, because
+    /// Rust's process spawner doesn't honour `PATHEXT`. Always `Some`
+    /// in the emscripten pipeline by the time the link step runs.
+    emcc_path: Option<PathBuf>,
 }
 
 /// What sort of output we're going to be generating and flags we're invoking
@@ -304,6 +311,7 @@ impl Build {
             extra_options,
             panic_unwind: build_opts.panic_unwind,
             wasm_path: None,
+            emcc_path: None,
         })
     }
 
@@ -633,6 +641,12 @@ impl Build {
         // older than 3.1.60 will likely fail at build time, so we warn
         // up front for clearer diagnostics.
         warn_if_emcc_too_old(&emcc_path);
+
+        // Stash the resolved path for the subsequent link / post-link
+        // steps. On Windows, emcc is `emcc.bat`; `which::which` honours
+        // `PATHEXT` but `Command::new("emcc")` would not, so we must
+        // invoke via the full resolved path.
+        self.emcc_path = Some(emcc_path);
         Ok(())
     }
 
@@ -665,8 +679,12 @@ impl Build {
             .as_ref()
             .context("step_emcc_link called before cargo build set wasm_path")?
             .clone();
+        let emcc = self
+            .emcc_path
+            .as_ref()
+            .context("step_emcc_link called before step_check_for_emcc set emcc_path")?;
         let out_wasm = self.intermediate_wasm_path();
-        emcc_link(&lib_path, &out_wasm)?;
+        emcc_link(emcc, &lib_path, &out_wasm)?;
         // step_run_wasm_bindgen reads `wasm_path` as its input — point it at
         // the freshly-linked wasm so wasm-bindgen processes the right file.
         self.wasm_path = Some(out_wasm.to_string_lossy().into_owned());
@@ -725,7 +743,12 @@ impl Build {
         // we linked arbitrary C/C++ exports, which the wasm-pack flow
         // doesn't support today. Avoiding it also sidesteps the emcc
         // assertion on wasm-bindgen-style multi-value-return exports.
+        let emcc = self
+            .emcc_path
+            .as_ref()
+            .context("step_emcc_post_link called before step_check_for_emcc set emcc_path")?;
         emcc_post_link(
+            emcc,
             &in_wasm,
             &in_library,
             extern_pre.as_deref(),
@@ -736,10 +759,14 @@ impl Build {
         info!("emcc --post-link produced {out_js:?}.");
 
         // Decorate wasm-bindgen's bare .d.ts with the emscripten-shaped
-        // factory: an `EmscriptenRuntime` interface for the runtime members
-        // we surface, a `MainModule` intersection type, and a default-export
-        // factory declaration. After this `import M from "./<name>.mjs"`
-        // type-checks against the produced module.
+        // factory: a `MainModule` alias for the wasm-bindgen surface and a
+        // default-export factory declaration. After this
+        // `import M from "./<name>.mjs"` type-checks against the produced
+        // module. We deliberately don't claim emscripten runtime members
+        // (`HEAP*`, `FS`, `ccall`/`cwrap`) on the factory return — they
+        // exist on the underlying module but aren't part of the typed API
+        // wasm-pack ships; users who need them can extend the `.d.ts` in
+        // pkg/.
         if !self.disable_dts && bindgen_dts.exists() {
             decorate_bindgen_dts_for_emscripten(&bindgen_dts)?;
         }
@@ -968,7 +995,11 @@ fn emcc_missing_install_message() -> String {
 /// generate any JS runtime. `-sEXPORTED_FUNCTIONS=` is populated by
 /// scanning the staticlib for the symbols wasm-bindgen needs so they
 /// survive dead-stripping.
-fn emcc_link(lib_path: &str, out_wasm: &PathBuf) -> Result<()> {
+///
+/// `emcc` is the resolved emcc binary path (via `which::which`), not the
+/// bare string `"emcc"` — `Command::new("emcc")` doesn't honour `PATHEXT`
+/// on Windows where emcc ships as `emcc.bat`.
+fn emcc_link(emcc: &Path, lib_path: &str, out_wasm: &PathBuf) -> Result<()> {
     if let Some(parent) = out_wasm.parent() {
         std::fs::create_dir_all(parent)
             .with_context(|| format!("creating intermediate dir {parent:?}"))?;
@@ -977,7 +1008,7 @@ fn emcc_link(lib_path: &str, out_wasm: &PathBuf) -> Result<()> {
     let exports = collect_wasm_bindgen_exports(lib_path)?;
     let exports_joined = exports.join(",");
 
-    let mut cmd = Command::new("emcc");
+    let mut cmd = Command::new(emcc);
     cmd.arg(lib_path)
         .arg("--no-entry")
         .arg("--oformat=bare")
@@ -1064,7 +1095,12 @@ fn emcc_post_link_settings_for(target: Target) -> Result<EmccPostLinkSettings> {
     // Node — useful for tests, CLIs, and SSR. The `nodejs` target stays
     // node-only since that's its explicit intent.
     Ok(match target {
-        Target::Bundler | Target::Web => EmccPostLinkSettings {
+        // `Bundler`, `Web`, and `Deno` collapse to the same emcc settings
+        // — `web,node` ENVIRONMENT, no source-phase imports, `.mjs` —
+        // because the differentiation between them happens earlier
+        // (wasm-bindgen produces target-specific JS glue) and emcc's
+        // post-link sees an already-finished module.
+        Target::Bundler | Target::Web | Target::Deno => EmccPostLinkSettings {
             environment: "web,node",
             source_phase_imports: false,
             extension: "mjs",
@@ -1079,11 +1115,6 @@ fn emcc_post_link_settings_for(target: Target) -> Result<EmccPostLinkSettings> {
             source_phase_imports: false,
             extension: "mjs",
         },
-        Target::Deno => EmccPostLinkSettings {
-            environment: "web,node",
-            source_phase_imports: false,
-            extension: "mjs",
-        },
         Target::NoModules => bail!(
             "`--target no-modules` is not supported for wasm32-unknown-emscripten builds. \
              The emscripten toolchain produces module-shaped output only. \
@@ -1127,15 +1158,19 @@ fn emcc_opt_level_for(profile: &BuildProfile) -> &'static str {
 /// where ESM `import` statements must live. wasm-bindgen routes module
 /// imports there because they can't legally appear inside emcc's
 /// `--js-library` content (that gets evaluated inside the wrapper).
+///
+/// `emcc` is the resolved emcc binary path (via `which::which`); see
+/// `emcc_link` for the Windows-`PATHEXT` rationale.
 fn emcc_post_link(
+    emcc: &Path,
     in_wasm: &Path,
     in_library: &Path,
     extern_pre: Option<&Path>,
     out_js: &Path,
     settings: &EmccPostLinkSettings,
     opt_level: &str,
 ) -> Result<()> {
-    let mut cmd = Command::new("emcc");
+    let mut cmd = Command::new(emcc);
     cmd.arg("--post-link")
         .arg(in_wasm)
         .arg("--js-library")

tests/all/emscripten.rs

@@ -54,15 +54,15 @@ fn patched_emcc_bin() -> Option<std::path::PathBuf> {
 
 /// Prepend `dir` to `$PATH` on `cmd`. Companion to `patched_emcc_bin` for
 /// injecting an alternative emcc directory ahead of the inherited PATH.
+/// Uses `std::env::join_paths` so the platform-correct separator (`;` on
+/// Windows, `:` elsewhere) is applied.
 #[allow(dead_code)]
 fn prepend_path(cmd: &mut Command, dir: &Path) {
     let existing = std::env::var_os("PATH").unwrap_or_default();
-    let mut new_path = std::ffi::OsString::from(dir);
-    if !existing.is_empty() {
-        new_path.push(":");
-        new_path.push(existing);
-    }
-    cmd.env("PATH", new_path);
+    let mut entries = vec![dir.to_path_buf()];
+    entries.extend(std::env::split_paths(&existing));
+    let joined = std::env::join_paths(entries).expect("PATH entries should be valid");
+    cmd.env("PATH", joined);
 }
 
 /// Skip the calling test (with an explanatory message) if emcc isn't
@@ -87,10 +87,14 @@ macro_rules! skip_without_emcc {
 /// one node invocation so we catch interactions between different codegen
 /// paths (e.g. heap reallocation invalidating cached views).
 fn make_node_driver(mjs_path: &Path) -> String {
+    // JSON-encode the path so backslashes (Windows) and unusual characters
+    // can't break out of the JS string literal.
+    let mjs_json = serde_json::to_string(&format!("file://{}", mjs_path.display()))
+        .expect("path should serialise");
     format!(
         r#"
         globalThis.rs_test_doubler = (n) => n * 2;
-        const {{ default: M }} = await import('file://{mjs}');
+        const {{ default: M }} = await import({mjs});
         const m = await M();
 
         function expect(name, got, want) {{
@@ -148,7 +152,7 @@ fn make_node_driver(mjs_path: &Path) -> String {
         const calc = new m.app.math.Calc(21);
         expect('Calc.double',                calc.double(),                      42);
         "#,
-        mjs = mjs_path.display(),
+        mjs = mjs_json,
     )
 }
 

wasm-pack-emscripten-template/Cargo.toml

@@ -10,7 +10,10 @@ edition = "2021"
 crate-type = ["staticlib"]
 
 [dependencies]
-wasm-bindgen = "0.2"
+# 0.2.122 is the floor that ships the emscripten output-mode fixes
+# (wasm-bindgen#5156); older releases will produce broken JS/wasm under
+# the emscripten target.
+wasm-bindgen = "0.2.122"
 
 [profile.release]
 # Tell `rustc` to optimize for small code size; emcc will further optimize at