Skip to content

[browser] Fix encoding problem when publishing - #82833

Merged
ilonatommy merged 11 commits into
dotnet:mainfrom
ilonatommy:fix-78953
Mar 16, 2023
Merged

[browser] Fix encoding problem when publishing#82833
ilonatommy merged 11 commits into
dotnet:mainfrom
ilonatommy:fix-78953

Conversation

@ilonatommy

@ilonatommyilonatommy commented Mar 1, 2023

Copy link
Copy Markdown
Member

Fixes#78953.

For usernames with Unicode characters we had a problem because Window's .cmd scripts are not supporting Unicode (https://ss64.com/nt/chcp.html). Temporary files paths, e.g. "C:\Users\煉\AppData\Local\Temp\tmpC538.tmp" were passed to emcc with incorrect encoding and then could not be found in the file system which caused exceptions.

Out-of-scope:

@ilonatommyilonatommy added arch-wasm WebAssembly architecture area-Build-mono labels Mar 1, 2023
@ilonatommyilonatommy self-assigned this Mar 1, 2023
@ghost

ghost commented Mar 1, 2023

Copy link
Copy Markdown

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

Issue Details

Fixes #78953.

For usernames with Unicode characters we had a problem because Window's .cmd scripts are not supporting Unicode (https://ss64.com/nt/chcp.html). Temporary files paths, e.g. "C:\Users\煉\AppData\Local\Temp\tmpC538.tmp" were passed to emcc with incorrect encoding and then could not be found in the file system which caused exceptions.

Author:ilonatommy
Assignees:ilonatommy
Labels:

arch-wasm, area-Build-mono

Milestone:-

@radical

Copy link
Copy Markdown
Member

This needs a test in WBT. Instead of the username, the unicode characters can be in the project name.

Comment threadsrc/mono/wasm/Wasm.Build.Tests/BuildPublishTests.cs Outdated
Comment threadsrc/tasks/Common/Utils.cs Outdated
Comment threadsrc/mono/wasm/Wasm.Build.Tests/BuildPublishTests.cs Outdated
@radical

Copy link
Copy Markdown
Member

Failing on windows, and linux:
wasm-ld : error : /root/helix/work/workitem/e/wbt/pztditip.fs4/obj/Debug/net8.0/browser-wasm/wasm/for-publish/driver.o: undefined symbol: mono_aot_module_build_publish_Debug煉_info

stringsymbolName=assemblyName.Replace('.','_').Replace('-','_').Replace(' ','_');
symbols.Add($"mono_aot_module_{symbolName}_info");

  • this probably needs to use FixupSymbolName too
  • And do we need to get aot compiler to use the fixed up name too?

@maraf could you please take a look?

@pavelsavara

Copy link
Copy Markdown
Member

This does similar operation on assembly names for WASI

privatestaticstringToSafeSymbolName(stringdestinationFileName)

@radical

Copy link
Copy Markdown
Member

We can replace the wasi one with FixupSymbolName which is more tested too.

@ilonatommy

Copy link
Copy Markdown
MemberAuthor

FixupSymbolName

Thank you for a hint. Using the FixumSymbolName does not seem to fix it yet. It changes the error into, e.g.:
wasm-ld : error : /workspaces/runtime/artifacts/bin/Wasm.Build.Tests/Debug/net8.0/browser-wasm/wbt/dhvzeehz.ggm/obj/Debug/net8.0/browser-wasm/wasm/for-publish/driver.o: undefined symbol: mono_aot_module_build_publish_Debug_E7__85__89__info
but the issue is still there. I will change this PR to non-AOT fix, then we can go on with AOT part in #83035.

Comment threadsrc/mono/wasm/Wasm.Build.Tests/Blazor/BuildPublishTests.cs Outdated
@ilonatommy

ilonatommy commented Mar 7, 2023

Copy link
Copy Markdown
MemberAuthor

Stashing for later investigation:
running runtime (Build browser-wasm windows Release WasmBuildTests) 7/11 Wasm.Build.Tests.Blazor.BuildPublishTests fail with

Failed opening 'runtime-fork2\artifacts\bin\Wasm.Build.Tests\Debug\net8.0\browser-wasm\wbt\blz_dllimp_Release_?build_then_publish\obj\Release\net8.0\wasm\for-build\dotnet.wasm'
emcc : error : '\runtime-fork2\artifacts\bin\dotnet-latest\packs\Microsoft.NET.Runtime.Emscripten.3.1.12.Sdk.win-x64\8.0.0-preview.3.23127.2\tools\bin\wasm-emscripten-finalize -g --dyncalls-i64 --dwarf \runtime-fork2\artifacts\bin\Wasm.Build.Tests\Debug\net8.0\browser-wasm\wbt\blz_dllimp_Release_?build_then_publish\obj\Release\net8.0\wasm\for-build\dotnet.wasm -o \runtime-fork2\artifacts\bin\Wasm.Build.Tests\Debug\net8.0\browser-wasm\wbt\blz_dllimp_Release_?build_then_publish\obj\Release\net8.0\wasm\for-build\dotnet.wasm --detect-features' failed (returned 1) [\runtime-fork2\artifacts\bin\Wasm.Build.Tests\Debug\net8.0\browser-wasm\wbt\blz_dllimp_Release_?build_then_publish\blz_dllimp_Release_?build_then_publish.csproj]
\runtime-fork2\artifacts\bin\dotnet-latest\packs\Microsoft.NET.Runtime.WebAssembly.Sdk\8.0.0-dev\Sdk\WasmApp.Native.targets(468,5): error MSB3073: The command "emcc "@\runtime-fork2\artifacts\bin\dotnet-latest\packs\Microsoft.NETCore.App.Runtime.Mono.browser-wasm\8.0.0-dev\runtimes\browser-wasm\native\src\emcc-default.rsp" "@\runtime-fork2\artifacts\bin\dotnet-latest\packs\Microsoft.NETCore.App.Runtime.Mono.browser-wasm\8.0.0-dev\runtimes\browser-wasm\native\src\emcc-link.rsp" "@\repos\runtime-fork2\artifacts\bin\Wasm.Build.Tests\Debug\net8.0\browser-wasm\wbt\blz_dllimp_Release_?build_then_publish\obj\Release\net8.0\wasm\for-build\emcc-link.rsp"" exited with code 1. [runtime-fork2\artifacts\bin\Wasm.Build.Tests\Debug\net8.0\browser-wasm\wbt\blz_dllimp_Release_?build_then_publish\blz_dllimp_Release_?build_then_publish.csproj]

@radical

Copy link
Copy Markdown
Member
  • 7/11 Wasm.Build.Tests.Blazor.BuildPublishTests fail with

These seem to be failing on CI too.

@ilonatommy

ilonatommy commented Mar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Publish with relink tests on Blazor do not work.
We are passing the unicode sign correctly but emcc cannot process it. The command it's failng on:

 emcc "artifacts\bin\dotnet-latest\packs\Microsoft.NETCore.App.Runtime.Mono.browser-wasm\8.0.0-dev\runtimes\browser-wasm\native\src\emcc-default.rsp" "artifacts\bin\dotnet-latest\packs\Microsoft.NETCore.App.Runtime.Mono.browser-wasm\8.0.0-dev\runtimes\browser-wasm\native\src\emcc-link.rsp" "artifacts\bin\Wasm.Build.Tests\Debug\net8.0\browser-wasm\wbt\blz_no_aot_Release_煉\obj\Release\net8.0\wasm\for-publish\emcc-link.rsp"

emscripten-core/emscripten#17817. I will block these tests till emcc solves it.

@ilonatommy
ilonatommy requested a review from radicalMarch 16, 2023 07:04
Comment threadsrc/mono/wasm/Wasm.Build.Tests/Blazor/BuildPublishTests.cs

@radicalradical left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the comment, LGTM 👍

@ilonatommy
ilonatommy merged commit 7f0c19a into dotnet:mainMar 16, 2023
@ilonatommy

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4435596854

@github-actions

Copy link
Copy Markdown
Contributor

@ilonatommy backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixed https://github.com/dotnet/runtime/issues/78953.
Applying: New wbt publish test case: Unicode sign in project name.
Using index info to reconstruct a base tree...
A	src/mono/wasm/Wasm.Build.Tests/BuildPublishTests.cs
Falling back to patching base and 3-way merge...
Applying: Removed duplicated, unused function+ addressed @akoeplinger review.
Applying: Fix.
Applying: Revert AOT tests to fix them in a follow-up.
error: sha1 information is lacking or useless (src/mono/wasm/Wasm.Build.Tests/BuildPublishTests.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Revert AOT tests to fix them in a follow-up.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions

Copy link
Copy Markdown
Contributor

@ilonatommy an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

@ghostghost locked as resolved and limited conversation to collaborators Apr 15, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-Build-mono

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Publish Error In Using Japanese Char Account

4 participants

@ilonatommy@radical@pavelsavara@akoeplinger