-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix tool projects so they don't remove apphosts when publishing (not packing) #49818
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix tool projects so they don't remove apphosts when publishing (not packing) #49818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where tool projects were incorrectly removing apphosts during publish operations. The issue was that PackTool targets were unconditionally overwriting UseAppHost and related properties at evaluation time, causing published tool projects to lack AppHosts when they were expected.
- Moved UseAppHost property modifications from evaluation-time to a target that runs early in the pack process
- Added a test to verify that tool projects can publish with apphosts correctly
- Relocated ToolCommandRunner property group to be set within the SetPackToolProperties target
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAToolProject.cs | Added new test class to verify tool projects can publish with apphosts |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets | Moved UseAppHost and ToolCommandRunner property settings from evaluation-time to SetPackToolProperties target |
Comments suppressed due to low confidence (2)
test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAToolProject.cs:35
- The test should verify that the publish command executed successfully before checking for the apphost file. Consider adding
.Should().Pass()
or similar assertion after Execute().
publishCommand.Execute();
test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAToolProject.cs
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets
Show resolved
Hide resolved
b43db53
to
06aebaf
Compare
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets
Show resolved
Hide resolved
…t modifications for non-Pack workflows
…RID-specific tools requested
…untimeIdentifier=any not implicitly bring in apphost assets. Find a more universal way to determine if a tool package should keep its platform-specific executable.
ca002d9
to
f282413
Compare
/backport to release/10.0.1xx-preview7 |
Started backporting to release/10.0.1xx-preview7: https://github.com/dotnet/sdk/actions/runs/16449036667 |
Fixes #49799
The PackTool targets were unconditionally overwriting the UseAppHost and related properties at evaluation-time. This results in publishes of the projects not having AppHosts when they were expected to.
Instead of modifying the way that the projects are Published, we now use knowledge of the intent of the Publish to determine if the apphost/binary should be included in the generated tool package.