EmccCompile: prefer TryRunProcess over RunShellCommand.

[tmds]

A shell is not needed to run emcc.

[maraf]

Thank you @tmds for the contribution!

There are test failures related to the change

src\mono\wasm\build\WasmApp.Common.targets(868,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) Failed to compile D:\a\_work\1\s\artifacts\bin\microsoft.netcore.app.runtime.browser-wasm\Release\runtimes\browser-wasm\native\src\driver.c -> D:\a\_work\1\s\artifacts\obj\System.Diagnostics.Tracing.Tests\Release\net11.0-browser\browser-wasm\wasm\for-publish\driver.o
An error occurred trying to start process 'D:\a\_work\1\s\src\mono\browser\emsdk\emscripten\emcc' with working directory 'D:\a\_work\1\s\src\libraries\System.Diagnostics.Tracing\tests'. The specified executable is not a valid application for this OS platform.

https://github.com/dotnet/runtime/pull/126150/checks?check_run_id=68749851592

[maraf]

I'm thinking if there wasn't reason to put the command in batch file, something like too long input

[dotnet-policy-service]

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

[pavelsavara]

There is problem with too long list of arguments.
This needs to be redesigned to use response files, similar to #123548

[tmds]

I'm thinking if there wasn't reason to put the command in batch file

It seems that on Windows emcc is invokable as emcc because of emcc.bat. You can't Process.Start a bat file. I've added some code to use cmd and a comment to explain.

There is problem with too long list of arguments.
This needs to be redesigned to use response files,

I think this is independent of the changes made here.

[tmds]

@maraf @pavelsavara please take another look.

[pavelsavara]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tmds]

@pavelsavara can you take another look?

[maraf]

I looked through the history here, and the shell wrapper appears to have been introduced for process execution reliability, not to avoid command-line length limits.

Relevant history:

• RunShellCommand was centralized in src/tasks/Common/Utils.cs in [mono] Fix race condition in Utils + remove duplicated code #46029 as shared process-launch code.
• EmccCompile started using it when the task was added in [wasm] Compile .bc->.o in parallel, before passing to the linker #54053.
• Later fixes like [wasi/wasm] EmccCompile.cs: quote the compiler path #97427 were specifically about quoting CompilerBinaryPath after switching to a custom emcc path in [wasi] Initial wasi/aot build support #95051.
• The helper also sets UTF-8 on Windows and uses /bin/sh on Unix, which fits the same purpose.

On the command-length question: storing the invocation in a .cmd/.sh file does not make it unlimited once executed.

• On Windows, cmd.exe still applies its 8191-character limit to commands run from batch files. Only a direct CreateProcess launch of an .exe gets the larger ~32767-char limit.
• On Unix-like systems, shell execution is still limited by ARG_MAX.

So the history supports that the wrapper was there for invoking emcc/wrapper scripts correctly across platforms, while the "too many arguments" concern is a separate issue and response files would still be the right fix for that.

[tmds]

@maraf that matches my understanding: wethether .NET or cmd launches a process, they are both subject to the same command length limit. The only way to pass something longer is by putting it in a response file.

The change made here is meant to eliminate the shell on non-Windows. On Windows, we still need cmd because emcc is invoked through a .bat file.

[tmds]

@pavelsavara @maraf is this good to merge?

[maraf]

@tmds I'm sorry for the delay!

Regarding the Windows invocation:

/c "chcp 65001 > nul && "C:\path with spaces\emcc.bat" <args>"

cmd.exe has a subtle rule for /c: it only strips the outer quotes when certain conditions hold (roughly: the string starts with ", and there are exactly two quote characters, or no special characters between them). With quoted paths plus quoted arguments there can be many inner quotes, and the parser’s behavior becomes hard to reason about — it is easy to end up with a command cmd interprets differently than intended.

A safer pattern is to pass /S /c "…". The /S flag forces cmd to always strip exactly one pair of outer quotes regardless of the inner quoting, which makes the command line robust when CompilerBinaryPath or any source path contains spaces or special characters.

[tmds]

@maraf is this good to merge?

[maraf]

@tmds Thank you for the contribution.

Unfortunately, we are reaching late phase of the release cycle and since we don't have enough test coverage in this area, we are not going to take the change in .NET 11. I'm happy to take it very early in 12, so that we have enough time to find potential side effects.

I know you had to wait on us often, but since the capacity of the team is limited, this was often a lower priority to get to.

[pavelsavara]

The new emscripten has slightly different layout and may influence this.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "58e76263fe342dcf15a9fe8809ab06ab0a0157ba",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "260b0cf26daca8937f4137bfdedb94776fbb5ad3",
  "last_reviewed_commit": "58e76263fe342dcf15a9fe8809ab06ab0a0157ba",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "260b0cf26daca8937f4137bfdedb94776fbb5ad3",
  "last_recorded_worker_run_id": "29681942628",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "58e76263fe342dcf15a9fe8809ab06ab0a0157ba",
      "review_id": 4730544926
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Justified. RunShellCommand wrapped every emcc invocation in a generated temporary .sh/.cmd script solely to run a single command; a shell is not needed to launch a process, so replacing it with a direct TryRunProcess call removes an unnecessary temp-file dependency and the last consumer of RunShellCommand, allowing that helper to be deleted.

Approach: Reasonable. The emcc launch now calls Utils.TryRunProcess directly, and on Windows wraps the emcc.bat batch file in cmd /S /c "chcp 65001 > nul && "<path>" <args>" to keep batch-file support and restore the UTF-8 path handling previously provided by the removed helper. The cmd /S single outer-quote-pair stripping and the chcp 65001 code-page switch are used correctly. The concern is that the enabling change in the shared TryRunProcess (forcing UTF-8 stdout/stderr encoding and always redirecting+closing stdin) is applied to every task that uses this helper, not just emcc.

Summary: ⚠️ Needs Human Review. The emcc-specific changes look correct, but the modifications to the shared Utils.TryRunProcess are cross-cutting and change behavior for many other build tasks (AOT compiler, bundle emitter, workload/package install, Apple SDK, Android, etc.). A human familiar with the mono/wasm build tasks should confirm the unconditional UTF-8 output decoding and stdin redirection are safe for all those callers. The PR is also currently labeled NO-MERGE.

---

Detailed Findings

✅ Refactor correctness — emcc invocation and helper removal

RunShellCommand was the sole remaining consumer of that helper, and its removal (Utils.cs) is clean. The Windows path reconstructs the previous script behavior directly: cmd /S /c "chcp 65001 > nul && "<CompilerBinaryPath>" <args>". With /S, cmd strips only the outermost quote pair, so the inner quoted compiler path and the chcp prefix are preserved correctly. The non-Windows path invokes the compiler binary directly, which is appropriate.

⚠️ Cross-cutting change — forced UTF-8 stdout/stderr encoding for all callers (Utils.cs)

StandardOutputEncoding/StandardErrorEncoding are now unconditionally set to Encoding.UTF8 in TryRunProcess, which is shared by many tasks (MonoAOTCompiler, EmitBundleObjectFiles, WorkloadBuildTasks, PackageInstaller, AppleSdk, and every RunProcess caller such as the Android/Apple builders). Previously the process output was decoded with the default console encoding. On Windows a child process that emits text in the console's OEM code page (rather than UTF-8) will now be mis-decoded, potentially garbling logged diagnostics or error text for those unrelated tools. The emcc path explicitly switches its own code page to 65001 so it matches, but the other callers do not. Please confirm this is intended for all consumers, or scope the UTF-8 encoding to the emcc call site if the goal is only emcc UTF-8 path support.

✅ stdin handling — always redirect and close

RedirectStandardInput is now always true, and process.StandardInput.Close() sends EOF after the optional inputProvider runs. This is a sound way to ensure a child that reads stdin does not block waiting for input; it preserves the existing inputProvider contract (write then EOF) and is harmless for children that never read stdin.

✅ Test coverage

No tests are included, which is consistent with this area (mono/wasm MSBuild task plumbing has no direct unit tests); the change is validated through the WASM build/CI pipelines. Given the cross-cutting encoding change, relying on the emcc/wasm build legs in CI to exercise the affected callers is reasonable, and CI status should be green before merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 80.6 AIC · ⌖ 10.8 AIC · ⊞ 10K