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

[browser] Remove WASM HybridGlobalization from runtime code #110567

Merged
merged 11 commits into from
Dec 17, 2024

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Dec 10, 2024

Connected PRs:
#110526
#110534

This is the last PR aiming at removing HybridGlobalization on WASM. Removals:

  • code from JS, including removal of hybrid globalization module from rollup
  • code from icalls
  • code from libraries, including stubs
  • passing the flag
  • processing the hybrid globalization assets, including segmentation-rules
  • removing hybrid-specific asserts in WBT
  • removing Blazor hybrid WBT

Renames / preserved code:
We are leaving JS-based globalization method GetLocaleInfo to support APIs like NativeName and DisplayName in the regular, ICU globalization mode. The code will be exported by src\mono\browser\runtime\globalization.ts.

  • rename locales-common.ts -> globalization-locale.ts
  • move useful helper methods to globalization-locale.ts

Closes #102373, #95795, #98483, #95921

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-System.Globalization os-browser Browser variant of arch-wasm labels Dec 10, 2024
@ilonatommy ilonatommy added this to the 10.0.0 milestone Dec 10, 2024
@ilonatommy ilonatommy self-assigned this Dec 10, 2024
@ilonatommy
Copy link
Member Author

ilonatommy commented Dec 10, 2024

We finish with

nugets\microsoft.dotnet.sharedframework.sdk\10.0.0-beta.24605.1\targets\sharedfx.targets(305,5): error : The following files are missing entries in the templated manifest: icudt_hybrid.dat. Add these file names with extensions to the 'PlatformManifestFileEntry' item group for the runtime pack and corresponding ref pack to include them in the platform manifest.

Edit:
we have to add a change to icu repo, so that we stop shipping icudt_hybrid.dat for wasm.

@ilonatommy
Copy link
Member Author

ilonatommy commented Dec 11, 2024

WBT tests will fail until dotnet/icu#536 flows in. From this reason, I am reverting WBT changes as well. They will be introduced in another PR. Removals for the follow up:

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Without having all the context, the changes look good, and I imagine tests will pass

Copy link
Member

@pavelsavara pavelsavara left a comment

Choose a reason for hiding this comment

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

I think instantiate_segmentation_rules_asset should be deleted too.

I wonder if this will break any file-based SDK tests


};
if (WasmEnableThreads) {
rh.dumpThreads = mono_wasm_dump_threads;
rh.mono_wasm_print_thread_dump = () => tcwraps.mono_wasm_print_thread_dump();
}
if (loaderHelpers.config.globalizationMode === GlobalizationMode.Hybrid) {
rh.stringToUTF16 = stringToUTF16;
Copy link
Member

Choose a reason for hiding this comment

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

is any of the string functions now un-used ?

Copy link
Member Author

Choose a reason for hiding this comment

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

instantiate_segmentation_rules_asset is deleted from src/mono/browser/runtime/assets.ts, I cannot find any other occurrence that is left, so I guess it's okay.
stringToUTF16 is used but it's taken from the main module. This one that is deleted in the quoted code is a forwarded method to hybrid globalization module. During removal of hybrid code, we also removed the hybrid module, that's why this line is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

"file-based SDK tests" what is a file-based test?

Copy link
Member

Choose a reason for hiding this comment

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

"file-based SDK tests" what is a file-based test?

More like this dotnet/sdk#45412

Copy link
Member

Choose a reason for hiding this comment

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

but for .js file

Copy link
Member

@radekdoulik radekdoulik left a comment

Choose a reason for hiding this comment

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

LGTM

@akoeplinger
Copy link
Member

WBT tests will fail until dotnet/icu#536 flows in.

@ilonatommy you can run this command locally to get the new ICU:

darc update-dependencies --source-repo icu --channel ".NET 10"

@ilonatommy
Copy link
Member Author

@ilonatommy you can run this command locally to get the new ICU:

darc update-dependencies --source-repo icu --channel ".NET 10"

{F4367A02-5808-4D6C-9B55-348A0FBE3980}

@akoeplinger
Copy link
Member

@ilonatommy you need to run it in a checkout of this PR

@ilonatommy
Copy link
Member Author

@ilonatommy you need to run it in a checkout of this PR

I got a result:
Local dependencies updated from channel '.NET 10'
Which means it's only a local change, it does not make a maestro PR to update main?

@ilonatommy ilonatommy merged commit 8a25506 into dotnet:main Dec 17, 2024
156 checks passed
@akoeplinger
Copy link
Member

Which means it's only a local change, it does not make a maestro PR to update main?

Correct, you'd need to commit it like any other change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly architecture area-System.Globalization os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[browser][hybridglobalization][aot] Assert.StartsWith cannot find a match
5 participants