[release/7.0][wasm-mt] Fix pack/build issues in threaded builds by lambdageek · Pull Request #75171 · dotnet/runtime

lambdageek · 2022-09-06T23:42:04Z

Manual backport of #75162

Addresses the following issues in #74654:

dotnet.worker.js missing from the runtime pack
WorkloadManifest.targets.in uses WasmEnableThreading instead of WasmEnableThreads
The CoreLibs in the multithread and perftrace build variants don't define the correct feature flags.
dotnet new wasmbrowser creates browser.csproj, not <dirname>.csproj

ghost · 2022-09-06T23:42:12Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost · 2022-09-06T23:42:42Z

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

Issue Details

Manual backport of #75162

Addresses the following issues in #74654:

dotnet.worker.js missing from the runtime pack
WorkloadManifest.targets.in uses WasmEnableThreading instead of WasmEnableThreads
The CoreLibs in the multithread and perftrace build variants don't define the correct feature flags.
dotnet new wasmbrowser creates browser.csproj, not <dirname>.csproj

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

…ithread' Also define the perf-tracing feature flag for the threaded and perf-tracing build variants.

not WasmEnabelThreading. Conversely, use `WasmEnablePerfTracing`, not `WasmEnablePerfTrace`.

lambdageek · 2022-09-07T00:01:36Z

Ah, the backport had to be done manually because the workload manifest is now src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.net7.Manifest/WorkloadManifest.targets.in

The workload resolver already did its job by the time this property is updated, so we need to use patterns that correspond to actual nupkg names (unversioned), not runtime pack alias names from the manifest (versioned with 'net7')

For local testing if the built runtime is a multithreaded one, make and we need to make the normal variant nuget, unset MonoWasmBuildVariant property

lambdageek · 2022-09-07T13:45:00Z

WasmBuildTests is now failing because this hack is no longer necessary and it's messing up the IncludedWorkloadManifests.txt file. The newest SDK that dotnet-install.sh pulls down ( 7.0.100-rc.2.22457.6) has the .net6 and .net7 toolchains already

runtime/src/tasks/WorkloadBuildTasks/InstallWorkloadFromArtifacts.cs

Lines 143 to 173 in 9dda765

// HACK BEGIN - because sdk doesn't yet have the net6/net7 manifest names in the known workloads
// list
string?txtPath=Directory.EnumerateFiles(Path.Combine(SdkWithNoWorkloadInstalledPath,"sdk"),"IncludedWorkloadManifests.txt",
newEnumerationOptions{RecurseSubdirectories=true,MaxRecursionDepth=2})
.FirstOrDefault();
if(txtPathisnull)
thrownewLogAsErrorException($"Could not find IncludedWorkloadManifests.txt in {SdkWithNoWorkloadInstalledPath}");
stringstampPath=Path.Combine(Path.GetDirectoryName(txtPath)!,".stamp");
if(!File.Exists(stampPath))
{
Log.LogMessage(MessageImportance.High,$"txtPath: {txtPath}");
stringnewTxt=File.ReadAllText(txtPath)
.Replace("microsoft.net.workload.mono.toolchain",
$"microsoft.net.workload.mono.toolchain.net6{Environment.NewLine}microsoft.net.workload.mono.toolchain.net7")
.Replace("microsoft.net.workload.emscripten",
$"microsoft.net.workload.emscripten.net6{Environment.NewLine}microsoft.net.workload.emscripten.net7");
File.WriteAllText(txtPath,newTxt);
File.WriteAllText(stampPath,"");
}
stringp=Path.Combine(SdkWithNoWorkloadInstalledPath,"sdk-manifests","7.0.100","microsoft.net.workload.mono.toolchain");
Log.LogMessage(MessageImportance.High,$"Deleting {p}");
if(Directory.Exists(p))
Directory.Delete(p,recursive:true);
p=Path.Combine(SdkWithNoWorkloadInstalledPath,"sdk-manifests","7.0.100","microsoft.net.workload.emscripten");
Log.LogMessage(MessageImportance.High,$"Deleting {p}");
if(Directory.Exists(p))
Directory.Delete(p,recursive:true);
// HACK END

Starting with 7.0.100-rc.2.22457.6 we already have the versioned net6/net7 toolchains, so the hack isn't needed anymore (and in fact, breaks workload installation during testing)

lambdageek · 2022-09-07T14:28:34Z

WasmBuildTests is now failing because this hack is no longer necessary

Pushed a fix to this PR to remove the hack. Separately opened #75198 to remove the hack on release/7.0-rc1

lambdageek · 2022-09-07T14:43:16Z

This should be ready to go now.

Could I get a review and approval @radical @lewing @marek-safar

radical · 2022-09-07T17:31:05Z

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


Just wondering - should we have WasmEnableThreading to match WasmEnablePerfTrac*ing*?

Maybe. 😁
I'm not really happy with either of these names. "perf tracing" is not really a great name either.
I think the customer-facing names should be reworked over in net8.

@lewing @steveisok thoughts?

In net8 we could try an item with list of features to enable, like the runtime components stuff.

I don't have any good suggestions :-).

radical · 2022-09-07T17:35:05Z

WBT has failures on windows due to networking issues, so trying once more.

carlossanlop · 2022-09-07T18:49:21Z

@marek-safar we need an approval please.
@lambdageek please ping me for merging after the discussions are resolved and the CI failures are determined to be unrelated.

radical · 2022-09-07T23:02:32Z

@carlossanlop this is ready to go

carlossanlop · 2022-09-07T23:18:00Z

@radical @lambdageek @steveisok is this tell-mode or ask-mode? I see some product code changes, even though most seem to be infra. The area label isn't making it clear.

lambdageek · 2022-09-07T23:57:56Z

Update pushed

I need to push one more commit here. System.Runtime.InteropServices.JavaScript also needs to be built with a MonoWasmBuildVariant check in the .csproj. Otherwise we don't get the browser synchronization context installed.

lambdageek · 2022-09-08T01:30:24Z

@carlossanlop tell mode. this is to support wasm multithreading, which is a preview feature in net7

lambdageek · 2022-09-08T13:13:48Z

@carlossanlop

Foward port changes from `release/7.0` dotnet#75171 that were not included in `main` dotnet#75162 - when building the InteropServices.JavaScript library, enable threading if MonoWasmBuildVariant is set appropriately. One consequence is that the runtime will (correctly) install the browser synchronization context on the main thread. - for workload build testing, unset MonoWasmBuildVariant when creating the non-threaded runtime

Foward port changes from `release/7.0` #75171 that were not included in `main` #75162 - when building the InteropServices.JavaScript library, enable threading if MonoWasmBuildVariant is set appropriately. One consequence is that the runtime will (correctly) install the browser synchronization context on the main thread. - for workload build testing, unset MonoWasmBuildVariant when creating the non-threaded runtime

lambdageek requested review from lewing, marek-safar, pavelsavara and radical as code owners September 6, 2022 23:42

ghost assigned lambdageek Sep 6, 2022

lambdageek added the arch-wasm WebAssembly architecture label Sep 6, 2022

lambdageek added Servicing-consider Issue for next servicing release review area-Build-mono and removed Servicing-consider Issue for next servicing release review labels Sep 6, 2022

lambdageek added 6 commits September 6, 2022 19:56

[wasm-mt] Build the threaded CoreLib if MonoWasmBuildVariant is 'mult…
7c7ecb8
…ithread' Also define the perf-tracing feature flag for the threaded and perf-tracing build variants.

Also fixup the BCL assemblies
94c4e8b

[wasm-mt] look for WasmEnableThreads in user projects
e5c2aae
not WasmEnabelThreading. Conversely, use `WasmEnablePerfTracing`, not `WasmEnablePerfTrace`.

include dotnet.worker.js in runtime pack
0fe387a

use set "preferDirectoryName" to true in WASM templates
364323d

fix whitespace
85789bd

lambdageek force-pushed the hack-wasm-mt-net7 branch from 0dfaba7 to 85789bdCompare September 6, 2022 23:59

lambdageek commented Sep 7, 2022
View reviewed changes

Comment threadsrc/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.net7.Manifest/WorkloadManifest.targets.in Outdated

lambdageek added 2 commits September 7, 2022 08:08

WBT: unset MonoWasmBuildVariant for the non-threaded runtime
a75226b
For local testing if the built runtime is a multithreaded one, make and we need to make the normal variant nuget, unset MonoWasmBuildVariant property

lambdageek mentioned this pull request Sep 7, 2022
[wasm-mt] Issues with enabling threads in .NET 7 RC 1 #74654
Closed
3 tasks

[WorkloadBuildTasks] remove the workload manifest net6/net7 hack
a0ee879
Starting with 7.0.100-rc.2.22457.6 we already have the versioned net6/net7 toolchains, so the hack isn't needed anymore (and in fact, breaks workload installation during testing)

radical reviewed Sep 7, 2022
View reviewed changes

radical approved these changes Sep 7, 2022
View reviewed changes

also build the JS interop library with threading if variant is used
27370f0

marek-safar approved these changes Sep 8, 2022
View reviewed changes

lewing merged commit f85d370 into dotnet:release/7.0Sep 8, 2022

lambdageek mentioned this pull request Sep 8, 2022
[wasm-mt] Additional build fixes #75297
Merged

ghost locked as resolved and limited conversation to collaborators Oct 8, 2022

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

Uh oh!

Conversation

lambdageek commented Sep 6, 2022

Uh oh!

ghost commented Sep 6, 2022

Uh oh!

ghost commented Sep 6, 2022

Uh oh!

lambdageek commented Sep 7, 2022

Uh oh!

Uh oh!

lambdageek commented Sep 7, 2022

Uh oh!

lambdageek commented Sep 7, 2022

Uh oh!

lambdageek commented Sep 7, 2022

Uh oh!

radicalSep 7, 2022

Choose a reason for hiding this comment

Uh oh!

lambdageekSep 7, 2022

Choose a reason for hiding this comment

Uh oh!

radicalSep 7, 2022

Choose a reason for hiding this comment

Uh oh!

radicalSep 7, 2022

Choose a reason for hiding this comment

Uh oh!

steveisokSep 7, 2022

Choose a reason for hiding this comment

Uh oh!

radical commented Sep 7, 2022

Uh oh!

carlossanlop commented Sep 7, 2022

Uh oh!

radical commented Sep 7, 2022

Uh oh!

carlossanlop commented Sep 7, 2022

Uh oh!

lambdageek commented Sep 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lambdageek commented Sep 8, 2022

Uh oh!

lambdageek commented Sep 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lambdageek commented Sep 7, 2022•
edited
Loading