Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[wasm] Modify workload to pick threading runtime packs #72412

Merged
merged 7 commits into from
Jul 27, 2022
Merged

[wasm] Modify workload to pick threading runtime packs #72412

merged 7 commits into from
Jul 27, 2022

Conversation

steveisok
Copy link
Member

@steveisok steveisok commented Jul 18, 2022 •
edited by lambdageek
Loading

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

[wasm] Modify workload to pick threading runtime packs
279bb62 
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
@ghost ghost assigned steveisok Jul 18, 2022
@lewing
Copy link
Member

lewing commented Jul 19, 2022

Need to make sure WBT knows about the packs

lewing
lewing reviewed Jul 19, 2022
View reviewed changes
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions but should be looks good otherwise

src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Manifest/WorkloadManifest.targets.in
Comment on lines +97 to +98
<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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@radical
Copy link
Member

radical commented Jul 19, 2022

Looks good. I can add a test in WBT.

@radical
Copy link
Member

radical commented Jul 19, 2022

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
Copy link
Member Author

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
Copy link
Member

radical commented Jul 19, 2022

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.

@lewing lewing mentioned this pull request Jul 26, 2022
Closed
@radical
[wasm] Set default value for MonoWasmBuildVariant, for runtime pack n…
d6e57a4 
…uget
@radical
[wasm] Wasm.Build.Tests: add support for multiple runtime packs
6efec93 
- 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
Merge remote-tracking branch 'origin/main' into resolve-wasm-mt-packs
ec9f701
@radical
Copy link
Member

radical commented Jul 27, 2022

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.

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

@radical
[wasm] Wasm.Build.Tests: build runtime pack nugets with different names
0fa442f 
- 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
fix comment
32d220c
@radical
Copy link
Member

radical commented Jul 27, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@runfoapp runfoapp bot mentioned this pull request Jul 27, 2022
Closed
@radical
Copy link
Member

radical commented Jul 27, 2022

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

@radical radical merged commit d7d0e42 into dotnet:main Jul 27, 2022
@steveisok steveisok deleted the resolve-wasm-mt-packs branch July 27, 2022 16:23
@runfoapp runfoapp bot mentioned this pull request Jul 27, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lewing lewing lewing approved these changes

@radical radical radical approved these changes

Assignees

@steveisok steveisok

Labels
area-Build-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@steveisok @lewing @radical