[msbuild] Merge runtimeconfig.dev.json into the runtime configuration. Fixes #26330 - #26332
[msbuild] Merge runtimeconfig.dev.json into the runtime configuration. Fixes #26330#26332rolfbjarne wants to merge 3 commits into
Conversation
We bake the runtime configuration directly into the app and hand it to the runtime, bypassing hostfxr. hostfxr is what would normally merge the companion '*.runtimeconfig.dev.json' file (where the .NET SDK writes Debug-only switches such as Hot Reload's System.StartupHookProvider.IsSupported and System.Reflection.Metadata.MetadataUpdater.IsSupported) into the main runtime configuration, so those switches never reached the runtime. Add a new MergeRuntimeConfigFiles MSBuild task that overlays the dev file's 'runtimeOptions.configProperties' on top of the main file's (dev wins, matching hostfxr), and feed the merged file to the existing RuntimeConfigParserTask. Fixes #26330 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a gap in the Apple workloads’ runtime initialization path: because we bypass hostfxr and bake runtime configuration directly into the app, *.runtimeconfig.dev.json wasn’t being merged, so Debug-only runtimeOptions.configProperties switches (notably Hot Reload-related switches) never reached the runtime.
Changes:
- Add a new MSBuild task (
MergeRuntimeConfigFiles) to overlayruntimeOptions.configPropertiesfrom*.runtimeconfig.dev.jsononto the main*.runtimeconfig.json(dev wins), producing a merged JSON input for the existingRuntimeConfigParserTask. - Update
_CreateRuntimeConfigurationto run the merge step, include the dev file inInputs, and track the merged file inFileWrites. - Add unit tests covering merge behavior, missing dev file, and dev file without
configProperties.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/MergeRuntimeConfigFilesTaskTest.cs | Adds unit coverage for merge semantics and no-dev/configProperties-absent cases. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/MergeRuntimeConfigFiles.cs | Implements the merge logic for runtimeOptions.configProperties into a merged runtimeconfig JSON. |
| msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx | Adds a localized error string for invalid runtimeconfig JSON root. |
| dotnet/targets/Xamarin.Shared.Sdk.targets | Wires the merge task into _CreateRuntimeConfiguration and switches RuntimeConfigParserTask to use the merged file. |
Comments suppressed due to low confidence (1)
msbuild/Xamarin.MacDev.Tasks/Tasks/MergeRuntimeConfigFiles.cs:65
- The null-forgiving operator is used on
outputDirectory!, butoutputDirectoryis already proven non-null by theIsNullOrEmptycheck. The postfix!operator is banned in this repo; use a simple block instead.
var outputDirectory = Path.GetDirectoryName (OutputFile);
if (!string.IsNullOrEmpty (outputDirectory))
Directory.CreateDirectory (outputDirectory!);
…, DeepClone. Address Copilot review feedback on MergeRuntimeConfigFiles: - Remove the banned null-forgiving (!) operator; use explicit null/length checks (netstandard2.0's string.IsNullOrEmpty isn't null-annotated). - Catch read/parse failures and report them as an MSBuild error (E7186) instead of letting the task crash. - Clone dev values with JsonNode.DeepClone () instead of serialize+parse. - Add a regression test for the invalid-JSON error path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jonathanpeppers
left a comment
jonathanpeppers
left a comment
There was a problem hiding this comment.
There is a minimal perf thing I saw, but up to you if you want to fix. Those files are small, so may be fine. 👍
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Per review feedback, avoid materializing the whole runtimeconfig file as a string: parse directly from a FileStream (JsonNode.Parse (Stream)) and write the merged result through a Utf8JsonWriter over a FileStream instead of File.ReadAllText / File.WriteAllText. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
🚀 [CI Build #eb03d7b] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 203 tests passed 🎉 Tests counts✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
We bake the runtime configuration directly into the app and hand it to the runtime, bypassing
hostfxr.hostfxris what would normally merge the companion*.runtimeconfig.dev.jsonfile into the main runtime configuration, so the Debug-only switches the .NET SDK writes there (such as Hot Reload'sSystem.StartupHookProvider.IsSupportedandSystem.Reflection.Metadata.MetadataUpdater.IsSupported) never reached the runtime.Fix
Rather than teach the shared
RuntimeConfigParserTask(from dotnet/runtime) about the dev file, this does the merge locally and keeps using that task unchanged:MergeRuntimeConfigFilesMSBuild task overlays the dev file'sruntimeOptions.configPropertieson top of the main file's (dev wins, matchinghostfxr) and writes a merged json. If there is no dev file, or it has noconfigProperties, the main file is written out unchanged so the target's incremental Inputs/Outputs logic stays consistent._CreateRuntimeConfigurationnow runs the merge intoruntimeconfig.merged.jsonin the device-specific intermediate dir, feeds that toRuntimeConfigParserTask, adds the dev file to the targetInputs, and adds the merged file toFileWrites.configProperties.This mirrors the equivalent fix in dotnet/android#12249.
Fixes #26330
🤖 Pull request created by Copilot