[browser] CoreCLR in-tree relink

[maraf]

Contributes to #123670
Contributes to #126100

[pavelsavara]

It would be good to enable some of the WBT which are currently disabled because of this.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

[Copilot]

The Exists($(EMSDK_PATH)) check is missing quotes around the path. If EMSDK_PATH contains spaces (common on Windows), the condition can evaluate incorrectly and produce a misleading toolchain-missing error. Use Exists('$(EMSDK_PATH)') here for correct MSBuild parsing.

[Copilot]

This looks like a temporary backup of BrowserWasmApp.CoreCLR.targets and it is not referenced anywhere in the repo. Keeping a full duplicate of build logic risks divergence and increases maintenance/packaging noise. Please remove this file (or replace it with a much smaller design note under a docs/research location if needed).

[Copilot]

$(_EmccExportedRuntimeMethods) is being formatted as a quoted bracketed array (e.g. "[BROWSER_HOST,'FS',...]"), but the emcc invocation used by CoreCLR’s browserhost build expects a comma-separated list (see browserhost/CMakeLists.txt: -sEXPORTED_RUNTIME_METHODS=BROWSER_HOST,${CMAKE_EMCC_EXPORTED_RUNTIME_METHODS}). As written, the linker will receive an invalid value and fail or export the wrong runtime methods. Please change this to emit the same comma-separated format as CMake (including the required BROWSER_HOST prefix) and avoid embedding extra quotes/brackets.

Suggested change

	<_EmccExportedRuntimeMethods>"[BROWSER_HOST,@(EmccExportedRuntimeMethod -> '%27%(Identity)%27', ',')]"</_EmccExportedRuntimeMethods>
	<_EmccExportedRuntimeMethods>BROWSER_HOST,@(EmccExportedRuntimeMethod -> '%(Identity)', ',')</_EmccExportedRuntimeMethods>

[Copilot]

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated 2 comments.

[Copilot]

The runtime pack JS input libSystem.Native.Browser.extpost.js is included unconditionally. If the runtime pack being used doesn't contain this file (e.g., older pack, partial build, or CopyWasmNativeFiles failing), the emcc link will fail with a missing input file. Consider guarding this item with an Exists(...) condition and/or emitting a clear error when it’s required for relinking but not present.

Suggested change

	<_CoreCLRJsLibrary Include="$(MicrosoftNetCoreAppRuntimePackRidNativeDir)libSystem.Native.Browser.extpost.js" Kind="extern-post-js" />
	<_CoreCLRJsLibrary Include="$(MicrosoftNetCoreAppRuntimePackRidNativeDir)libSystem.Native.Browser.extpost.js"
	Kind="extern-post-js"
	Condition="Exists('$(MicrosoftNetCoreAppRuntimePackRidNativeDir)libSystem.Native.Browser.extpost.js')" />

[Copilot]

This file looks like a large, generated/one-off research artifact (multi-thousand-line JSON test result dump). Checking this into the main repo will significantly bloat the PR and future clones without providing build/runtime value. Consider removing it from the PR (or replacing with a short summarized markdown report) and attaching the raw results to the PR/issue instead.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 22 changed files in this pull request and generated no new comments.

[github-actions]

🤖 Copilot Code Review — PR #125607

Note

This review was generated by GitHub Copilot using automated code analysis.

Holistic Assessment

Motivation: This PR implements per-app native re-linking for CoreCLR browser-wasm, matching the capability Mono already has. The motivation is clear and justified — without this, CoreCLR WASM apps cannot include custom native code via NativeFileReference.

Approach: The approach is sound — it mirrors the existing Mono build pipeline architecture while remaining self-contained (no import of WasmApp.Common.targets). Link flags correctly match src/native/corehost/browserhost/CMakeLists.txt. The test category split (native for Mono-only, coreclr-native for CoreCLR-capable) integrates cleanly with the existing xUnit trait filtering in the .csproj.

Summary: ⚠️ Needs Changes. The PR contains committed research artifacts and a backup file that must be removed before merge. There are also a few build system issues worth addressing.

---

Detailed Findings

❌ Committed research artifacts and backup file — Must be removed

The branch includes 7 files in a .research/ directory (6,211 lines total) and a BrowserWasmApp.CoreCLR.targets.backup file. These are development/analysis artifacts that should not be merged:

• .research/build-wasm-browser-sample.md
• .research/native-build-analysis.md
• .research/native-coreclr-test-report.md
• .research/native-coreclr-test-results.json
• .research/runtime-native-build-analysis.md
• .research/wasm-native-build-analysis.md
• .research/wasm-native-build-binlog-analysis.md
• src/mono/browser/build/BrowserWasmApp.CoreCLR.targets.backup

These should be removed in a cleanup commit before merge.

⚠️ WasmDirSep property is undefined — Glob pattern won't match

In BrowserWasmApp.CoreCLR.targets line 166:

<_SystemRuntimePathItem Include="$(MicrosoftNetCoreAppRuntimePackRidDir)lib$(WasmDirSep)net*$(WasmDirSep)System.Runtime.dll" />

WasmDirSep is not defined anywhere in the repository. This results in the glob pattern ...libnet*System.Runtime.dll instead of the intended ...lib/net*/System.Runtime.dll. Compare with WasmApp.Common.targets line 316 which uses a literal backslash:

<_SystemRuntimePathItem Include="$(MicrosoftNetCoreAppRuntimePackRidDir)\lib\net*\System.Runtime.dll" />

Impact: MicrosoftNetCoreAppRuntimePackRidLibTfmDir won't be resolved via this glob. The fallback path for CoreLib (line 326, using the native dir) likely compensates, but if any other code depends on MicrosoftNetCoreAppRuntimePackRidLibTfmDir being set, it will fail. Should use \ or / literal separators like the Mono equivalent.

⚠️ Duplicate -fwasm-exceptions -msimd128 flags in the link step

The _CoreCLRLinkNative target passes these flags twice:

1. Via $(_WasmDefaultFlags) on the command line (line 562): -fwasm-exceptions -msimd128
2. Via @(_WasmLinkRsp) which contains @(_EmccCommonFlags) (lines 264-265, 487): same -fwasm-exceptions -msimd128

While emcc tolerates duplicate flags, this is redundant. Consider either removing $(_WasmDefaultFlags) from the Exec command (since the RSP already includes them via _EmccCommonFlags), or removing them from _EmccCommonFlags in the link args.

⚠️ WasmNativeStrip property is set but never consumed

Lines 43-45 set WasmNativeStrip=true for CI AOT builds and the comment mentions avoiding OOM during wasm-opt, but there is no wasm-opt or strip step in the CoreCLR targets. The property is dead code. Either remove it or add the corresponding strip/optimize step.

⚠️ Missing trailing newline in BrowserWasmApp.CoreCLR.targets

The file ends without a trailing newline character (confirmed via hex dump). While not a functional issue, this is a git hygiene concern — it produces "No newline at end of file" warnings in diffs and doesn't match the convention used by other .targets files in the repo.

💡 EnsureXHarnessAvailable() has no process timeout

BuildTestBase.cs line 166 calls process.WaitForExit() without a timeout. If dotnet tool restore hangs (e.g., network issues resolving NuGet packages), the test process will block indefinitely. Consider adding a timeout (e.g., process.WaitForExit(120_000)) with an appropriate error message.

💡 Commit messages include non-conventional entries

Some commit messages in the branch history are unusual (e.g., "Analysis", "Second round", "Cleanup" ×4, "Fix", "The changes are clean and focused. Three surgical edits:"). Consider squashing these into clean, descriptive commits before merge to maintain a clear git history.

✅ Test category changes are correctly structured

The split between [TestCategory("native")] (Mono-only, excluded by CoreCLR CI via -notrait category=native) and [TestCategory("coreclr-native")] (runs on CoreCLR CI) is well-designed. The per-method granularity in DllImportTests, NativeBuildTests, and PInvokeTableGeneratorTests correctly identifies which tests are Mono-specific vs CoreCLR-compatible.

✅ AddCoreClrProjectProperties extraction is a good refactor

The extraction of CoreCLR project property injection into a shared AddCoreClrProjectProperties method (and its use from both CreateWasmTemplateProject and CopyTestAsset) eliminates duplication and ensures consistency. The addition of <UsingBrowserRuntimeWorkload>false</UsingBrowserRuntimeWorkload> is appropriate for CoreCLR projects.

✅ Link flags correctly mirror CMakeLists.txt

The emcc link flags in _CoreCLRWriteLinkRsp (memory sizes, module config, exported functions/methods, JS library files) match src/native/corehost/browserhost/CMakeLists.txt. The library ordering follows the CMake convention with BrowserHost linked first.

✅ eng/native.wasm.targets condition is well-scoped

The Condition="'$(IsBrowserWasmProject)' != 'true'" on the ICU/timezone NuGet package references correctly prevents content file pollution during app-level relink while keeping them available for the runtime build itself. The comment clearly explains the rationale.

✅ corehost.proj change correctly ships extpost.js

Adding libSystem.Native.Browser.extpost.js from the source tree to the runtime pack native directory is necessary for the re-link step. The file exists at the referenced path and the --extern-post-js flag in the link step depends on it being available.

Generated by Code Review for issue #125607 · ◷

[maraf]

Note

This comment was generated with the assistance of GitHub Copilot.

Investigation: Delivering CoreCLR Native Build Targets to Wasm.Build.Tests

As part of enabling template-based native tests (IcuTests, NativeBuildTests, etc.) in the CoreCLR CI pipeline, I investigated how to make BrowserWasmApp.CoreCLR.targets available to standalone test projects created by dotnet new wasmbrowser.

The Problem

Test projects created during Wasm.Build.Tests are standalone — they live in temp directories and use the SDK targets (or Local.Directory.Build.targets in no-workload mode). Currently, BrowserWasmApp.CoreCLR.targets is only imported via WasmApp.InTree.targets for in-tree repo builds. When a test project sets <NativeFileReference>, the old WorkloadManifest.targets just warns (because WasmNativeWorkloadAvailable != true) and native relinking never happens.

This means tests like BuildWithUndefinedNativeSymbol and ProjectWithDllImportsRequiringMarshalIlGen pass/fail for the wrong reasons — native build is silently skipped.

Root Cause: Missing Import Chain

BrowserWasmApp.CoreCLR.targets depends on repo-level paths that don't exist in standalone test projects:

BrowserWasmApp.CoreCLR.targets
  ├── Import: $(RepositoryEngineeringDir)native.wasm.targets   ← repo path
  ├── UsingTask: $(WasmAppBuilderTasksAssemblyPath)             ← task DLL path
  └── Import: EmSdkRepo.Defaults.props (needs EMSDK_PATH)      ← emscripten

How It Works Today

Context	How targets are delivered
In-tree builds	WasmApp.InTree.targets imports BrowserWasmApp.CoreCLR.targets when RuntimeFlavor=CoreCLR
Mono workload	wasm-tools workload installs all targets + tasks into the SDK
Helix CI (CoreCLR)	Correlation payload ships build/wasm/, build/wasm-shared/, build/WasmAppBuilder/, build/emsdk/; run script sets env vars

Proposed Approach

Wire up BuildEnvironment.cs + Local.Directory.Build.targets to mirror the Helix correlation payload pattern for local runs:

1. BuildEnvironment.cs (CoreCLR mode): Set environment/MSBuild properties pointing to repo artifacts:

   • RepositoryEngineeringDir → {repoRoot}/eng/
   • WasmAppBuilderTasksAssemblyPath → artifacts/bin/WasmAppBuilder/...
   • EMSDK_PATH → from system or artifacts/obj/emsdk/

2. Local.Directory.Build.targets: Add conditional import of BrowserWasmApp.CoreCLR.targets when RuntimeFlavor=CoreCLR

3. AddCoreClrProjectProperties(): Set <RuntimeFlavor>CoreCLR</RuntimeFlavor> in each test project csproj

This keeps the same code paths for local dev and Helix CI — just different values for the path properties.

What's Already Done (This PR Branch)

• ✅ Changed test traits from native → coreclr-native (5 test classes)
• ✅ Added EnsureWasmTemplatesInstalled() to auto-install WASM templates (mirrors xharness pattern)
• ✅ Added test classes to BuildWasmAppsJobsListCoreCLR.txt
• ✅ Verified template install + project creation works end-to-end

What Remains

• Implement the target delivery mechanism described above
• Verify BuildWithUndefinedNativeSymbol and ProjectWithDllImportsRequiringMarshalIlGen actually perform native linking
• Handle AOT-dependent tests (need skip conditions for no-workload CoreCLR mode)
• Handle ICU sharding tests (need native build for ICU data replacement)