Conversation
…roperties Co-authored-by: maraf <10020471+maraf@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
CoreCLR browser-wasm has no workload-labelled KnownRuntimePack, so the SDK resolves its runtime pack from KnownFrameworkReference's LatestRuntimeFrameworkVersion, which the workload manifest never overrode. Builds picked the SDK-bundled runtime pack instead of the one the workload had just installed, pairing a stale runtime pack with the locally built WebAssembly SDK. Override LatestRuntimeFrameworkVersion for browser-wasm when UseMonoRuntime is false, and make the runtime pack name in the non-workload WasmOverridePacks.targets flavour-aware. This is what the per-test KnownFrameworkReference workaround in WasmTemplateTestsBase was standing in for. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate findings concern missing browser RID metadata and insufficient CoreCLR runtime-pack test coverage.
Pull request overview
Simplifies CoreCLR project-property setup and moves browser-Wasm runtime-pack handling into shared targets.
Changes:
- Removes obsolete
extraItems/insertAtEndmutations and updates callers. - Adds runtime-pack selection for CoreCLR and Mono.
- Adds CoreCLR browser-Wasm framework-version handling.
File summaries
| File | Changes and final review comments |
|---|---|
src/mono/wasm/Wasm.Build.Tests/Templates/WasmTemplateTestsBase.cs |
Removes obsolete project mutations and simplifies the helper signature. |
src/mono/wasm/Wasm.Build.Tests/data/WasmOverridePacks.targets |
Adds runtime-pack selection. Moderate (1 vote): CoreCLR selection lacks focused automated coverage. Nit (1 vote): Document the behavioral changes and validation. |
src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WorkloadManifest.targets.in |
Adds CoreCLR framework-version handling. Moderate (1 vote): Preserve the browser-wasm RuntimePackRuntimeIdentifiers metadata. |
Review details
Suppressed comments (3)
src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WorkloadManifest.targets.in:260
- This replacement drops the
RuntimePackRuntimeIdentifiersupdate that the removed helper used to add (browser-wasm;%(RuntimePackRuntimeIdentifiers)). The repository's localKnownFrameworkReferencedefinition does not includebrowser-wasm(eng/targetingpacks.targets:43-52), so on that SDK path the CoreCLR browser runtime pack is not selected in the no-workload/CoreCLR lane. Preserve the browser RID metadata in this CoreCLR update as well.
<KnownFrameworkReference Update="Microsoft.NETCore.App"
Condition="'$(RuntimeIdentifier)' == 'browser-wasm' and '$(UseMonoRuntime)' == 'false'">
<LatestRuntimeFrameworkVersion Condition="'%(KnownFrameworkReference.TargetFramework)' == '${NetVersion}.0'">$(_MonoWorkloadRuntimePackPackageVersion)</LatestRuntimeFrameworkVersion>
src/mono/wasm/Wasm.Build.Tests/data/WasmOverridePacks.targets:19
- The PR description presents this as only a helper/signature cleanup, but this change also alters runtime-pack selection for CoreCLR and the workload manifest adds a new framework-version override. These are behavioral changes rather than incidental cleanup; please document their purpose and validation (or split them) so reviewers can assess the added resolution behavior.
<_WasmOverrideRuntimePackName Condition="'$(UseMonoRuntime)' == 'false'">Microsoft.NETCore.App.Runtime.$(RuntimeIdentifier)</_WasmOverrideRuntimePackName>
<_WasmOverrideRuntimePackName Condition="'$(_WasmOverrideRuntimePackName)' == ''">Microsoft.NETCore.App.Runtime.Mono.$(RuntimeIdentifier)</_WasmOverrideRuntimePackName>
src/mono/wasm/Wasm.Build.Tests/data/WasmOverridePacks.targets:19
- This new CoreCLR/Mono package-name selection is not covered by the current test suite:
UseWBTOverridePackTargetsis only declared and is never enabled anywhere in the repository, so no automated test exercises theUseMonoRuntime == falsebranch or verifies the resultingResolvedRuntimePackpath. Add a focused CoreCLR workload test that enables the override and asserts the resolved pack, or otherwise provide equivalent coverage for this target.
<_WasmOverrideRuntimePackName Condition="'$(UseMonoRuntime)' == 'false'">Microsoft.NETCore.App.Runtime.$(RuntimeIdentifier)</_WasmOverrideRuntimePackName>
<_WasmOverrideRuntimePackName Condition="'$(_WasmOverrideRuntimePackName)' == ''">Microsoft.NETCore.App.Runtime.Mono.$(RuntimeIdentifier)</_WasmOverrideRuntimePackName>
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
Conflict in WasmTemplateTestsBase.cs: #133413 extended the per-test pack override target this branch removes, renaming it to _UpdateKnownCoreClrWebAssemblyPacks and adding a KnownCrossgen2Pack version override, because crossgen2 is now resolved through the SDK rather than installed by the workload. Resolved by keeping the removal and moving the crossgen2 override into the workload manifest alongside the runtime pack override, so the behaviour #133413 added is preserved for every CoreCLR wasm consumer rather than only inside Wasm.Build.Tests. Nothing else patched KnownCrossgen2Pack, so keeping the removal alone would have silently regressed it. The override is scoped to browser-wasm with UseMonoRuntime false, leaving Mono wasm and non-wasm ReadyToRun builds untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (1)
src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WorkloadManifest.targets.in:260
- Could we add a regression case that asserts the versions of both the resolved CoreCLR browser-wasm runtime pack and
ResolvedCrossgen2Pack?WasmSdkBasedProjectProvider.AssertRuntimePackPathonly validates the runtime-pack path in workload lanes, whileCoreCLRWasmNativeDefaultsTests.NativeRelinkResolvesCrossgen2WithoutReadyToRunchecks that a generator exists but not that its package version matches the runtime pack. A future item-ordering or version regression in either override could therefore pass the current tests.
<KnownCrossgen2Pack Update="@(KnownCrossgen2Pack)"
Condition="'$(RuntimeIdentifier)' == 'browser-wasm' and '$(UseMonoRuntime)' == 'false'">
<Crossgen2PackVersion Condition="'%(KnownCrossgen2Pack.TargetFramework)' == '${NetVersion}.0'">$(_MonoWorkloadRuntimePackPackageVersion)</Crossgen2PackVersion>
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
| <KnownCrossgen2Pack Update="@(KnownCrossgen2Pack)" | ||
| Condition="'$(RuntimeIdentifier)' == 'browser-wasm' and '$(UseMonoRuntime)' == 'false'"> | ||
| <Crossgen2PackVersion Condition="'%(KnownCrossgen2Pack.TargetFramework)' == '${NetVersion}.0'">$(_MonoWorkloadRuntimePackPackageVersion)</Crossgen2PackVersion> | ||
| </KnownCrossgen2Pack> |
There was a problem hiding this comment.
should we do this in WasmOverridePacks.targets instead?
I'm fine with merging this to unblock testing but that feels like the more correct spot to me (as we want to ideally get rid of the runtime copy of the workload manifest)
There was a problem hiding this comment.
For mono, in production, we needed to enforce that runtime pack, aot compiler and sdk pack, are all in the same version. Because of that we have this "production" override.
I thought we would do the same with CoreCLR. Is that correct or not?
There was a problem hiding this comment.
I don't think we have a need to, since coreclr/crossgen doesn't have that tight coupling
There was a problem hiding this comment.
We are not actually using the WasmOverridePacks.targets, see UseWBTOverridePackTargets = false and also #91350, but if we don't include the crossgen in workload, overriding the version in workload isn't correct.
Problem
CoreCLR
browser-wasmbuilds resolved the wrong runtime pack. The build picked the runtime pack bundled with the SDK (12.0.0-alpha.*) instead of the one the workload had just installed (12.0.0-ci), pairing a stale runtime pack with a locally built WebAssembly SDK.The SDK picks the runtime pack differently per flavour:
KnownRuntimePackwhoseRuntimePackLabels == 'Mono'. The workload manifest patches itsLatestRuntimeFrameworkVersionto the in-workload version, so Mono gets the right pack.KnownRuntimePack. Resolution falls back toKnownFrameworkReference[Microsoft.NETCore.App].LatestRuntimeFrameworkVersion— which the manifest never patched, so it stayed at the SDK-bundled version.Wasm.Build.Testspapered over this with per-test pack-version overrides injected into every generated project (added in #122646). That kept the test lanes working but left every other CoreCLR wasm consumer with the same latent problem.Changes
WorkloadManifest.targets.in— patch the pack versions the SDK uses to resolve CoreCLR browser-wasm, alongside the existingTargetingPackVersionandKnownWebAssemblySdkPackoverrides:KnownFrameworkReference.LatestRuntimeFrameworkVersion— the runtime pack.KnownCrossgen2Pack.Crossgen2PackVersion— crossgen2 generates the wasm P/Invoke call helpers, which encode struct sizes that have to agree with the headers in the runtime pack, so it must not drift from it.Both are scoped to
browser-wasmwithUseMonoRuntimefalse, leaving Mono wasm and non-wasm ReadyToRun builds untouched.WasmOverridePacks.targets— the non-workload test path hardcodedMicrosoft.NETCore.App.Runtime.Mono.$(RuntimeIdentifier), so CoreCLR misresolved there too. The pack name is now flavour-aware.WasmTemplateTestsBase.cs— with the manifest fixed, the per-test workaround is redundant and is removed:KnownFrameworkReferenceitem block — superseded by the manifest fix._UpdateKnownCoreClrWebAssemblyPackstarget —KnownWebAssemblySdkPackwas already covered by the manifest;KnownCrossgen2Packis covered by the manifest change above.RuntimePackRuntimeIdentifiers— was already redundant; the SDK's bundled value already ends with;browser-wasm.AddCoreClrProjectPropertiesreduces to settingUseMonoRuntime, so it no longer needsextraItems/insertAtEnd. Both call sites updated.Interaction with #133413
#133413 moved crossgen2 acquisition from the workload to the SDK and extended the per-test override target with a
KnownCrossgen2Packversion patch. Merging it here conflicted with the removal above.Nothing outside that target patched
KnownCrossgen2Pack, so removing it alone would have silently regressed #133413 — CoreCLR wasm builds would have resolved the SDK-bundled crossgen2 against a locally built runtime pack. The override moved into the manifest instead, which preserves the behaviour for every CoreCLR wasm consumer rather than only insideWasm.Build.Tests.Validation
Root cause was established by diffing MSBuild binlogs from the Mono and CoreCLR
WasmBuildTestsHelix work items of the same CI run. The evaluations are identical exceptUseMonoRuntimeand the resultingLatestRuntimeFrameworkVersion.Both manifest overrides were verified against a harness reproducing the SDK's item shape:
browser-wasmbrowser-wasmKnownRuntimePacklinux-x64)Only the Current manifest is changed; the net6–net10 manifests predate CoreCLR-on-wasm.
CI is the real validation here — a full wasm workload build plus emsdk was not run locally.
Notes
PackageDirectoryinWasmOverridePacks.targetsstill uses%(Identity), which is what keepsMono.multithread.browser-wasmworking.WasmOverridePacks.targetsis currently unreachable —UseWBTOverridePackTargetsdefaults tofalseand nothing sets it — so CI will not exercise that half.Note
This PR description was generated with the assistance of GitHub Copilot.