[msbuild] Skip unchanged assembly preparation - #26610
rolfbjarne wants to merge 3 commits into
Conversation
Assert that both assembly preparation passes are skipped when an incremental build has no input changes. This captures the intended behavior already documented by the test and reproduces the unnecessary work seen in unchanged-build binlogs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9117a629-9654-4c0d-9a2f-355a8eca6186
Use the assembly preparer's persistent MSBuild output files as the incremental completion markers. Preparation intentionally passes unmodified assemblies through without creating transformed output files, while post-processing copies preserve source timestamps; declaring those assembly paths as target outputs therefore forced both tasks to run on every unchanged build. Keep the complete assembly and configuration input sets so source or option changes still rerun preparation, and apply the same marker to the post-ILC variant for consistent incremental behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9117a629-9654-4c0d-9a2f-355a8eca6186
There was a problem hiding this comment.
🟡 Changes recommended
Critical incremental-state issues remain in the preparation and post-ILC targets.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates MSBuild assembly preparation and post-processing incrementality using persistent output markers, with regression checks for unchanged builds.
Changes:
- Uses persistent preparer outputs as incremental markers.
- Applies marker-based tracking to post-ILC processing.
- Adds unchanged-build skip assertions.
File summaries
| File | Review summary |
|---|---|
tests/dotnet/UnitTests/PrepareAssembliesTest.cs |
Adds incremental assertions. Nit (2 votes): lacks post-ILC coverage. Nit (1 vote): does not verify reruns after source or configuration changes. |
msbuild/Xamarin.Shared/Xamarin.Shared.targets |
Critical (2 votes, lines 3773/3822): skipped targets do not restore transformed item changes, potentially causing downstream targets to consume original assemblies. |
dotnet/targets/Xamarin.Shared.Sdk.targets |
Critical (1 vote, line 1522): skipped post-ILC processing does not restore postprocessed assemblies or publish-item updates. |
Review details
Suppressed comments (2)
msbuild/Xamarin.Shared/Xamarin.Shared.targets:3822
- ❌ bug — The post-processing body likewise removes
@(_AssembliesToPostProcess)fromResolvedFileToPublishand adds@(_PostProcessedAssemblies). If this target is skipped in a fresh build because only the marker is up to date, that item replacement is not replayed andNativeCompile/CreateReadyToRunImagescan receive pre-postprocessed assemblies. Persist/reload the transformed-item mapping or keep the transformed outputs in the incremental contract.
Outputs="$(_AssemblyPostProcessorMSBuildOutputFile)"
tests/dotnet/UnitTests/PrepareAssembliesTest.cs:40
⚠️ Test coverage — This regression only verifies the no-change skip case. The PR also promises that source and configuration changes still rerun preparation/post-processing, but neither the changed assembly inputs nor the configuration stamp/linker-cache path is exercised here; add follow-up builds that change each class of input and assert both targets execute.
var result = DotNet.AssertBuild (project_path, properties);
var targets = BinLog.GetAllTargets (result.BinLogPath);
AssertTargetNotExecuted (targets, "_PrepareAssemblies", "Incremental preparation");
AssertTargetNotExecuted (targets, "_PostprocessAssemblies", "Incremental post-processing");
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
Persist the prepared and postprocessed assembly item mappings, including metadata, and reload them from always-running wrapper targets. This preserves ResolvedFileToPublish and FileWrites state when the expensive execution targets are skipped on unchanged builds. Add NativeAOT post-ILC incremental coverage and verify that source and configuration changes still execute assembly preparation. Update existing target assertions to distinguish the execution targets from the state-restoring wrappers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9117a629-9654-4c0d-9a2f-355a8eca6186
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 |
There was a problem hiding this comment.
🟡 Changes recommended
Address the moderate test-coverage findings and the nullable-analysis nit in PrepareAssembliesTest.cs.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
tests/dotnet/UnitTests/PrepareAssembliesTest.cs:77
⚠️ Test coverage — As above,AssertTargetNotExecutedtreats a missing target as success, so this NativeAOT regression check would also pass if_ExecutePostprocessAssembliesAfterIlcstopped being wired into the build. Require the target entries to exist before checking that they were skipped.
AssertTargetNotExecuted (targets, "_ExecutePrepareAssemblies", "Incremental NativeAOT preparation");
AssertTargetNotExecuted (targets, "_ExecutePostprocessAssembliesAfterIlc", "Incremental NativeAOT post-processing");
tests/dotnet/UnitTests/PrepareAssembliesTest.cs:42
⚠️ Best practices — Avoid the newly added null-forgiving operator here. Use an explicit null check (or throw) forPath.GetDirectoryName (project_path)so this test remains warning-clean without suppressing nullable analysis.
var sourcePath = Path.Combine (Path.GetDirectoryName (project_path)!, "..", "AppDelegate.cs");
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #caeb139] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 264 tests passed 🎉 Tests counts✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Sonoma (14): All 5 tests passed. [attempt 2] Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Use the assembly preparer's persistent MSBuild output files as incremental completion markers instead of transformed assembly paths that may not exist or may retain old source timestamps.
This prevents
_PrepareAssembliesand_PostprocessAssembliesfrom running when no project inputs have changed, while preserving reruns for source and configuration changes. Apply the same behavior to the post-ILC variant.Add regression assertions that both targets are skipped on unchanged incremental builds.
🤖 Pull request created by Copilot