Enable runtime-async in CoreCLR System.Private.CoreLib#126594
Enable runtime-async in CoreCLR System.Private.CoreLib#126594
Conversation
…scv64, loongarch64, and Mono Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/46f839bd-54df-446d-8b31-41b32d813c8c Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
|
Tagging subscribers to this area: @agocke, @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
Enables the Roslyn runtime-async=on feature flag when building CoreCLR’s System.Private.CoreLib, aligning CoreCLR CoreLib behavior with existing runtime-async enablement elsewhere while excluding known-unsupported architectures and Mono builds.
Changes:
- Add
runtime-async=ontoFeaturesfor CoreCLRSystem.Private.CoreLibbuilds. - Gate the feature behind MSBuild conditions to exclude
riscv64,loongarch64, andRuntimeFlavor=Mono.
VSadov
left a comment
There was a problem hiding this comment.
LGTM. Hopefully no surprises when tests get to run.
src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
Outdated
Show resolved
Hide resolved
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Are we still in the window of preview3 so that this can be backported to it? |
| <!-- RISC-V: https://github.com/dotnet/runtime/issues/124934 --> | ||
| <!-- LoongArch: https://github.com/dotnet/runtime/issues/124935 --> | ||
| <PropertyGroup Condition="'$(TargetArchitecture)' != 'riscv64' and '$(TargetArchitecture)' != 'loongarch64'"> | ||
| <Features>$(Features);runtime-async=on</Features> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
The PR description says runtime-async should be excluded for the Mono runtime flavor, but this PropertyGroup condition only excludes riscv64/loongarch64. This can enable runtime-async=on in unsupported Mono scenarios (the shared RuntimeAsyncSupported predicate elsewhere explicitly checks '$(RuntimeFlavor)' != 'Mono', e.g. src/libraries/Directory.Build.targets:131-136). Consider adding '$(RuntimeFlavor)' != 'Mono' to this condition (or otherwise reusing the repo’s existing RuntimeAsyncSupported predicate) so the behavior matches the PR description and existing platform support gating.
|
Note This review was generated by Copilot. 🤖 Copilot Code Review — PR #126594Holistic AssessmentMotivation: Enabling Approach: The approach is correct and minimal — a single Summary: ✅ LGTM. The change is a 6-line addition to a build file that enables Detailed Findings✅ Correctness — Condition matches established patternThe new <PropertyGroup Condition="'$(TargetArchitecture)' != 'riscv64' and '$(TargetArchitecture)' != 'loongarch64'">
<Features>$(Features);runtime-async=on</Features>
</PropertyGroup>is identical to what NativeAOT CoreLib uses (lines 48-50 of ✅ Mono exclusion removal is safeThe second commit removed
✅ Consistency with codebaseThe change aligns with how
💡 Minor — Missing descriptive commentThe NativeAOT CoreLib has a comment
|
| </PropertyGroup> | ||
|
|
||
| <!-- RISC-V: https://github.com/dotnet/runtime/issues/124934 --> | ||
| <!-- LoongArch: https://github.com/dotnet/runtime/issues/124935 --> | ||
| <PropertyGroup Condition="'$(TargetArchitecture)' != 'riscv64' and '$(TargetArchitecture)' != 'loongarch64'"> |
There was a problem hiding this comment.
Both of them were fixed: #125446 and #125114.
| </PropertyGroup> | |
| <!-- RISC-V: https://github.com/dotnet/runtime/issues/124934 --> | |
| <!-- LoongArch: https://github.com/dotnet/runtime/issues/124935 --> | |
| <PropertyGroup Condition="'$(TargetArchitecture)' != 'riscv64' and '$(TargetArchitecture)' != 'loongarch64'"> |
Description
Runtime-async was enabled for
src/libraries(non-mobile/wasm) but was missing from the CoreCLRSystem.Private.CoreLib. This adds theruntime-async=onfeature flag tosrc/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj, matching the existing NativeAOT pattern with architecture and runtime flavor exclusions: