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][mt] Fix System.Threading.Tasks.Parallel build #97505

Merged
merged 6 commits into from
Jan 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5644f37
[wasm][mt] Fix System.Threading.Tasks.Parallel build
radekdoulik Jan 25, 2024
8f40d96
Enable 5 more tests
radekdoulik Jan 25, 2024
90ac6c9
Merge branch 'main' into pr-wasm-mt-fix-sttp-build
radekdoulik Jan 25, 2024
7d52b40
Merge branch 'main' into pr-wasm-mt-fix-sttp-build
radekdoulik Jan 25, 2024
ec393b7
Feedback
radekdoulik Jan 26, 2024
3a840ad
Fix typo
radekdoulik Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/System.Threading.Tasks.Parallel/src/System.Threading.Tasks.Parallel.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

@ilonatommy ilonatommy Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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>

Copy link
Member

Choose a reason for hiding this comment

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

@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 ?

Copy link
Member

@akoeplinger akoeplinger Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

@akoeplinger akoeplinger Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member

@akoeplinger akoeplinger Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

<DefineConstants Condition="'$(FeatureWasmThreads)' == 'true'" >$(DefineConstants);FEATURE_WASM_THREADS</DefineConstants>
</PropertyGroup>
<ItemGroup>
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Threading.Tasks.Parallel/tests/ParallelLoopResultTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,6 @@ private static void OrderablePartitionerForEachPLRTest(

// Perform tests on various combinations of Stop()/Break()
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/91579", typeof(PlatformDetection), nameof(PlatformDetection.IsWasmThreadingSupported))]
public static void SimultaneousStopBreakTests()
{
//
Expand Down
Loading