[release/10.0] [wasm] Fix native build failing when temp path contains parentheses#131039
[release/10.0] [wasm] Fix native build failing when temp path contains parentheses#131039github-actions[bot] wants to merge 3 commits into
Conversation
RunShellCommand writes the compile command to a temporary batch file under
Path.GetTempPath() and runs it with 'cmd /c "<script>"'. When the temp path
contains characters cmd.exe treats specially - most commonly parentheses from a
Windows user profile name like C:\Users\John(US) - cmd's /c quote handling
strips the surrounding quotes (it sees special characters between the two
quotes) and then mis-parses the now-unquoted path at the first '(', producing:
'C:\Users\John' is not recognized as an internal or external command
and the whole native (emcc) build fails.
Invoke the script as 'cmd /S /c ""<script>""' instead: /S makes cmd strip
only the outermost pair of quotes and treat the rest verbatim, so the inner
quotes around the path are preserved.
Adds a Windows-only Wasm.Build.Tests regression that runs a native build with
%TMP%/%TEMP% pointed at a directory containing parentheses.
Fixes #120327
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 62dcd5b3-7227-426b-a072-8812e9c4382d
…path Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 62dcd5b3-7227-426b-a072-8812e9c4382d
…eses Restore the non-ASCII characters (dropped in the previous commit) so the temp path exercises both the cmd.exe quoting fix (parentheses) and the UTF-8 (chcp 65001) handling in RunShellCommand, while GetRandomId keeps it unique. Rename the test accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 62dcd5b3-7227-426b-a072-8812e9c4382d
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "9232c2cafdf865cfb1bed5f9cb78edd84e8bb191",
"last_dispatched_base_ref": "release/10.0",
"last_dispatched_base_sha": "018b2627823158a827a8222f0327f99f6a970565",
"last_reviewed_commit": "9232c2cafdf865cfb1bed5f9cb78edd84e8bb191",
"last_reviewed_base_ref": "release/10.0",
"last_reviewed_base_sha": "018b2627823158a827a8222f0327f99f6a970565",
"last_recorded_worker_run_id": "29693365936",
"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": "9232c2cafdf865cfb1bed5f9cb78edd84e8bb191",
"review_id": 4731042955
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: This is a clean backport of #131025 to release/10.0, fixing #120327. On Windows, Utils.RunShellCommand writes the native (emcc) compile command to a temporary batch file under Path.GetTempPath() and runs it with cmd /c "<script>". When the temp path contains characters cmd.exe treats specially—most commonly parentheses from a Windows user profile name such as C:\Users\John(US)—cmd's /c quote handling strips the surrounding quotes and then mis-parses the now-unquoted path at the first (, producing 'C:\Users\John' is not recognized as an internal or external command and failing the whole native build.
Approach: The invocation is changed from cmd /c "<script>" to cmd /S /c ""<script>"". Per cmd's documented /S behavior, when the argument begins and ends with a quote, /S strips only the outermost pair of quotes and runs the remainder verbatim, preserving the inner quotes around the still-quoted script path. This is the canonical, well-understood remedy for this class of cmd quoting bug and is the minimal correct change. Only the Windows branch is affected; the /bin/sh path is untouched. A Windows-only Wasm.Build.Tests regression test runs a native build with %TMP%/%TEMP% pointed at a directory whose name contains both parentheses and the existing s_unicodeChars (GB18030-style) characters, so it exercises both the new quoting fix and the pre-existing UTF-8 (chcp 65001) handling in the same code path.
Summary: LGTM. The fix is correct, minimal, and matches the standard cmd /S /c pattern for quoting paths containing special characters. Scope is appropriately limited to the Windows branch, and the added regression test is well-targeted, [SkipOnPlatform]-guarded to Windows, and passes the parenthesized/Unicode temp path through ExtraBuildEnvironmentVariables so the build subprocess resolves Path.GetTempPath() accordingly. Risk is low and appropriate for servicing. No actionable findings.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 47.6 AIC · ⌖ 16 AIC · ⊞ 10K
Backport of #131025 to release/10.0
/cc @lewing
Customer Impact
[Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.]
Regression
[If yes, specify when the regression was introduced. Provide the PR or commit if known.]
Testing
[How was the fix verified? How was the issue missed previously? What tests were added?]
Risk
[High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]
IMPORTANT: If this backport is for a servicing release, please verify that:
release/X.0-staging, notrelease/X.0.release/X.0(no-stagingsuffix).Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.