-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[wasm] Initial emscripten 3.1.30 support #81215
Conversation
It now builds, we need updated icu and thus docker images to be able to run anything yet. |
@maraf looks like the undefined icu symbol causes problem with newer emscripten. could you please take a look? |
Emscripten changed a default value of |
It's an undefined symbol in ICU, because we are not linking in a lib, that should in the end be linked out. Updated version of emscripten has by default turned on the LLD_REPORT_UNDEFINED .
Should be `--lto-O0`
@radical could you please take a look at problem with certificates on windows build?
Looks like installing the certifi doesn't help anymore. https://github.com/dotnet/runtime/blob/main/eng/pipelines/mono/update-machine-certs.ps1#L3
|
@@ -325,10 +325,12 @@ | |||
<WriteLinesToFile Lines="@(_EmccCFlags)" File="$(_EmccCompileRsp)" Overwrite="true" WriteOnlyWhenDifferent="true" /> | |||
<ItemGroup> | |||
<FileWrites Include="$(_EmccCompileRsp)" /> | |||
<EmscriptenMinimalBuildEnvVars Include="@(EmscriptenEnvVars)" /> | |||
<EmscriptenMinimalBuildEnvVars Include="FROZEN_CACHE=" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Microsoft.NET.Runtime.Emscripten.Cache SDK overrides the WasmCachePath
property. If we're unfreezing the cache here does that mean we can overwrite the nuget contents sometimes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See dotnet/emsdk#211
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do that on purpose IIRC
runtime/src/mono/wasm/build/WasmApp.Native.targets
Lines 332 to 333 in 9f145a3
<!-- warm up the cache --> | |
<Exec Command="$(_EmBuilder) build MINIMAL" EnvironmentVariables="@(EmscriptenMinimalBuildEnvVars)" StandardOutputImportance="Low" StandardErrorImportance="Low" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the freezing/build was required before we were shipping the cache package we should probably review the logic there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to provide more context: the reason for the change itself is that the default value of FROZEN_CACHE
proably changed with newer emscripten, and so the warming embuilder build MINIMAL
build was now failing, complaining about not being able to update frozen cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole does need to be reviewed again. If the workload is installed in non-user-writable location then how does the build behave? How does the FROZEN_CACHE
setting affect things? I have opened #83281 .
@lambdageek @vargaz I see these assertion crashes in some tests:
Do you know what it might be? Should this be called in non-threaded build too? |
@radekdoulik |
The linker puts stack at 0 sometime, so just check that the stack size is not zero.
The emscripten is now setting stack bounds by the linker generated values. The linker now sometimes puts the stack at the beginning of the linear memory, like in one of the wbt tests, when rebuilding with link -O0 flag.
The dotnet.wasm before rebuild looks like:
The app seems to run fine locally, so I removed the assert checking the beginning of the stack memory area. Let see how it will look on CI. |
@@ -37,8 +38,8 @@ export declare interface EmscriptenModule { | |||
_free(ptr: VoidPtr): void; | |||
|
|||
// this should match emcc -s EXPORTED_RUNTIME_METHODS | |||
print(message: string): void; | |||
printErr(message: string): void; | |||
out(message: string): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will probably break Blazor.
@@ -45,8 +45,8 @@ declare interface EmscriptenModule { | |||
HEAPF64: Float64Array; | |||
_malloc(size: number): VoidPtr; | |||
_free(ptr: VoidPtr): void; | |||
print(message: string): void; | |||
printErr(message: string): void; | |||
out(message: string): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is API breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we consider this API stable?
We can still get the deprecated ones with -sINCOMING_MODULE_JS_API=print,printErr
if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we consider this API stable?
No
We can still get the deprecated ones with
-sINCOMING_MODULE_JS_API=print,printErr
if needed.
Yes please, I think these 2 are widely enough used even by Uno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They could be removed from .d.ts but should keep working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to be in the business of maintaining unstable emscripten apis that emscripten has dropped long term but we should document the change and short term compat is fine.
it's green! 🎊 |
if [[ "$SCENARIO" == "WasmTestOnNodeJS" || "$SCENARIO" == "wasmtestonnodejs" ]]; then | ||
JS_ENGINE_ARGS="--engine-arg=--stack-trace-limit=1000" | ||
else | ||
JS_ENGINE_ARGS="--engine-arg=--stack-trace-limit=1000 --engine-arg=--experimental-wasm-bigint" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at it shouldn't we enable SIMD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not needed here, shouldn't harm though. I can do it in the follow up PR, to avoid more icu/emsdk package switch backs.
@@ -322,10 +323,12 @@ | |||
<WriteLinesToFile Lines="@(_EmccCFlags)" File="$(_EmccCompileRsp)" Overwrite="true" WriteOnlyWhenDifferent="true" /> | |||
<ItemGroup> | |||
<FileWrites Include="$(_EmccCompileRsp)" /> | |||
<EmscriptenMinimalBuildEnvVars Include="@(EmscriptenEnvVars)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be clearer to rename this to a private name like _EmscriptenEnvVarsForBuildMinimal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do it in the follow up PR, to avoid more icu/emsdk package switch backs.
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳 🎉
Failures are known |
Part of #80784 implementation