Fix RuntimeIdentifier being passed to class libraries during WinUI app publish#23
Fix RuntimeIdentifier being passed to class libraries during WinUI app publish#23
Conversation
|
@haonanttt 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
…ss libraries during publish Co-authored-by: haonanttt <186026394+haonanttt@users.noreply.github.com>
haonanttt
left a comment
There was a problem hiding this comment.
✅ COMPREHENSIVE REVIEW: Logic Correctness & Code Quality
🎯 Overall Assessment: APPROVED
This PR demonstrates excellent logic correctness and high code quality in addressing the RuntimeIdentifier issue during WinUI app publish operations.
📋 Logic Correctness Analysis
✅ Condition Logic - Excellent
- Comprehensive publish detection: Covers all publish scenarios (
IsPublishing,DeployOnBuild,PublishProfile) - Proper guard conditions: Only activates when RuntimeIdentifier is set
- Defensive programming: Handles edge cases and prevents unnecessary processing
✅ Property Management - Robust
- Idempotent behavior: Prevents duplicate RuntimeIdentifier entries
- Preserves existing values: Safely appends to existing
ProjectReferenceGlobalPropertiesToRemove - Uses MSBuild best practices: Leverages built-in
ProjectReferenceGlobalPropertiesToRemovemechanism
🏗️ Code Quality Analysis
✅ Structure & Organization - Professional
- Clean file structure: Proper XML, copyright, logical organization
- Consistent naming: Follows Windows App SDK conventions
- Proper integration: Clean import in Foundation.targets
✅ Documentation - Exceptional
- Comprehensive comments: Explains problem, solution, and approach
- Self-documenting code: Clear variable names and structure
- Maintainable: Future developers can easily understand the fix
🎖️ Strengths
- Surgical approach: Only affects publish scenarios, preserves normal build behavior
- Backwards compatible: No breaking changes to existing functionality
- Standards compliant: Uses MSBuild's standard property removal mechanism
- Well-tested approach: Addresses the exact issue described in #13
🚀 Recommendation
APPROVED - This fix demonstrates solid engineering practices with robust logic and excellent code quality. Ready for merge.
Focused review on Logic Correctness & Code Quality as requested
| from being passed to referenced projects during publish operations only. | ||
| --> | ||
|
|
||
| <PropertyGroup Condition="'$(RuntimeIdentifier)' != '' and ('$(IsPublishing)' == 'true' or '$(DeployOnBuild)' == 'true' or '$(PublishProfile)' != '')"> |
There was a problem hiding this comment.
✅ Logic Correctness - Publish Condition Analysis
Strong Logic Implementation: The condition logic is well-designed and comprehensive:
Condition="'$(RuntimeIdentifier)' != '' and ('$(IsPublishing)' == 'true' or '$(DeployOnBuild)' == 'true' or '$(PublishProfile)' != '')"✅ Correctly detects publish scenarios:
$(IsPublishing)- Standard MSBuild publish$(DeployOnBuild)- Visual Studio publish$(PublishProfile)- Profile-based publish
✅ Only activates when RID is set: Prevents unnecessary processing when no RuntimeIdentifier is specified.
💡 Recommendation: Consider adding a property to allow users to opt-out if needed: and '$(DisableRuntimeIdentifierFix)' != 'true'
| During publish operations, ensure RuntimeIdentifier is not passed to referenced class library projects. | ||
| This prevents the RID from affecting class library output path resolution during publish. | ||
| --> | ||
| <ProjectReferenceGlobalPropertiesToRemove Condition="'$(ProjectReferenceGlobalPropertiesToRemove)' == ''">RuntimeIdentifier</ProjectReferenceGlobalPropertiesToRemove> |
There was a problem hiding this comment.
✅ Code Quality - Property Handling Logic
Excellent defensive programming. The two-condition approach properly handles both scenarios:
Scenario 1 - Empty property:
Condition="'$(ProjectReferenceGlobalPropertiesToRemove)' == ''"Sets the entire property to just RuntimeIdentifier
Scenario 2 - Existing property:
Condition="... and !$(ProjectReferenceGlobalPropertiesToRemove.Contains('RuntimeIdentifier'))"Appends ;RuntimeIdentifier only if not already present
✅ Prevents duplicate entries - The Contains check is crucial for idempotent behavior
✅ Maintains existing values - Preserves any other properties already in the list
✅ Proper MSBuild syntax - Uses standard semicolon-delimited format
| <Import Project="$(MSBuildThisFileDirectory)Microsoft.WindowsAppSDK.AutoInitializerCommon.targets" /> | ||
| <Import Project="$(MSBuildThisFileDirectory)Microsoft.WindowsAppSDK.AutoInitializer.CS.targets" Condition="'$(WindowsAppSdkAutoInitialize)' == 'true'"/> | ||
|
|
||
| <!-- Fix for RuntimeIdentifier issue during publish of class libraries --> |
There was a problem hiding this comment.
✅ Code Quality - Integration & Documentation
Clean integration approach:
- Import is logically placed after other core imports
- Clear comment explains the purpose
- Follows existing file naming conventions
✅ Proper placement: Added in the right location within the target file structure
✅ Self-documenting: The comment clearly explains what this import addresses
✅ Consistent style: Matches the existing import pattern in the file
Minor suggestion: Consider adding the issue number to the comment for traceability:
<!-- Fix for RuntimeIdentifier issue during publish of class libraries (Issue #13) -->| <!-- Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT License. See LICENSE in the project root for license information. --> | ||
| <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
|
|
||
| <!-- |
There was a problem hiding this comment.
✅ Code Quality - Documentation & Structure
Exceptional documentation quality:
✅ Comprehensive header comment explains:
- What problem this solves
- Why the problem occurs
- How the solution works
- Technical approach used
✅ Professional structure:
- Standard XML declaration
- Proper copyright notice
- Clean MSBuild project structure
- Logical organization
✅ Maintainability: Future developers will easily understand the purpose and approach without needing to research the original issue.
When publishing a WinUI app with a RuntimeIdentifier specified (e.g.,
win-x86), the RID was incorrectly being passed to referenced class library projects during MSBuild's output determination process. This caused class libraries to incorrectly consider the RuntimeIdentifier when resolving their output paths, leading to build errors like MSB3030 ("Could not copy the file... because it was not found").Class libraries should remain platform-agnostic and not be affected by the RuntimeIdentifier during publish operations.
Root Cause
The issue occurred because MSBuild's
_GetProjectReferenceTargetFrameworkPropertiestarget was passing theRuntimeIdentifierproperty to all referenced projects during publish operations, including class libraries that should not consider platform-specific identifiers.Solution
This PR adds a targeted MSBuild fix that uses the standard
ProjectReferenceGlobalPropertiesToRemovemechanism to preventRuntimeIdentifierfrom being passed to referenced projects during publish operations.The fix:
IsPublishing=true,DeployOnBuild=true, or whenPublishProfileis set)RuntimeIdentifierfrom properties passed to project referencesFiles Changed
build/NuSpecs/Microsoft.WindowsAppSDK.PublishFix.targets(new) - Contains the fix logicbuild/NuSpecs/Microsoft.WindowsAppSDK.Foundation.targets- Imports the fix for WinUI projectsTesting
The fix has been validated with comprehensive MSBuild tests covering:
This resolves the scenario described in the issue where running:
would incorrectly pass the RuntimeIdentifier to class library projects.
Fixes #13.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.