-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[release/7.0][wasm-mt] Fix pack/build issues in threaded builds #75171
Conversation
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. |
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsManual backport of #75162 Addresses the following issues in #74654:
|
…ithread' Also define the perf-tracing feature flag for the threaded and perf-tracing build variants.
not WasmEnabelThreading. Conversely, use `WasmEnablePerfTracing`, not `WasmEnablePerfTrace`.
0dfaba7
to
85789bd
Compare
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
Outdated
Show resolved
Hide resolved
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
WasmBuildTests is now failing because this hack is no longer necessary and it's messing up the IncludedWorkloadManifests.txt file. The newest SDK that runtime/src/tasks/WorkloadBuildTasks/InstallWorkloadFromArtifacts.cs Lines 143 to 173 in 9dda765
|
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)
Pushed a fix to this PR to remove the hack. Separately opened #75198 to remove the hack on |
This should be ready to go now. Could I get a review and approval @radical @lewing @marek-safar |
<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> |
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.
Just wondering - should we have WasmEnableThreading
to match WasmEnablePerfTrac*ing*
?
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.
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.
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.
@lewing @steveisok thoughts?
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.
In net8 we could try an item with list of features to enable, like the runtime components stuff.
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 don't have any good suggestions :-).
WBT has failures on windows due to networking issues, so trying once more. |
@marek-safar we need an approval please. |
@carlossanlop this is ready to go |
@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. |
Update pushed I need to push one more commit here. |
@carlossanlop tell mode. this is to support wasm multithreading, which is a preview feature in net7 |
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
Manual backport of #75162
Addresses the following issues in #74654:
dotnet.worker.js
missing from the runtime packWasmEnableThreading
instead ofWasmEnableThreads
dotnet new wasmbrowser
createsbrowser.csproj
, not<dirname>.csproj