Update BootJsonData generation to account for runtimeconfig.dev.json#130825
Conversation
|
Azure Pipelines: 15 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
@copilot Add tests validating the change |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "93bef74398e8ac235605749ac7de341fc9bc6ea8",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f94898a9b55df07348434e86915c7405962427b6",
"last_reviewed_commit": "93bef74398e8ac235605749ac7de341fc9bc6ea8",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f94898a9b55df07348434e86915c7405962427b6",
"last_recorded_worker_run_id": "29755365875",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "c0d0b71b0a2f2a5cefb58e33f3c7815b5056a353",
"review_id": 4730812736
},
{
"commit": "93bef74398e8ac235605749ac7de341fc9bc6ea8",
"review_id": 4736525867
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: GenerateWasmBootJson emitted only the main runtimeconfig.json into the boot config and ignored the companion runtimeconfig.dev.json, which the SDK uses to inject Hot Reload switches in debug builds (#130823). As a result WASM/Blazor debug builds did not surface those config properties at runtime.
Approach: A new optional RuntimeConfigDevJsonPath task input is threaded from the build targets (only for non-publish builds via IsPublish ? null : RuntimeConfigDevJsonPath, and added to the target Inputs for incremental correctness). The old inline read is refactored into an internal static ReadRuntimeConfigFiles helper that reads the main config and, when a dev config exists, merges its configProperties with dev values taking precedence, null-safely initializing intermediate objects. InternalsVisibleTo exposes the helper to a new xUnit test project covering null/missing paths, additive merge, override, empty-props no-op, main-with-no-configProperties, and JSON boolean/number type preservation.
Summary: The core logic change is small, focused, and correct. File.Exists(null) returns false, so the null-path guards are sound; restricting the dev merge to non-publish builds matches the intent (dev config is a debug/build-time artifact). The empty-devProps guard correctly avoids materializing an empty configProperties on an otherwise-sparse config. Test coverage is thorough and the JSON-type-preservation test is a nice touch. My one substantive concern is that the new test project is placed under src/tests/ (the CoreCLR runtime-test tree) and is not wired into any subset or solution, so it likely won't build or run in CI — see the inline comment. Verdict: approve pending resolution of the test-project placement/wiring so the added coverage actually executes.
Detailed Findings
-
Test project not wired into the build (blocking for the tests to be effective): see the inline comment on
src/tests/tasks/.../Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Tests.csproj. The project sits undersrc/tests/but is a plain SDK xUnit project not referenced byeng/Subsets.props,tasks.proj, or any.slnx; theILLink.Tasks.Testspattern undersrc/tools/illink/test/(explicit subset + slnx entry) is the model to follow. -
Nit (non-blocking):
ReadRuntimeConfigFilesdeserializes the dev config into a fullRuntimeConfigDatabut only consumesruntimeOptions.configProperties. That is fine and clear; just note other dev-config fields are intentionally ignored, which matches the stated scope.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 94.7 AIC · ⌖ 10.8 AIC · ⊞ 10K
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/GenerateWasmBootJson.cs:578
- ReadRuntimeConfigFiles currently relies entirely on the caller to provide the dev config path. If RuntimeConfigDevJsonPath/$(ProjectRuntimeConfigDevFilePath) is empty or not set, the companion runtimeconfig.dev.json next to the main config will be ignored even if present. Adding a fallback derivation keeps the task resilient and matches the intended behavior described in this PR (App.runtimeconfig.json -> App.runtimeconfig.dev.json).
internal static RuntimeConfigData? ReadRuntimeConfigFiles(string? mainConfigPath, string? devConfigPath)
{
if (!File.Exists(mainConfigPath))
return null;
There was a problem hiding this comment.
Holistic Review
Motivation: GenerateWasmBootJson emitted only the main runtimeconfig.json into the boot config and ignored the companion runtimeconfig.dev.json, which the SDK uses to inject Hot Reload switches in debug builds (#130823). WASM/Blazor debug builds therefore did not surface those config properties at runtime.
Approach: The read is refactored into an internal static ReadRuntimeConfigFiles helper that reads the main config and, for non-publish builds, merges configProperties from the dev config (dev values win), null-safely initializing intermediate objects. InternalsVisibleTo exposes the helper to a new xUnit test project. This incremental revision addresses the prior review's one blocking concern: the test project was moved from src/tests/tasks/ (the CoreCLR runtime-test tree, where it was unwired) to src/tasks/tests/, and is now wired into libs.tests via eng/Subsets.props (gated to CoreCLR desktop OSes) while being excluded from the tasks build via a Remove="**\tests\**\*.csproj" in tasks.proj. The test csproj now mirrors the ILLink.Tasks.Tests model.
Summary: The build-wiring changes are correct and follow the established ILLink.Tasks.Tests pattern (arcade auto-provides the xunit harness for *Tests projects, so no explicit xunit PackageReference is required; RepoTasksDir is defined and the paths resolve). The libs.tests gating to CoreCLR on windows/linux/osx is appropriate for plain MSBuild-task tests. The tasks.proj exclusion correctly prevents the test project from being pulled into the tasks build. The core logic in GenerateWasmBootJson.cs is unchanged since the prior review and remains correct. The prior blocking concern is fully resolved. Verdict: LGTM.
Assessment History
- review 4730812736 reviewed commit
c0d0b71b0a2f2a5cefb58e33f3c7815b5056a353with an approve-pending verdict (blocked on the test project being unwired undersrc/tests/). Current verdict: LGTM. Changed: the incremental "Move tests" commit relocated the project tosrc/tasks/tests/and wired it intolibs.tests(eng/Subsets.props) plus excluded it fromtasks.proj, resolving the sole blocking finding; motivation and core-logic approach are otherwise unchanged.
Detailed Findings
No new actionable findings in the incremental scope. The build-wiring changes (eng/Subsets.props, src/tasks/tasks.proj, the moved/updated test csproj and targets) are sound and follow repo conventions.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 102.9 AIC · ⌖ 9.93 AIC · ⊞ 10K
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/GenerateWasmBootJson.cs:583
ReadRuntimeConfigFilesonly honorsruntimeconfig.dev.jsonwhen the caller passes an explicitdevConfigPath. The PR description and the method XML comment describe a "companion" dev config derived from the main runtimeconfig name, but the current implementation doesn't derive a default (and the MSBuild property$(ProjectRuntimeConfigDevFilePath)is not defined anywhere in this repo). Consider adding a fallback that derives*.runtimeconfig.dev.jsonfrommainConfigPathwhendevConfigPathis null/empty so the behavior is robust across call sites and matches the documented intent.
internal static RuntimeConfigData? ReadRuntimeConfigFiles(string? mainConfigPath, string? devConfigPath)
{
if (!File.Exists(mainConfigPath))
return null;
using var fs = File.OpenRead(mainConfigPath);
var runtimeConfig = JsonSerializer.Deserialize<RuntimeConfigData>(fs, BootJsonBuilderHelper.JsonOptions);
if (File.Exists(devConfigPath))
{
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/GenerateWasmBootJson.cs:65
- RuntimeConfigDevJsonPath is introduced as a new public MSBuild task parameter. Since it can be derived from RuntimeConfigJsonPath (App.runtimeconfig.json -> App.runtimeconfig.dev.json), consider computing it internally (and/or inside the targets) to avoid expanding the task's public surface area and reducing compatibility risk for external consumers.
public string? RuntimeConfigJsonPath { get; set; }
public string? RuntimeConfigDevJsonPath { get; set; }
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/GenerateWasmBootJson.cs:468
- WriteBootConfig currently relies on RuntimeConfigDevJsonPath being set by the targets. If that MSBuild property is unset/empty (or older SDKs), runtimeconfig.dev.json will be ignored. Consider falling back to Path.ChangeExtension(RuntimeConfigJsonPath, ".dev.json") when not publishing and no explicit dev path was provided.
result.runtimeConfig = ReadRuntimeConfigFiles(RuntimeConfigJsonPath, IsPublish ? null : RuntimeConfigDevJsonPath);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/GenerateWasmBootJson.cs:468
- The PR description says the task derives the companion
*.runtimeconfig.dev.jsonpath fromRuntimeConfigJsonPathviaPath.ChangeExtension(...), but the current code only reads the dev config whenRuntimeConfigDevJsonPathis explicitly provided. In MSBuild, an unset property typically flows as an empty string (not null), so without a fallback derivation the dev config can still be ignored.
result.runtimeConfig = ReadRuntimeConfigFiles(RuntimeConfigJsonPath, IsPublish ? null : RuntimeConfigDevJsonPath);
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b49d3786-9f03-4fd3-8076-28a73d639686
|
I have reverted the subsets.props change. We don't have a nice way to run MSBuild tasks tests in runtime now. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/tasks.tests/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Tests/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Tests.csproj:6
- This new test project doesn't opt into the repo's test infrastructure (xUnit + Microsoft.NET.Test.Sdk). As-is, the project likely won't compile (missing Xunit references) and won't be discoverable/runnable as a test project.
Consider marking it as a test project and importing the standard test props so xUnit dependencies/targets are applied consistently.
<PropertyGroup>
<TargetFramework>$(NetCoreAppToolCurrent)</TargetFramework>
<Nullable>enable</Nullable>
</PropertyGroup>
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
|
/backport to release/11.0-preview7 |
|
Started backporting to |
GenerateWasmBootJsonincludedruntimeConfigfromruntimeconfig.jsonbut ignoredruntimeconfig.dev.json, which the SDK uses to inject Hot Reload switches in debug builds.Changes in
GenerateWasmBootJson.cs:runtimeconfig.json, derive the companion dev config path viaPath.ChangeExtension(RuntimeConfigJsonPath, ".dev.json")(App.runtimeconfig.json→App.runtimeconfig.dev.json)!IsPublish, mergeconfigPropertiesfrom the dev config into the result — dev values override main config valuesresult.runtimeConfigassignment after all merge logic, with null-safe initialization of intermediate objects if the main config was absent or sparse