[wasm] Modify workload to pick threading runtime packs by steveisok · Pull Request #72412 · dotnet/runtime

steveisok · 2022-07-18T22:02:28Z

This change adds the 2 wasm threading runtime packs to the wasm workload. In order for a threading runtime pack to be chosen, WorkloadManifest.targets is also modified to override the runtime pack name when the following props are set:

WasmEnableThreads - full threading support and will load Microsoft.NETCore.App.Runtime.multithread.Mono.browser-wasm

WasmEnablePerfTrace - runtime only threading support and will load Microsoft.NETCore.App.Runtime.perftrace.Mono.browser-wasm

Contributes to #72872

This change adds the 2 wasm threading runtime packs to the wasm workload. In order for a threading runtime pack to be chosen, WorkloadManifest.targets is also modified to override the runtime pack name when the following props are set: WasmEnableThreads - full threading support and will load Microsoft.NETCore.App.Runtime.multithread.Mono.browser-wasm WasmEnablePerfTrace - runtime only threading support and will load Microsoft.NETCore.App.Runtime.perftrace.Mono.browser-wasm

lewing · 2022-07-19T13:53:17Z

Need to make sure WBT knows about the packs

lewing

A couple of questions but should be looks good otherwise

lewing · 2022-07-19T13:54:23Z

src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Manifest/WorkloadManifest.targets.in

+        <RuntimePackNamePatterns Condition="'$(RuntimeIdentifier)' == 'browser-wasm' and '$(WasmEnableThreading)' == 'true'">Microsoft.NETCore.App.Runtime.Mono.multithread.**RID**</RuntimePackNamePatterns>
+        <RuntimePackNamePatterns Condition="'$(RuntimeIdentifier)' == 'browser-wasm' and '$(WasmEnablePerfTrace)' == 'true'">Microsoft.NETCore.App.Runtime.Mono.perftrace.**RID**</RuntimePackNamePatterns>


can both of these be true or do we only allow perf trace on a single threaded userspace? do we need another mt perftrace runtime pack?

Perftrace is single threaded userspace, multi-threaded runtime. Do you think we should error if both are true?

that is probably fine for the moment we may want to have something like named configs once this is supported.

…race are true

radical · 2022-07-19T14:45:51Z

Looks good. I can add a test in WBT.

radical · 2022-07-19T17:57:16Z

Workload installation failed: microsoft.netcore.app.runtime.mono.multithread.browser-wasm::7.0.0-ci is not found in NuGet feeds. I don't see the this nuget being generated in the build.

steveisok · 2022-07-19T19:30:44Z

Workload installation failed: microsoft.netcore.app.runtime.mono.multithread.browser-wasm::7.0.0-ci is not found in NuGet feeds. I don't see the this nuget being generated in the build.

To get this to work, I think WasmBuildTests is going to need to wait for the threading builds to complete and then import its runtime packs.

radical · 2022-07-19T20:28:27Z

To get this to work, I think WasmBuildTests is going to need to wait for the threading builds to complete and then import its runtime packs.

Ok, I will add that.

…uget

- Runtime pack nugets other than the one that gets built might not be available. In that case, we copy the only one available as the missing ones. - This is useful for local builds, allowing running Wasm.Build.Tests without having to build all 3 runtimes. - This is enabled for CI also, in this commit. But once CI gets the ability to fetch the other runtime packs, this will change.

radical · 2022-07-27T00:16:34Z

I have pushed some changes that will unblock this PR. Essentially, we copy the single runtime pack nuget to the other nuget filenames, and should allow running Wasm.Build.Tests . Once I have the PR, to get the other correct runtime packs on CI, completed, I will change this behavior to be applicable only for local builds.

It will emit this when it copies:

        ********************

        Note: Could not find the expected three runtime packs in /workspaces/runtime/artifacts/packages/Release/Shipping/. Found Microsoft.NETCore.App.Runtime.Mono.browser-wasm.7.0.0-dev, Microsoft.NETCore.App.Runtime.Mono.multith
read.browser-wasm.7.0.0-dev, Microsoft.NETCore.App.Runtime.Mono.perftrace.browser-wasm.7.0.0-dev .
              To support local builds, /workspaces/runtime/artifacts/packages/Release/Shipping/Microsoft.NETCore.App.Runtime.Mono.browser-wasm.7.0.0-dev.nupkg was copied as the missing nugets.

        *******************

- The earlier approach of simply making copies of the existing runtime pack nuget with different names doesn't work, and `dotnet workload install` rejects it. ``` Installing pack Microsoft.NETCore.App.Runtime.Mono.multithread.browser-wasm version 7.0.0-ci... Workload installation failed. Rolling back installed packs... ``` Instead, now we build the missing nugets from the project with different values for `$(MonoWasmBuildVariant)`. - this handles local builds, and incremental builds also - To skip building the missing nugets, for example, when you have all of them available, then set `WasmSkipMissingRuntimePackBuild=true`.

radical · 2022-07-27T06:50:46Z

/azp run runtime-wasm

azure-pipelines · 2022-07-27T06:50:59Z

Azure Pipelines successfully started running 1 pipeline(s).

radical · 2022-07-27T15:51:56Z

The coreclr failure seems to be #68289 and unrelated to this PR.
The WBT failures are known (#72880), and unrelated.

steveisok requested review from lewing and radical as code owners July 18, 2022 22:02

ghost added the area-Build-mono label Jul 18, 2022

ghost assigned steveisok Jul 18, 2022

lewing reviewed Jul 19, 2022

View reviewed changes

Add error condition when both WasmEnableThreading and WasmEnablePerfT…

0f9e3b4

…race are true

lewing approved these changes Jul 19, 2022

View reviewed changes

lewing mentioned this pull request Jul 26, 2022

The browser-wasm SDK should automatically reference the threading/perf-tracing ref assemblies #72872

Closed

radical added 3 commits July 26, 2022 23:37

[wasm] Set default value for MonoWasmBuildVariant, for runtime pack n…

d6e57a4

…uget

Merge remote-tracking branch 'origin/main' into resolve-wasm-mt-packs

ec9f701

radical added 2 commits July 27, 2022 04:06

fix comment

32d220c

radical approved these changes Jul 27, 2022

View reviewed changes

runfoapp bot mentioned this pull request Jul 27, 2022

System.Net.* cancellation tests fail in CI #72818

Closed

radical merged commit d7d0e42 into dotnet:main Jul 27, 2022

steveisok deleted the resolve-wasm-mt-packs branch July 27, 2022 16:23

runfoapp bot mentioned this pull request Jul 27, 2022

system.net.http.functional.tests timing out on multiple platforms #72949

Closed

ghost locked as resolved and limited conversation to collaborators Aug 26, 2022

		<RuntimePackNamePatterns Condition="'$(RuntimeIdentifier)' == 'browser-wasm' and '$(WasmEnableThreading)' == 'true'">Microsoft.NETCore.App.Runtime.Mono.multithread.RID</RuntimePackNamePatterns>
		<RuntimePackNamePatterns Condition="'$(RuntimeIdentifier)' == 'browser-wasm' and '$(WasmEnablePerfTrace)' == 'true'">Microsoft.NETCore.App.Runtime.Mono.perftrace.RID</RuntimePackNamePatterns>

Conversation

steveisok commented Jul 18, 2022 • edited by lambdageek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lewing commented Jul 19, 2022

Uh oh!

lewing left a comment

Choose a reason for hiding this comment

Uh oh!

lewing Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

steveisok Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

lewing Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

radical commented Jul 19, 2022

Uh oh!

radical commented Jul 19, 2022

Uh oh!

steveisok commented Jul 19, 2022

Uh oh!

radical commented Jul 19, 2022

Uh oh!

radical commented Jul 27, 2022

Uh oh!

radical commented Jul 27, 2022

Uh oh!

azure-pipelines bot commented Jul 27, 2022

Uh oh!

radical commented Jul 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steveisok commented Jul 18, 2022 •

edited by lambdageek

Loading