[msbuild] Fix incremental build for Metal shaders. Fixes #24816.#24829
[msbuild] Fix incremental build for Metal shaders. Fixes #24816.#24829rolfbjarne merged 1 commit intomainfrom
Conversation
The _SmeltMetal target lacked Inputs/Outputs attributes, causing Metal shaders to always be recompiled even when unchanged. This follows the same cache/stamp file pattern used by _CoreCompileInterfaceDefinitions: - _BeforeSmeltMetal: deletes cache when any Metal file is newer - _ReadSmeltedMetal: reads cached _SmeltedMetal items for incremental builds - _SmeltMetal: now has Inputs/Outputs and writes cache after compilation Also adds a unit test (MetalShadersNotRecompiled) that verifies _SmeltMetal and _TemperMetal are skipped on the second build when no Metal files have changed. Fixes #24816 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rolfbjarne
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes incremental build behavior for Metal shader compilation in the Apple-platform MSBuild targets, so unchanged .metal inputs don’t force recompilation on every build. Adds a unit test to validate _SmeltMetal/_TemperMetal are skipped on a no-change rebuild.
Changes:
- Add Metal item caching/stamp workflow around
_SmeltMetal(pre-delete cache on input changes, read cached items on incremental builds, write cache after compilation). - Add
Inputs/Outputsto_SmeltMetalso MSBuild can skip the target when inputs are unchanged. - Add
MetalShadersNotRecompiledunit test to verify_SmeltMetaland_TemperMetalare skipped on the second build.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
msbuild/Xamarin.Shared/Xamarin.Shared.targets |
Introduces _SmeltedMetal cache file and incremental build targets (_BeforeSmeltMetal, _ReadSmeltedMetal) + Inputs/Outputs for _SmeltMetal. |
tests/dotnet/UnitTests/IncrementalBuildTest.cs |
Adds a regression test ensuring Metal compilation/linking targets are skipped on a second build with no .metal changes. |
You can also share your feedback on Copilot code review. Take the survey.
| <Target Name="_BeforeSmeltMetal" Condition="'$(_CanOutputAppBundle)' == 'true' And '@(Metal)' != ''" | ||
| Inputs="@(Metal)" | ||
| Outputs="$(_SmeltedMetalCache)"> | ||
| <!-- If any Metal file is newer than the cached items list, delete the cache so that the _SmeltMetal target runs again --> | ||
| <Delete Files="$(_SmeltedMetalCache)" /> | ||
| </Target> | ||
|
|
||
| <Target Name="_ReadSmeltedMetal" Condition="'$(_CanOutputAppBundle)' == 'true' And '@(Metal)' != ''" DependsOnTargets="_BeforeSmeltMetal"> | ||
| <!-- If _BeforeSmeltMetal did not delete the cache, read the cached _SmeltedMetal items --> | ||
| <ReadItemsFromFile File="$(_SmeltedMetalCache)" Condition="Exists('$(_SmeltedMetalCache)')"> | ||
| <Output TaskParameter="Items" ItemName="_SmeltedMetal" /> | ||
| </ReadItemsFromFile> | ||
| </Target> | ||
|
|
||
| <Target Name="_SmeltMetal" Condition="'$(_CanOutputAppBundle)' == 'true' And '@(Metal)' != ''" DependsOnTargets="$(_SmeltMetalDependsOn)" | ||
| Inputs="@(Metal)" | ||
| Outputs="$(_SmeltedMetalCache)"> |
There was a problem hiding this comment.
With the new incremental Inputs/Outputs, _SmeltMetal can now be skipped even if the compiled app manifest changed (which can change $(_MinimumOSVersion) and therefore the compiler arguments). This risks producing stale .air outputs when Info.plist / supported OS settings change but the .metal sources don’t. Consider including $(_TemporaryAppManifest) (or another manifest stamp file) in the Inputs for _BeforeSmeltMetal/_SmeltMetal, similar to how _BeforeCompileCoreMLModels includes $(_TemporaryAppManifest) in its Inputs.
✅ [CI Build #646daee] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #646daee] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ 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 #646daee] Build passed (Build macOS tests) ✅Pipeline on Agent |
🔥 [CI Build #646daee] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 12 tests failed, 144 tests passed. Failures❌ monotouch tests (iOS)11 tests failed, 0 tests passed.Failed tests
|
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #646daee] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 156 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
The _SmeltMetal target lacked Inputs/Outputs attributes, causing Metal
shaders to always be recompiled even when unchanged. This follows the
same cache/stamp file pattern used by _CoreCompileInterfaceDefinitions:
Also adds a unit test (MetalShadersNotRecompiled) that verifies
_SmeltMetal and _TemperMetal are skipped on the second build when
no Metal files have changed.
Fixes #24816