Conversation
Implement per-app native relinking via emcc for CoreCLR browser-wasm, matching the capability Mono already has. Unlike Mono, CoreCLR has no C source files in the runtime pack, so the native build is link-only. Key changes: - BrowserWasmApp.CoreCLR.targets: Complete implementation replacing stubs - Emscripten toolchain setup (_CoreCLRSetupEmscripten, _CoreCLRSetupToolchain) - Build native defaults (WasmBuildNative=false for build, true for publish) - emcc link target with all CoreCLR .a libraries + JS library files - Heap size calculation (default 128MB matching CMakeLists.txt) - WasmBuildApp/WasmTriggerPublishApp/WasmNestedPublishApp target chain - wasm-opt post-link optimization - corehost.proj: Copy libSystem.Native.Browser.extpost.js to runtime pack The implementation is self-contained and does not import WasmApp.Common.targets or BrowserWasmApp.targets. It reuses MSBuild task assemblies via UsingTask. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When WasmBuildNative=true, re-link dotnet.native.wasm from CoreCLR static libraries using emcc, allowing apps to include custom native code via NativeFileReference items. The implementation: - Imports eng/native.wasm.targets for shared Emscripten/ICU/export properties - Sets up Emscripten toolchain and environment - Compiles user native sources (NativeFileReference .c/.cpp) with EmccCompile - Links all CoreCLR .a libraries + user objects into dotnet.native.wasm - Handles both build and publish (nested publish) flows - Link flags mirror src/native/corehost/browserhost/CMakeLists.txt - Gates ICU libraries on InvariantGlobalization - Supports WasmEnableExceptionHandling, WasmEnableSIMD, custom EmccFlags Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Integrate the CoreCLR ManagedToNativeGenerator MSBuild task into the native re-link pipeline. This generates P/Invoke dispatch tables, reverse P/Invoke tables, and interpreter-to-native thunks that are compiled and linked into dotnet.native.wasm. Key changes: - Add _CoreCLRGenerateManagedToNative target that scans managed assemblies for P/Invoke signatures and generates C++ source files - Generate coreclr_compat.h shim header providing type/macro stubs (MethodDesc, PCODE, ULONG, LOG, PORTABILITY_ASSERT) so generated files compile outside the full CoreCLR build context - Use .cpp extensions for generated files (they contain extern "C", namespaces, and other C++ constructs) - Fix CoreLib discovery to check native/ dir first (CoreCLR WASM ships CoreLib there, not in lib/net11.0/) - Fix UsingTask assembly paths: EmccCompile and ManagedToNativeGenerator both live in WasmAppBuilder.dll, not WasmBuildTasks.dll Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Don't compile reverse-pinvoke-table.cpp separately to avoid duplicate symbols with callhelpers-reverse.cpp.o in libcoreclr_static.a - Condition LLD_REPORT_UNDEFINED and ERROR_ON_UNDEFINED_SYMBOLS to Debug only, matching CMakeLists.txt behavior - Add Dependencies metadata to generated source files for incremental builds - Use node$(_ExeExt) for cross-platform compat - Reorder link libraries: libBrowserHost.a first, matching CMakeLists.txt Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t string - Remove MAXIMUM_MEMORY from _EmccCommonFlags (compile+link shared flags); it only belongs in the link step's _EmccLinkStepArgs - Fix PORTABILITY_ASSERT fprintf format: use %25s MSBuild encoding to produce correct %s printf format specifier Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…jection - Add SimpleNativeBuildForCoreCLR test that publishes WasmBrowserRunMainOnly with WasmBuildNative=true, verifying the CoreCLR native build pipeline works - Add CoreCLR property injection to CreateWasmTemplateProject (was missing, only CopyTestAsset had it), enabling template-based tests on CoreCLR - Add UsingBrowserRuntimeWorkload=false to both CoreCLR injection blocks to bypass the wasm-tools workload check (NETSDK1147) that fires when WasmBuildNative=true triggers _WasmNativeWorkloadNeeded Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On Windows, the EmccCompile task's CompilerBinaryPath was set to bare 'emcc', which relies on the PATH environment variable to resolve. However, the PATH constructed in _CoreCLRSetupEmscripten was fragmented by MSBuild item-separator splitting on semicolons, so only the emsdk root directory ended up in PATH — not the emscripten subdirectory where emcc lives. Fix by setting WasmClang to an absolute path using EmscriptenUpstreamEmscriptenPath, matching the existing pattern in the Mono targets (BrowserWasmApp.targets line 173). Also update the link step Exec command to use $(WasmClang) instead of bare 'emcc'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Note This comment was generated with the assistance of GitHub Copilot. Investigation: Delivering CoreCLR Native Build Targets to Wasm.Build.TestsAs part of enabling template-based native tests ( The ProblemTest projects created during This means tests like Root Cause: Missing Import Chain
How It Works Today
Proposed ApproachWire up
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)
What Remains
|
|
Note This comment was generated with the assistance of GitHub Copilot. Implementation Plan: Pack CoreCLR Native Build Files as Helix Payload for WBTProblemCoreCLR WBT native tests ( For Mono WBT, the ApproachMirror the existing Mono Helix correlation payload pattern, adding CoreCLR-specific items and wiring up the test project templates to import the CoreCLR targets. Changes (8 files)1.
|
| <Message Text="Linking CoreCLR WASM with initial memory $(WasmInitialHeapSize) bytes." Importance="High" /> | ||
| <Message Text="Linking with emcc $(EmccLinkOptimizationFlag). This may take a while ..." Importance="High" /> | ||
|
|
||
| <Exec Command='"$(WasmClang)" $(_WasmDefaultFlags) "@$(_WasmLinkRsp)"' |
There was a problem hiding this comment.
The Exec Command value includes literal leading/trailing single-quote characters (Command='"$(WasmClang)" ...'), which will be passed to the command interpreter and can break execution/quoting (notably on Windows). Use XML attribute quoting only (as done elsewhere) and avoid embedding extra quote characters in the command string.
| <Exec Command='"$(WasmClang)" $(_WasmDefaultFlags) "@$(_WasmLinkRsp)"' | |
| <Exec Command=""$(WasmClang)" $(_WasmDefaultFlags) "@$(_WasmLinkRsp)"" |
| return; | ||
|
|
||
| string? templateNupkg = Directory.GetFiles(s_buildEnv.BuiltNuGetsPath, "Microsoft.NET.Runtime.WebAssembly.Templates.*.nupkg") | ||
| .Where(f => !f.EndsWith(".symbols.nupkg", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Directory.GetFiles(...).FirstOrDefault() makes the selected template nupkg non-deterministic when multiple matching packages exist in BUILT_NUGETS_PATH (GetFiles ordering is unspecified). Consider selecting the intended version explicitly (e.g., match the built version) or sorting and picking the newest to avoid flaky installs.
| .Where(f => !f.EndsWith(".symbols.nupkg", StringComparison.OrdinalIgnoreCase)) | |
| .Where(f => !f.EndsWith(".symbols.nupkg", StringComparison.OrdinalIgnoreCase)) | |
| .OrderByDescending(File.GetLastWriteTimeUtc) | |
| .ThenByDescending(f => f, StringComparer.OrdinalIgnoreCase) |
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| UseShellExecute = false | ||
| }; |
There was a problem hiding this comment.
The dotnet new install process is started without the standard s_buildEnv.EnvVars (DOTNET_ROOT/PATH/etc) that other test dotnet invocations use. This can reintroduce the repo/machine environment contamination that BuildEnvironment is explicitly trying to avoid; consider applying s_buildEnv.EnvVars to psi.Environment (not just DOTNET_SKIP_FIRST_TIME_EXPERIENCE).
| }; | |
| }; | |
| foreach (KeyValuePair<string, string> envVar in s_buildEnv.EnvVars) | |
| psi.Environment[envVar.Key] = envVar.Value; |
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| UseShellExecute = false | ||
| }; |
There was a problem hiding this comment.
dotnet tool restore is invoked via ProcessStartInfo without applying s_buildEnv.EnvVars (DOTNET_ROOT/PATH/etc), which other test dotnet invocations rely on to isolate from the repo/machine environment. Consider reusing the existing DotNetCommand/ToolCommand infrastructure or at least merging s_buildEnv.EnvVars into psi.Environment for consistent behavior across runs.
| }; | |
| }; | |
| foreach (KeyValuePair<string, string> envVar in s_buildEnv.EnvVars) | |
| psi.Environment[envVar.Key] = envVar.Value; |
Remove all TestCategory additions (native, coreclr-native, workload, mono) from Wasm.Build.Tests, delete BuildWasmAppsJobsListCoreCLR.txt, and revert the xunit trait filter changes in sendtohelix-browser.targets and Wasm.Build.Tests.csproj back to the original CoreCLR filter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit 78761fc.
Change all [TestCategory("coreclr-native")] back to
[TestCategory("native")] and revert the 5 test class entries
added to BuildWasmAppsJobsListCoreCLR.txt.
The native/coreclr-native split and expanded job list will be
re-introduced in a follow-up PR with proper Helix payload support.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…to main These WBT files should not be changed in this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove class-level [TestCategory("native")] that should not be in this PR.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| <!-- Locate System.Private.CoreLib in runtime pack if not already bundled --> | ||
| <PropertyGroup> | ||
| <_HasCoreLib Condition="'%(_WasmManagedAssemblies.FileName)%(_WasmManagedAssemblies.Extension)' == 'System.Private.CoreLib.dll'">true</_HasCoreLib> | ||
| <!-- CoreCLR WASM ships CoreLib in the native directory, not the lib directory --> | ||
| <_CoreLibPath Condition="'$(_HasCoreLib)' != 'true'">$(MicrosoftNetCoreAppRuntimePackRidNativeDir)System.Private.CoreLib.dll</_CoreLibPath> | ||
| <_CoreLibPath Condition="'$(_HasCoreLib)' != 'true' and !Exists('$(_CoreLibPath)')">$([System.IO.Path]::Combine($(MicrosoftNetCoreAppRuntimePackRidLibTfmDir), 'System.Private.CoreLib.dll'))</_CoreLibPath> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
_HasCoreLib is computed by comparing '%(_WasmManagedAssemblies.FileName)%(_WasmManagedAssemblies.Extension)' to a single filename, but when multiple assemblies are present this expression becomes a semicolon-separated list and the condition will never be true. This makes the "if not already bundled" logic ineffective and can lead to duplicate CoreLib entries in the scan list. Consider computing this via an ItemGroup filter + Count() check (or similar) instead.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/mono/wasm/Wasm.Build.Tests/Templates/WasmTemplateTestsBase.cs:373
EnsureXHarnessAvailable()can run an expensivedotnet tool restore, butRunHost.DotnetRundoesn’t invoke xharness (it runsdotnet run). Calling it unconditionally adds avoidable overhead and can fail in environments where tool restore isn’t possible even though xharness isn’t needed. Consider invokingEnsureXHarnessAvailable()only for hosts that actually use xharness (e.g., theWebServerbranch).
{
if (EnvironmentVariables.UseJavascriptBundler)
{
runOptions = runOptions with { CustomBundleDir = Path.GetFullPath(Path.Combine(GetBinFrameworkDir(runOptions.Configuration, forPublish: true), "..", "public")) };
}
EnsureXHarnessAvailable();
return runOptions.Host switch
{
RunHost.DotnetRun =>
await BrowserRunTest($"run -c {runOptions.Configuration} --no-build", _projectDir, runOptions),
RunHost.WebServer =>
await BrowserRunTest($"{s_xharnessRunnerCommand} wasm webserver --app=. --web-server-use-default-files",
string.IsNullOrEmpty(runOptions.CustomBundleDir) ?
Path.GetFullPath(Path.Combine(GetBinFrameworkDir(runOptions.Configuration, forPublish: true), "..")) :
runOptions.CustomBundleDir,
runOptions),
| <!-- Resolve managed assembly TFM directory --> | ||
| <ItemGroup Condition="'$(MicrosoftNetCoreAppRuntimePackRidLibTfmDir)' == ''"> | ||
| <_SystemRuntimePathItem Include="$(MicrosoftNetCoreAppRuntimePackRidDir)lib$(WasmDirSep)net*$(WasmDirSep)System.Runtime.dll" /> | ||
| </ItemGroup> | ||
|
|
||
| <PropertyGroup Condition="'$(MicrosoftNetCoreAppRuntimePackRidLibTfmDir)' == '' and @(_SystemRuntimePathItem->Count()) > 0"> | ||
| <MicrosoftNetCoreAppRuntimePackRidLibTfmDir>$([System.IO.Path]::GetDirectoryName(%(_SystemRuntimePathItem.Identity)))</MicrosoftNetCoreAppRuntimePackRidLibTfmDir> |
There was a problem hiding this comment.
_CoreCLRWasmInitialize uses $(WasmDirSep) when globbing for System.Runtime.dll, but WasmDirSep doesn’t appear to be defined anywhere else in the repo. If it expands to an empty string, the fallback path becomes invalid and MicrosoftNetCoreAppRuntimePackRidLibTfmDir may never be resolved. Use a standard separator (e.g., \ like other wasm targets in this repo, or $(DirectorySeparatorChar) / NormalizePath) instead of $(WasmDirSep).
| <Error Condition="'$(MicrosoftNetCoreAppRuntimePackDir)' == '' and ('%(ResolvedRuntimePack.PackageDirectory)' == '' or !Exists(%(ResolvedRuntimePack.PackageDirectory)))" | ||
| Text="%24(MicrosoftNetCoreAppRuntimePackDir) is not set and cannot find %25(ResolvedRuntimePack.PackageDirectory). One of these must be a valid path." /> |
There was a problem hiding this comment.
The Exists(%(ResolvedRuntimePack.PackageDirectory)) call is missing quotes around the path. If the runtime pack directory contains spaces, MSBuild condition parsing can fail or evaluate incorrectly. Wrap the metadata in quotes (e.g., Exists('%(ResolvedRuntimePack.PackageDirectory)')) in both the Exists and the error message to make the check robust.
| <Error Condition="'$(MicrosoftNetCoreAppRuntimePackDir)' == '' and ('%(ResolvedRuntimePack.PackageDirectory)' == '' or !Exists(%(ResolvedRuntimePack.PackageDirectory)))" | |
| Text="%24(MicrosoftNetCoreAppRuntimePackDir) is not set and cannot find %25(ResolvedRuntimePack.PackageDirectory). One of these must be a valid path." /> | |
| <Error Condition="'$(MicrosoftNetCoreAppRuntimePackDir)' == '' and ('%(ResolvedRuntimePack.PackageDirectory)' == '' or !Exists('%(ResolvedRuntimePack.PackageDirectory)'))" | |
| Text="%24(MicrosoftNetCoreAppRuntimePackDir) is not set and cannot find '%25(ResolvedRuntimePack.PackageDirectory)'. One of these must be a valid path." /> |
| <ItemGroup> | ||
| <_WasmSourceFileToCompile Remove="@(_WasmSourceFileToCompile)" /> | ||
| <_WasmSourceFileToCompile Include="@(NativeFileReference)" Condition="'%(Extension)' == '.c' or '%(Extension)' == '.cpp'" /> | ||
| <_WasmSourceFileToCompile ObjectFile="$(_WasmIntermediateOutputPath)%(FileName).o" /> |
There was a problem hiding this comment.
_WasmSourceFileToCompile sets ObjectFile to $(_WasmIntermediateOutputPath)%(FileName).o. This will collide (and overwrite) when two NativeFileReference sources share the same filename in different directories, producing incorrect links. Include additional disambiguation in the object file path (e.g., %(RecursiveDir) or a hashed/normalized identity) to guarantee uniqueness.
| <_WasmSourceFileToCompile ObjectFile="$(_WasmIntermediateOutputPath)%(FileName).o" /> | |
| <_WasmSourceFileToCompile ObjectFile="$(_WasmIntermediateOutputPath)%(RecursiveDir)%(FileName).o" /> |
🤖 Copilot Code Review — PR #125607Note This review was generated by GitHub Copilot. Holistic AssessmentMotivation: Justified. This PR cleans up the test categorization from the CoreCLR WASM native relink work. The Approach: Correct. The mechanical rename from Summary: ✅ LGTM. The test categorization changes are internally consistent — every test class removed from Detailed Findings✅ Test Category Consolidation —
|
| Removed Class | Methods | All Excluded? |
|---|---|---|
IcuShardingTests |
2 methods, both native |
✅ Yes |
IcuShardingTests2 |
1 method, native |
✅ Yes |
IcuTests |
2 native + 1 workload |
✅ Yes (all filtered by CoreCLR traits) |
NativeBuildTests |
Class-level native |
✅ Yes |
Wasm.Build.Templates.Tests.NativeBuildTests |
Class-level native |
✅ Yes |
Classes NOT in the CoreCLR list (InvariantGlobalizationTests, InvariantTimezoneTests, MemoryTests) were never in it, so the category rename has no CoreCLR impact.
Blazor tests (BuildPublishTests, MiscTests, SimpleRunTests) also aren't in the CoreCLR job list, so the method-level native tags only affect Mono runs (where native is not filtered).
✅ Dead Code Removal — EnsureWasmTemplatesInstalled
The removed EnsureWasmTemplatesInstalled method, along with s_wasmTemplatesInstalled and s_wasmTemplatesLock, has zero remaining references. The using System.Diagnostics import was only needed by this method. Clean removal.
✅ Class-Level Category Hoisting — DllImportTests, NativeBuildTests, PInvokeTableGeneratorTests
For these three classes, ALL test methods previously had individual [TestCategory("native")] or [TestCategory("coreclr-native")] attributes. Hoisting to class-level is a correct simplification — xUnit applies class-level trait attributes to all methods.
✅ Research Files Cleanup
The .research/ files only exist on this branch (no history on main). Proper cleanup of development artifacts before merge.
Generated by Code Review for issue #125607 · ◷
Contributes to #123670
Contributes to #126100