Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/7.0] Fix encoding problem when publishing with AOT #89388

Closed

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Jul 24, 2023

Backport of #83510 to release/7.0.

Customer Impact

Customers had a problem with publishing applications with AOT that were located in a project named with Unicode chars. It was because we were using internally the assembly name directly to paste into C code. We were only replacing a few chosen characters with underscored. Now we are replacing all non-ASCII chars with underscores, to avoid throwing an error in the C code.

Testing

Creating a project that has non-ASCII chars in the name and publish with AOT on.

Risk

Low, this only makes a previously failing use case work now.

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-Build-mono wasm-aot-test WebAssembly AOT Test labels Jul 24, 2023
@ilonatommy ilonatommy requested a review from lewing July 24, 2023 14:23
@ilonatommy ilonatommy requested a review from radical as a code owner July 24, 2023 14:23
@ilonatommy ilonatommy self-assigned this Jul 24, 2023
@ghost
Copy link

ghost commented Jul 24, 2023

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

Issue Details

Backport of #83035 to release/7.0.

Customer Impact
Customers had a problem with publishing applications that were located in a path with Unicode chars. It was because Windows’s scripts do not support UTF-8 by default. Now we added that feature. It does not work for relinking yet, because the issue is still blocked: #83497, so relinking tests got disabled.

Testing
Creating a project that has non-ASCII chars in the path and publish with AOT on.

Risk
Low, it broadens the accepted range of codes for Windows to UTF-8.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-Build-mono, wasm-aot-test

Milestone: -

@ilonatommy ilonatommy added the Servicing-consider Issue for next servicing release review label Jul 24, 2023
@ilonatommy
Copy link
Member Author

The failure is not connected.

[BuildAndRun(host: RunHost.Chrome, aot: true, config: "Debug")]
public void BuildThenPublishWithAOT(BuildArgs buildArgs, RunHost host, string id)
[BuildAndRun(host: RunHost.Chrome, aot: true, config: "Release", testUnicode: false)]
[BuildAndRun(host: RunHost.Chrome, aot: true, config: "Debug", testUnicode: false)]
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you are missing the corresponding changes to BuildAndRunAttribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

With @radical we decided to introduce some changes to tests in net8 first, maybe it will help resolve #83497 and only then backport it.

@lewing lewing added this to the 7.0.x milestone Jul 25, 2023
@ilonatommy ilonatommy marked this pull request as draft July 26, 2023 06:48
@lewing lewing removed the Servicing-consider Issue for next servicing release review label Jul 27, 2023
@carlossanlop
Copy link
Member

Friendly reminder: if you want this servicing fix to be included in the September 2023 Release, you'll have to merge this PR before August 14th.

@ghost ghost closed this Sep 1, 2023
@ghost
Copy link

ghost commented Sep 1, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono wasm-aot-test WebAssembly AOT Test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants