[wasm][mt] Fix System.Threading.Tasks.Parallel build #97505

radekdoulik · 2024-01-25T12:02:58Z

Fixes #91541
Fixes #91579
Fixes #91581
Fixes #91582
Fixes #91583
Fixes #97396
Fixes #97440

ilonatommy · 2024-01-25T12:25:10Z

src/libraries/System.Threading.Tasks.Parallel/src/System.Threading.Tasks.Parallel.csproj

@@ -7,7 +7,7 @@
  </PropertyGroup>
  <PropertyGroup>
    <TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
-    <FeatureWasmThreads Condition="'$(TargetPlatformIdentifier)' == 'browser' and '$(MonoWasmBuildVariant)' == 'multithread'">true</FeatureWasmThreads>
+    <FeatureWasmThreads Condition="'$(TargetOS)' == 'browser' and '$(MonoWasmBuildVariant)' == 'multithread'">true</FeatureWasmThreads>


what is the old thing, TargetPlatformIdentifier? Was it empty? I found:

runtime/docs/coding-guidelines/project-guidelines.md

Line 117 in 135fec0

4. Set the `TargetPlatformIdentifier` property in the project to be able to condition on it in properties in the project file.

, that it might be empty if it's not set explicitly

Why not do it consistently in all project files if TargetPlatformIdentifier doesn't work well ?

it is the platform identifier from $(TargetFramework). I think it would have the value 'browser' if the target framework was something like net9.0-browser

That's interesting. TargetFramework is not in the form of net9.0-browser. We have quite a few places where we use the same condition, not connected with threading. It should mean that they are not triggered, e.g.

runtime/src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj

Line 51 in 135fec0

<ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'unix' or '$(TargetPlatformIdentifier)' == 'browser' ">

runtime/src/libraries/System.Runtime.InteropServices.JavaScript/src/System.Runtime.InteropServices.JavaScript.csproj

Line 19 in 135fec0

<EmitCompilerGeneratedFiles Condition="'$(Configuration)' == 'Debug' and '$(TargetPlatformIdentifier)' == 'browser'">true</EmitCompilerGeneratedFiles>

@radekdoulik i didn't understand why/how you resolved this. We have many same conditions in the other project files. Do they all work or not ? Could we have them consistent ?

We discourage using TargetOS to vary builds (except in some special cases for tests) because it defaults to the host OS if a target OS is not explicitly passed in, which is the case if you build a library from VS or in the "allConfiguration" CI build.

The correct solution here is to add $(NetCoreAppCurrent)-browser TFM and use TargetPlatformIdentifier in the condition.

@akoeplinger, @lewing I am curious, how are the produced assemblies used further in the process. So in this case, where does the assembly built with net9.0 TFM ends up and where the net9.0-browser one. I imagine they end up in the right Sdk and workload so that users projects get the right assembly?

I see many <IgnoreForCI Condition="'$(TargetOS)' == 'browser'">true</IgnoreForCI> but I don't know what IgnoreForCI does.

There are also many <DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(TargetOS)' == 'browser'">true</DebuggerSupport>

Overall we have '$(TargetOS)' != 'browser' 15x and '$(TargetOS)' == 'browser' 126x

Should we fix it all ?

So in this case, where does the assembly built with net9.0 TFM ends up and where the net9.0-browser one. I imagine they end up in the right Sdk and workload so that users projects get the right assembly?

Yes the build system will select the right assets when building a specific runtime pack, based on the RID fallback graph in PortableRuntimeIdentifierGraph.json

[..] but I don't know what IgnoreForCI does.

IgnoreForCI skips sending a test project to helix, it is just an optimization for projects where all tests are disabled/skipped for whatever reason so that we don't waste resources sending and running a helix workitem where 0 tests are executed.

In theory the correct way would be to add additional browser TFMs here too but it'd make build times longer and test projects are always built for a specific target OS so we let it slide. The important part is that we only do this in tests, not in the src csproj. I've checked and only System.Linq.Parallel.csproj is affected (#97539 has the fix).

And just to be clear: the current way "works" because in CI/official builds we'll always have TargetOS=browser, the correct way is mostly important for VS scenarios e.g. you should be able to use the TFM dropdown in VS to select a different config and hit build to build the "browser' version, this likely won't work today anyway because of additional properties like MonoWasmBuildVariant.

pavelsavara · 2024-01-25T12:44:26Z

/azp run runtime-wasm

azure-pipelines · 2024-01-25T12:44:35Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2024-01-25T12:46:21Z

note that you need to look into AzDO runtime-wasm for the MT libraries leg, because it doesn't report failures on the github.

I will change it soon, see

ghost · 2024-01-25T13:19:33Z

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

Issue Details

Fixes #91541, #91581, #91582, #91583 and #91579

Author:	radekdoulik
Assignees:	radekdoulik
Labels:	`arch-wasm`, `area-VM-threading-mono`, `os-browser`
Milestone:	-

pavelsavara · 2024-01-25T15:16:33Z

Orange means something is broken.

System.Runtime.InteropServices.JavaScript.Tests I'm working on in #97441 and it's unrelated to you

System.Numerics.Tensors.Tests and System.Numerics.Tensors.Net8.Tests are broken by #97295 unrelated to you

System.Threading.Tasks.Extensions.Tests is #96545

radekdoulik · 2024-01-25T16:12:04Z

Orange means something is broken.

System.Runtime.InteropServices.JavaScript.Tests I'm working on in #97441 and it's unrelated to you

System.Numerics.Tensors.Tests and System.Numerics.Tensors.Net8.Tests are broken by #97295 unrelated to you

System.Threading.Tasks.Extensions.Tests is #96545

these are unrelated, right, so it should be fine to merge it?

pavelsavara

Please fix all the other projects which could have the same problem before you merge this.

radekdoulik · 2024-01-25T23:37:30Z

/azp run runtime-wasm

azure-pipelines · 2024-01-25T23:37:41Z

Azure Pipelines successfully started running 1 pipeline(s).

radekdoulik · 2024-01-25T23:57:14Z

Please fix all the other projects which could have the same problem before you merge this.

There are few more places to fix. I will do it in a following PR as it might need more work in case there are build issues.

> git grep FeatureWasmThreads .|grep TargetPlatformIdentifier|cut -f1 -d:|while read proj; do echo --- $proj ---; grep \<TargetFramework $proj; done
--- src/libraries/System.Net.Http/src/System.Net.Http.csproj ---
    <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-linux;$(NetCoreAppCurrent)-osx;$(NetCoreAppCurrent)-freebsd;$(NetCoreAppCurrent)-maccatalyst;$(NetCoreAppCurrent)-ios;$(NetCoreAppCurrent)-tvos;$(NetCoreAppCurrent)-browser;$(NetCoreAppCurrent)-wasi;$(NetCoreAppCurrent)-illumos;$(NetCoreAppCurrent)-solaris;$(NetCoreAppCurrent)-haiku;$(NetCoreAppCurrent)-android;$(NetCoreAppCurrent)</TargetFrameworks>
--- src/libraries/System.Net.WebSockets.Client/src/System.Net.WebSockets.Client.csproj ---
    <TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppCurrent)-browser</TargetFrameworks>
--- src/libraries/System.Runtime.InteropServices.JavaScript/src/System.Runtime.InteropServices.JavaScript.csproj ---
    <TargetFrameworks>$(NetCoreAppCurrent)-browser;$(NetCoreAppCurrent)</TargetFrameworks>
--- src/libraries/System.Runtime.InteropServices.JavaScript/src/System.Runtime.InteropServices.JavaScript.csproj ---
    <TargetFrameworks>$(NetCoreAppCurrent)-browser;$(NetCoreAppCurrent)</TargetFrameworks>
--- src/libraries/System.Runtime.InteropServices.JavaScript/src/System.Runtime.InteropServices.JavaScript.csproj ---
    <TargetFrameworks>$(NetCoreAppCurrent)-browser;$(NetCoreAppCurrent)</TargetFrameworks>
--- src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System.Runtime.InteropServices.JavaScript.Tests.csproj ---
    <TargetFrameworks>$(NetCoreAppCurrent)-browser</TargetFrameworks>
--- src/libraries/System.Threading.Tasks.Parallel/src/System.Threading.Tasks.Parallel.csproj ---
    <TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
--- src/libraries/System.Threading.Thread/ref/System.Threading.Thread.csproj ---
    <TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
--- src/libraries/System.Threading/ref/System.Threading.csproj ---
    <TargetFramework>$(NetCoreAppCurrent)</TargetFramework>

I was thinking about adding the logic to the src/libraries/Directory.Build.props. That would probably not work though as it is imported too early. But maybe we could try to add a new target to the Directory.Build.targets to run before Build or Csc target to set the define we need. I will play with it later.

radekdoulik · 2024-01-26T00:13:01Z

/azp run runtime-wasm

azure-pipelines · 2024-01-26T00:13:11Z

Azure Pipelines successfully started running 1 pipeline(s).

radekdoulik · 2024-01-26T00:14:31Z

/azp run runtime-wasm

azure-pipelines · 2024-01-26T00:14:42Z

Azure Pipelines successfully started running 1 pipeline(s).

[wasm][mt] Fix System.Threading.Tasks.Parallel build

5644f37

Fixes dotnet#91583 and dotnet#91579

dotnet-issue-labeler bot added the area-VM-threading-mono label Jan 25, 2024

ghost assigned radekdoulik Jan 25, 2024

radekdoulik requested review from pavelsavara and ilonatommy January 25, 2024 12:18

ilonatommy reviewed Jan 25, 2024

View reviewed changes

radekdoulik added 2 commits January 25, 2024 13:36

Enable 5 more tests

8f40d96

Merge branch 'main' into pr-wasm-mt-fix-sttp-build

90ac6c9

pavelsavara added arch-wasm WebAssembly architecture os-browser Browser variant of arch-wasm labels Jan 25, 2024

pavelsavara added this to the 9.0.0 milestone Jan 25, 2024

This was referenced Jan 25, 2024

Tracking issue for "WORKLOAD TIMED OUT" #90309

Closed

Segmentation fault in System.Text.RegularExpressions.Tests #93206

Closed

System.Net.Http.Functional.Tests.HttpMetricsTest_Http11_Async_HttpMessageInvoker Test failure #96407

Closed

radekdoulik requested review from lewing and ilonatommy January 25, 2024 17:59

pavelsavara approved these changes Jan 25, 2024

View reviewed changes

Merge branch 'main' into pr-wasm-mt-fix-sttp-build

7d52b40

Feedback

ec393b7

Fix typo

3a840ad

This was referenced Jan 26, 2024

[browser][MT] GC deadlock #96190

Closed

Tests crashing in CI with no dump: exit code 137 means SIGKILL Killed #97049

Closed

radekdoulik merged commit c105e7d into dotnet:main Jan 26, 2024
130 of 137 checks passed

radekdoulik mentioned this pull request Jan 26, 2024

[wasm][mt] Fix few libraries projects #97539

Merged

github-actions bot locked and limited conversation to collaborators Feb 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm][mt] Fix System.Threading.Tasks.Parallel build #97505

[wasm][mt] Fix System.Threading.Tasks.Parallel build #97505

radekdoulik commented Jan 25, 2024 •

edited by akoeplinger

Loading

ilonatommy Jan 25, 2024 •

edited

Loading

pavelsavara Jan 25, 2024

radekdoulik Jan 25, 2024

ilonatommy Jan 25, 2024

pavelsavara Jan 25, 2024

akoeplinger Jan 25, 2024 •

edited

Loading

radekdoulik Jan 26, 2024

pavelsavara Jan 26, 2024

akoeplinger Jan 26, 2024 •

edited

Loading

akoeplinger Jan 26, 2024 •

edited

Loading

pavelsavara commented Jan 25, 2024

azure-pipelines bot commented Jan 25, 2024

pavelsavara commented Jan 25, 2024

ghost commented Jan 25, 2024

pavelsavara commented Jan 25, 2024

radekdoulik commented Jan 25, 2024

pavelsavara left a comment

radekdoulik commented Jan 25, 2024

azure-pipelines bot commented Jan 25, 2024

radekdoulik commented Jan 25, 2024

radekdoulik commented Jan 26, 2024

azure-pipelines bot commented Jan 26, 2024

radekdoulik commented Jan 26, 2024

azure-pipelines bot commented Jan 26, 2024

[wasm][mt] Fix System.Threading.Tasks.Parallel build #97505

[wasm][mt] Fix System.Threading.Tasks.Parallel build #97505

Conversation

radekdoulik commented Jan 25, 2024 • edited by akoeplinger Loading

ilonatommy Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

pavelsavara Jan 25, 2024

Choose a reason for hiding this comment

radekdoulik Jan 25, 2024

Choose a reason for hiding this comment

ilonatommy Jan 25, 2024

Choose a reason for hiding this comment

pavelsavara Jan 25, 2024

Choose a reason for hiding this comment

akoeplinger Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

radekdoulik Jan 26, 2024

Choose a reason for hiding this comment

pavelsavara Jan 26, 2024

Choose a reason for hiding this comment

akoeplinger Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

akoeplinger Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

pavelsavara commented Jan 25, 2024

azure-pipelines bot commented Jan 25, 2024

pavelsavara commented Jan 25, 2024

ghost commented Jan 25, 2024

pavelsavara commented Jan 25, 2024

radekdoulik commented Jan 25, 2024

pavelsavara left a comment

Choose a reason for hiding this comment

radekdoulik commented Jan 25, 2024

azure-pipelines bot commented Jan 25, 2024

radekdoulik commented Jan 25, 2024

radekdoulik commented Jan 26, 2024

azure-pipelines bot commented Jan 26, 2024

radekdoulik commented Jan 26, 2024

azure-pipelines bot commented Jan 26, 2024

radekdoulik commented Jan 25, 2024 •

edited by akoeplinger

Loading

ilonatommy Jan 25, 2024 •

edited

Loading

akoeplinger Jan 25, 2024 •

edited

Loading

akoeplinger Jan 26, 2024 •

edited

Loading

akoeplinger Jan 26, 2024 •

edited

Loading