[msbuild] Resolve Content and BundleResource items using the main project's location. Fixes 23898.#25713
[msbuild] Resolve Content and BundleResource items using the main project's location. Fixes 23898.#25713rolfbjarne wants to merge 9 commits into
Conversation
Add a test that reproduces the issue where Content/BundleResource items defined by an SDK (like the Razor SDK) have their LogicalName computed relative to the SDK directory instead of the project directory, resulting in paths like '../../../../../src/MyProject/file.png' that get rejected. This test currently fails, demonstrating the bug described in #23898. Co-authored-by: Copilot <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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…s location. Fixes #23898. Always compute the LogicalName for Content and BundleResource items relative to the project file (LocalMSBuildProjectFullPath) instead of the defining project file (LocalDefiningProjectFullPath). This fixes the issue where SDKs (like the Razor SDK) that add Content items from a directory far from the project would produce nonsensical paths like '../../../../../src/MyProject/file.png' that get rejected. Co-authored-by: Copilot <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.
…10/11, opt-out for .NET 12+ Add a new MSBuild property ResolveResourceItemsRelativeToProject that controls whether Content and BundleResource items have their LogicalName computed relative to the project file (new behavior) or the defining project file (legacy behavior). - .NET 10 and .NET 11: defaults to false (opt-in) - .NET 12+: defaults to true (opt-out) The property is passed through a new IHasResolveResourceItemsRelativeToProject interface so that GetVirtualProjectPath can check the flag without changing its generic constraints. Co-authored-by: Copilot <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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ode paths Test with ResolveResourceItemsRelativeToProject=true (.NET 12+ default) and ResolveResourceItemsRelativeToProject=false (.NET 10/11 default) to ensure both the fixed and legacy behaviors are covered. Co-authored-by: Copilot <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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/review |
|
✅ .NET for Apple Platforms PR Reviewer completed successfully! |
There was a problem hiding this comment.
Pull request overview
This PR introduces an MSBuild switch (ResolveResourceItemsRelativeToProject) to compute Content/BundleResource LogicalName values relative to the main project file location (LocalMSBuildProjectFullPath) instead of the defining project (LocalDefiningProjectFullPath), addressing cases where SDKs add items from directories far from the project.
Changes:
- Add and plumb
ResolveResourceItemsRelativeToProjectfrom MSBuild props/targets intoCollectBundleResources. - Update
BundleResource.GetVirtualProjectPathto optionally resolve relative paths against the main project file directory. - Add a regression test and document the new build property.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/CollectBundleResourcesTaskTests.cs | Adds a test covering SDK-defined content items far from the project directory. |
| msbuild/Xamarin.Shared/Xamarin.Shared.targets | Passes ResolveResourceItemsRelativeToProject into the CollectBundleResources task invocation. |
| msbuild/Xamarin.Shared/Xamarin.Shared.props | Introduces defaulting logic for ResolveResourceItemsRelativeToProject based on target framework version. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/Interfaces.cs | Adds a new task interface to expose ResolveResourceItemsRelativeToProject. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/CollectBundleResources.cs | Implements the new interface/property so BundleResource logic can pick it up. |
| msbuild/Xamarin.MacDev.Tasks/BundleResource.cs | Implements conditional “resolve relative to main project” behavior when computing virtual paths. |
| docs/building-apps/build-properties.md | Documents the new build property and its defaults. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
✅ LGTM — This is a solid fix for a real MSBuild issue. The PR addresses the problem where SDKs (like Razor) add items from directories far from the project, causing incorrect path computation.
Strengths
✅ Well-tested — Comprehensive test coverage for both the new behavior (.NET 12+) and legacy behavior (.NET 10/11)
✅ Backward compatible — Proper opt-in/opt-out strategy across .NET versions
✅ Good documentation — The build-properties.md file clearly explains the new property
✅ Clean implementation — The code changes are focused and follow existing patterns
Observations
- The implementation correctly moves the
localDefiningProjectFullPathnull check to only run when needed (line 129 inBundleResource.cs) - The interface-based approach (
IHasResolveResourceItemsRelativeToProject) is consistent with other MSBuild task properties in this codebase - Test assertions validate both the success case (3 resources with correct logical names) and the failure case (3 warnings, 0 resources)
Minor Suggestions
See inline comments for:
- 💡 Potential enforcement of the "gone in .NET 13" claim
- 💡 Test pattern consistency verification
Overall, this is good work addressing a real pain point for Blazor/Razor developers on Apple platforms. The gradual rollout strategy (opt-in → default → mandatory) is well-designed.
Generated by .NET for Apple Platforms PR Reviewer for issue #25713 · 38.7 AIC · ⌖ 9.02 AIC · ⊞ 5.1K
Comment /review to run again
| <!-- Resolve Content and BundleResource items relative to the project file instead of the defining project file. | ||
| This fixes issues where SDKs (like the Razor SDK) add items from a directory far from the project, | ||
| producing nonsensical paths. Ref: https://github.com/dotnet/macios/issues/23898 | ||
| Opt-in for .NET 10 and .NET 11, opt-out for .NET 12, gone in .NET 13. --> |
There was a problem hiding this comment.
🤖 💡 Documentation — The comment mentions "gone in .NET 13", but there's no enforcement mechanism in the code.
Consider adding a condition that causes a build error if the property is set in .NET 13+, to ensure this breaking change happens as documented. For example:
<PropertyGroup Condition="$([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 13.0))">
<MSBuildError Condition="'$(ResolveResourceItemsRelativeToProject)' == 'false'">
ResolveResourceItemsRelativeToProject is no longer supported in .NET 13. Remove this property.
</MSBuildError>
</PropertyGroup>Rule: Track TODOs as issues (or enforce them)
| items.Add (item); | ||
| } | ||
|
|
||
| var task = CreateTask<CollectBundleResources> (); |
There was a problem hiding this comment.
🤖 💡 Testing — The test uses CreateTask<CollectBundleResources>() which likely inherits from a test base. Does this properly initialize task.ProjectDir?
Looking at line 79, you explicitly set task.ProjectDir = projDir + Path.DirectorySeparatorChar;, but in the existing test LogicalNameOutsideAppBundle(), there's no explicit task.ProjectDir initialization. This suggests the base CreateTask method might not initialize it correctly.
Verify that the ProjectDir property is always initialized, or document that tests must set it explicitly. The current code works, but it's worth confirming the pattern is consistent.
Rule: Test edge cases
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #1f73a0d] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #1f73a0d] Build passed (Build packages) ✅Pipeline on Agent |
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 |
✅ [PR Build #1f73a0d] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🔥 [CI Build #1f73a0d] Test results 🔥Test results❌ Tests failed on VSTS: test results 1 tests crashed, 1 tests failed, 193 tests passed. Failures❌ windows tests1 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download ❌ Tests on macOS Sonoma (14) tests🔥 Failed catastrophically on VSTS: test results - mac_sonoma (no summary found). Html Report (VSDrops) Download Successes✅ 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 |
Always compute the LogicalName for Content and BundleResource items relative to the project file (
LocalMSBuildProjectFullPath) instead of the defining project file (LocalDefiningProjectFullPath). This fixes the issue where SDKs (like the Razor SDK) that add Content items from a directory far from the project would produce nonsensical paths like'../../../../../src/MyProject/file.png'that get rejected.Fixes #23898
🤖 Pull request created by Copilot