[wasm] Fix up conditions to trigger relink, and require `wasm-tools` workload #89754

radical · 2023-08-01T02:12:08Z

Trigger relinking (WasmBuildNative=true) if:
- WasmNativeStrip=false
- WasmEnableSIMD=false
- WasmEnableExceptionHandling=false
- The above are in addition to the existing conditions
Also, trigger "workload required" when:
- WasmNativeStrip=false
- WasmEnableExceptionHandling=true
- InvariantGlobalization=true
- InvariantTimeZone=true
- The above are in addition to the existing conditions
Rationalize WasmNativeDebugSymbols, and WasmNativeStrip
- WasmNativeDebugSymbols will cause symbols to be included
  (essentially -g)
- WasmNativeStrip will cause these to be stripped with wasm-opt --strip-dwarf ...

ghost · 2023-08-01T02:12:17Z

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

Issue Details

null

Author:	radical
Assignees:	-
Labels:	`arch-wasm`
Milestone:	-

- `WasmNativeStrip=false` - this will trigger a relink. Runtime pack is built with `WasmNativeStrip=true`. - Also, honor `WasmEnableSIMD=false`, and `WasmEnableExceptionHandling=false` for publish to trigger a relink.

…voids unnecessary relinking for the no-aot case

.. and don't set `WasmNativeStrip=false` by default, as it triggers relinking now.

…-required

…eads

…ativeStrip - `WasmNativeDebugSymbols` will cause symbols to be included (essentially `-g`) - `WasmNativeStrip` will cause these to be stripped with `wasm-opt --strip-dwarf ...`

radical · 2023-08-08T06:04:18Z

/azp run runtime-wasm

azure-pipelines · 2023-08-08T06:04:27Z

Azure Pipelines successfully started running 1 pipeline(s).

eng/testing/tests.browser.targets

kg · 2023-08-10T21:46:58Z

...ono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WorkloadManifest.targets.in

-    <_WasmNativeWorkloadNeeded Condition="'$(RunAOTCompilation)' == 'true' or '$(WasmEnableSIMD)' == 'true' or '$(WasmEnableLegacyJsInterop)' == 'false' or '$(WasmBuildNative)' == 'true' or
-      '$(WasmGenerateAppBundle)' == 'true' or '$(_UsingBlazorOrWasmSdk)' != 'true'" >true</_WasmNativeWorkloadNeeded>
+    <!-- Keep in sync with WasmApp.Native.targets -->
+    <_WasmNativeWorkloadNeeded Condition="


Does this get evaluated late enough that defaults have already been applied for all these properties? If not, shouldn't most of these be of the form != 'false' instead of == 'true'?

Good question! Setting _WasmNativeWorkloadNeeded will cause the build to fail if the user does not have the wasm-tools workload installed.

The workload is needed for doing any native build, which is when:

If the various settings don't match, what the bits in the runtime pack were built with (for example WasmEnableLegacyJsInterop=false)

Using native references, or AOT

Explicitly asked for, by setting WasmBuildNative=true

But without the workload the targets for these native builds are not available at all, so essentially, the user will be expecting a native build but nothing would happen.

To prompt the user to install the the workload we set _WasmNativeWorkloadNeeded=true here, and essentially try to determine whether a native build might be needed.

We don't want it to get triggered by default

So, most of these properties are for settings different from the runtime pack build.

But, having said that, I'm now curious about WasmEnableSIMD, and WasmEnableExceptionHandling, which are true for the runtime pack, and thus AFAIU, the conditions should be inverted. I will check this more, and get back to you.

And to answer the original question, we want to explicitly checking for a value that is not the default. And that's why this list needs to be in sync with what we have WasmApp.Native.targets, what wasm.proj` uses for building the runtime pack bits.

So, the WasmEnableSIMD, and WasmEnableExceptionHandling were incorrect. Apparently, we hadn't changed them since they were made the default.

kg · 2023-08-10T22:00:39Z

src/mono/wasm/build/WasmApp.Native.targets

+    </ItemGroup>
+
+    <PropertyGroup>
+      <WasmBuildNative Condition="'$(WasmBuildNative)' == '' and


This is really clever 👍

kg

The parts I understand all look good

@kg

This was not changed after `WasmEnableSIMD=true` became the default for the runtime pack. And update the tests to match. Effectively: - `WasmEnableSIMD=true` will not trigger requiring a workload - `WasmEnableSIMD=false` *will* trigger requiring a workload Thanks to @kg for the prompt to check all this.

radical · 2023-08-11T00:37:34Z

/azp run runtime-wasm

azure-pipelines · 2023-08-11T00:37:49Z

Azure Pipelines successfully started running 1 pipeline(s).

radical · 2023-08-11T03:38:27Z

The failure is unrelated, and known.

The following commit caused workload to be required for a blazor app when `InvariantGlobalization==true`, but this is not required. ``` commit 26ae097 Author: Ankit Jain <radical@gmail.com> Date: Thu Aug 10 23:39:10 2023 -0400 [wasm] Fix up conditions to trigger relink, and require `wasm-tools` workload (dotnet#89754) ``` And this broke some sdk tests.

The following commit caused workload to be required for a blazor app when `InvariantGlobalization==true`, but this is not required. ``` commit 26ae097 Author: Ankit Jain <radical@gmail.com> Date: Thu Aug 10 23:39:10 2023 -0400 [wasm] Fix up conditions to trigger relink, and require `wasm-tools` workload (#89754) ``` And this broke some sdk tests.

radical added the arch-wasm WebAssembly architecture label Aug 1, 2023

ghost assigned radical Aug 1, 2023

dotnet-issue-labeler bot added the area-Build-mono label Aug 1, 2023

radical changed the title ~~wip~~ [wasm] Trigger native build when some properties have a different value than the runtime pack Aug 1, 2023

vargaz approved these changes Aug 1, 2023

View reviewed changes

vargaz self-requested a review August 1, 2023 21:34

build-analysis bot mentioned this pull request Aug 7, 2023

Insufficient memory of docker containers on CI dotnet/dnceng#450

Open

3 tasks

radical mentioned this pull request Aug 7, 2023

[wasm] Blazor, or console template with AOT/relinking fails with: mono_wasm_load_runtime () failed Error: unreachable #90111

Closed

radical added 8 commits August 8, 2023 05:46

Remove old hack

ea1a43e

[wasm] Fix up conditions to relink

14de9e2

- `WasmNativeStrip=false` - this will trigger a relink. Runtime pack is built with `WasmNativeStrip=true`. - Also, honor `WasmEnableSIMD=false`, and `WasmEnableExceptionHandling=false` for publish to trigger a relink.

tests.browser.targets: Use WasmNativeStrip=false only for AOT. This a…

d99d7ba

…voids unnecessary relinking for the no-aot case

WBT: Update tests for properties that trigger relinking

1393670

.. and don't set `WasmNativeStrip=false` by default, as it triggers relinking now.

WorkloadManifest.targets: Add new property checks to trigger workload…

84a6b2f

…-required

[wasm] WorkloadManifest.targets - net8 only knows about WasmEnableThr…

fa69887

…eads

WasmApp.Native.targets: Rationalize WasmNativeDebugSymbols, and WasmN…

8087a32

…ativeStrip - `WasmNativeDebugSymbols` will cause symbols to be included (essentially `-g`) - `WasmNativeStrip` will cause these to be stripped with `wasm-opt --strip-dwarf ...`

cleanup

4a5d3d3

radical force-pushed the wasm-strip-native branch from 33212f1 to 4a5d3d3 Compare August 8, 2023 05:57

radical changed the title ~~[wasm] Trigger native build when some properties have a different value than the runtime pack~~ [wasm] Fix up conditions to trigger relink, and require wasm-tools workload Aug 8, 2023

radical requested review from pavelsavara, brickgeorge, kg and radekdoulik and removed request for brickgeorge August 8, 2023 06:01

pavelsavara reviewed Aug 8, 2023

View reviewed changes

eng/testing/tests.browser.targets Show resolved Hide resolved

radical added 2 commits August 8, 2023 15:49

Merge remote-tracking branch 'origin/main' into wasm-strip-native

34e84d0

add missing file

1921e75

radical marked this pull request as ready for review August 8, 2023 17:29

radical requested a review from lewing as a code owner August 8, 2023 17:29

radical added 2 commits August 8, 2023 19:08

cleanup

25b0cb6

Merge branch 'main' into wasm-strip-native

61c8895

pavelsavara mentioned this pull request Aug 9, 2023

Performance profiling for WebAssembly #76316

Open

13 tasks

Merge branch 'main' into wasm-strip-native

b23d61e

kg reviewed Aug 10, 2023

View reviewed changes

kg approved these changes Aug 10, 2023

View reviewed changes

radical added 2 commits August 10, 2023 22:22

Merge remote-tracking branch 'origin/main' into wasm-strip-native

b3a0f44

This was referenced Aug 11, 2023

[browser] mono runtime already exited with 0 #89425

Closed

[browser] Fragile WebSocket CI tests #90257

Closed

radical merged commit 26ae097 into dotnet:main Aug 11, 2023
41 of 43 checks passed

radical deleted the wasm-strip-native branch August 11, 2023 03:39

ilonatommy mentioned this pull request Aug 11, 2023

[main] Update dependencies from dotnet/runtime dotnet/sdk#34522

Merged

radekdoulik mentioned this pull request Aug 23, 2023

[Perf] Linux/x64: 225 Regressions on 8/18/2023 7:47:26 AM dotnet/perf-autofiling-issues#20642

Open

ghost locked as resolved and limited conversation to collaborators Sep 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm] Fix up conditions to trigger relink, and require `wasm-tools` workload #89754

[wasm] Fix up conditions to trigger relink, and require `wasm-tools` workload #89754

radical commented Aug 1, 2023 •

edited

Loading

ghost commented Aug 1, 2023

radical commented Aug 8, 2023

azure-pipelines bot commented Aug 8, 2023

kg Aug 10, 2023

radical Aug 10, 2023

radical Aug 10, 2023

radical Aug 10, 2023

kg Aug 10, 2023

kg left a comment

radical commented Aug 11, 2023

azure-pipelines bot commented Aug 11, 2023

radical commented Aug 11, 2023

[wasm] Fix up conditions to trigger relink, and require wasm-tools workload #89754

[wasm] Fix up conditions to trigger relink, and require wasm-tools workload #89754

Conversation

radical commented Aug 1, 2023 • edited Loading

ghost commented Aug 1, 2023

radical commented Aug 8, 2023

azure-pipelines bot commented Aug 8, 2023

kg Aug 10, 2023

Choose a reason for hiding this comment

radical Aug 10, 2023

Choose a reason for hiding this comment

radical Aug 10, 2023

Choose a reason for hiding this comment

radical Aug 10, 2023

Choose a reason for hiding this comment

kg Aug 10, 2023

Choose a reason for hiding this comment

kg left a comment

Choose a reason for hiding this comment

radical commented Aug 11, 2023

azure-pipelines bot commented Aug 11, 2023

radical commented Aug 11, 2023

[wasm] Fix up conditions to trigger relink, and require `wasm-tools` workload #89754

[wasm] Fix up conditions to trigger relink, and require `wasm-tools` workload #89754

radical commented Aug 1, 2023 •

edited

Loading