Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+66 −19
Open

Fix tool projects so they don't remove apphosts when publishing (not packing) #49818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2649518
pull the modification of UseAppHost into a Target execution to preven…
baronfel Jul 16, 2025
06aebaf
add a test so we don't regress
baronfel Jul 16, 2025
d4af932
Make sure that 'any' RID builds never use an apphost during the Resto…
baronfel Jul 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 19 additions & 13 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,6 @@ NOTE: This file is imported from the following contexts, so be aware when writin
<_InnerToolsPublishAot Condition="$(_HasRIDSpecificTools) and '$(PublishAot)' == 'true'">true</_InnerToolsPublishAot>
<PublishAot Condition="!$(_IsRidSpecific) and '$(MSBuildIsRestoring)' != 'true'">false</PublishAot>

<!-- we want to ensure that we don't publish any AppHosts for the 'outer', RID-agnostic builds -->
<UseAppHost Condition="!$(_IsRidSpecific)">false</UseAppHost>
<!-- we want to ensure that we _do_ publish any AppHosts for the 'inner', RID-specific builds -->
<UseAppHost Condition="$(_IsRidSpecific)">true</UseAppHost>

<!-- Tool implementation files are not included in the primary package when the tool has RID-specific packages. So only pack the tool implementation
(and only depend on publish) if there are no RID-specific packages, or if the RuntimeIdentifier is set. -->
<_ToolPackageShouldIncludeImplementation Condition=" '$(PackAsTool)' == 'true' And
Expand Down Expand Up @@ -132,14 +127,27 @@ NOTE: This file is imported from the following contexts, so be aware when writin
<Target Name="SetPackToolProperties"
BeforeTargets="_GenerateRestoreProjectSpec;_GenerateToolsSettingsFileInputCache;_GenerateShimInputCache;_GetOutputItemsFromPack">

<!-- We set UseAppHost here instead of as a 'raw' property because we don't want to impact 'normal' Publish operations on these apps. -->
<PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels flaky to me. We publish before we pack, does this mean that the contents of a normal Publish would be different from a Publish-for-Pack operation? That seems kind of bad and like it would hurt incrementality.

Could we let the app host be created in Publish regardless, but then filter it out of the files that we pack if need be?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I actually thing this brings the two outputs into alignment - when publishing an exe you get an apphost by default. When packing a RID-agnostic tool you never want an apphost. Making this explicit on the interior publish seems like a very safe thing to do IMO. It's also pretty ok from an incrementality perspective - the expensive parts of publishing aren't copying an apphost.

Re: your second part - that's an interesting idea. I confess that I find the idea of more explicitly controlling the publish more attractive though - it feels more intentional and less ad-hoc to me. We're publishing for a distinctly different purpose when we pack a tool - it feels right to express that on the parameters to the publish operation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have some problems with Publish because in some cases the Build needs to be different depending on whether Publish is going to run. This seems like the same thing, only this time the behavior of Publish needs to be different depending on whether we're going to run Pack or not. That's why this feels off to me and why it seems like it would be better to change the behavior of Pack here than that of Publish.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow - by definition I am changing the behavior of Pack - Pack wants the published output of the app, but within specific parameters for framework-dependent, rid--agnostic tools. The 'publish' in this case is an implementation detail of packaging the tool.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self: this thread is about the publish outputs from the users' perspective - that should remain untouched.
We could remove the apphsot as part of packing, publish to an intermediate dir, or something else to solve this in a more clean way.

<!-- we want to ensure that we don't publish any AppHosts for the 'outer', RID-agnostic builds -->
<UseAppHost Condition="!$(_IsRidSpecific)">false</UseAppHost>
<!-- we want to ensure that we _do_ publish any AppHosts for the 'inner', RID-specific builds -->
<UseAppHost Condition="$(_IsRidSpecific)">true</UseAppHost>
</PropertyGroup>

<PropertyGroup Condition="'$(ToolCommandRunner)' == ''">
<ToolCommandRunner Condition="!$(UseAppHost)">dotnet</ToolCommandRunner>
<ToolCommandRunner Condition="$(UseAppHost)">executable</ToolCommandRunner>
</PropertyGroup>

<!-- Needs to be in a target so we don't need to worry about evaluation order with NativeBinary property -->
<PropertyGroup Condition="'$(ToolEntryPoint)' == ''">
<ToolEntryPoint>$(TargetFileName)</ToolEntryPoint>
<ToolEntryPoint Condition="$(_IsRidSpecific) and '$(UseAppHost)' == 'true' ">$(AssemblyName)$(_NativeExecutableExtension)</ToolEntryPoint>
</PropertyGroup>

<!-- inner-build tool packages get a RID suffix -->
<PropertyGroup Condition="'$(_HasRIDSpecificTools)' != '' And $(RuntimeIdentifier) != ''">
<PropertyGroup Condition="'$(_HasRIDSpecificTools)' != '' And '$(RuntimeIdentifier)' != ''">
<PackageId>$(PackageId).$(RuntimeIdentifier)</PackageId>
</PropertyGroup>
</Target>
Expand All @@ -157,7 +165,7 @@ NOTE: This file is imported from the following contexts, so be aware when writin
<Target Name="PackToPublishDependencyIndirection"
DependsOnTargets="$(_PackToolPublishDependency)"/>

<Target Name="PackTool" DependsOnTargets="SetPackToolProperties;GenerateToolsSettingsFileFromBuildProperty;PackToPublishDependencyIndirection;_PackToolValidation;PackToolImplementation"
<Target Name="PackTool" DependsOnTargets="SetPackToolProperties;GenerateToolsSettingsFileFromBuildProperty;PackToPublishDependencyIndirection;_PackToolValidation;PackToolImplementation"
Condition=" '$(PackAsTool)' == 'true' "
Returns="@(TfmSpecificPackageFile)">
<ItemGroup>
Expand Down Expand Up @@ -195,11 +203,6 @@ NOTE: This file is imported from the following contexts, so be aware when writin
<_GenerateToolsSettingsFileCacheFile>$([MSBuild]::NormalizePath($(MSBuildProjectDirectory), $(_GenerateToolsSettingsFileCacheFile)))</_GenerateToolsSettingsFileCacheFile>
</PropertyGroup>

<PropertyGroup Condition="'$(ToolCommandRunner)' == ''">
<ToolCommandRunner Condition="!$(UseAppHost)">dotnet</ToolCommandRunner>
<ToolCommandRunner Condition="$(UseAppHost)">executable</ToolCommandRunner>
</PropertyGroup>

<Target Name="_GenerateToolsSettingsFileInputCache">
<ItemGroup>
<_ToolPackageRuntimeIdentifier Include="$(_UserSpecifiedToolPackageRids)" />
Expand Down Expand Up @@ -410,7 +413,10 @@ NOTE: This file is imported from the following contexts, so be aware when writin

<ItemGroup>
<_rids Include="$(_PackageRids)" />
<_RidSpecificToolPackageProject Include="$(MSBuildProjectFullPath)" AdditionalProperties="RuntimeIdentifier=%(_rids.Identity);" />
<_ridsWithoutAny Include="@(_rids)" Condition="%(Identity) != 'any'" />
<_ridsWithAny Include="@(_rids)" Condition="%(Identity) == 'any'" />
<_RidSpecificToolPackageProject Include="$(MSBuildProjectFullPath)" AdditionalProperties="RuntimeIdentifier=%(_ridsWithoutAny.Identity);" />
<_RidSpecificToolPackageProject Include="$(MSBuildProjectFullPath)" AdditionalProperties="UseAppHost=false;RuntimeIdentifier=any;" Condition="@(_ridsWithAny->Count()) &gt; 0" />
</ItemGroup>

<MSBuild BuildInParallel="true" Projects="@(_RidSpecificToolPackageProject)" Targets="Pack">
Expand Down
41 changes: 41 additions & 0 deletions test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAToolProject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable disable

using System.Runtime.CompilerServices;
using Microsoft.DotNet.Cli.Utils;

namespace Microsoft.NET.Publish.Tests
{
public class GivenThatWeWantToPublishAToolProject : SdkTest
{
public GivenThatWeWantToPublishAToolProject(ITestOutputHelper log) : base(log)
{
}

private TestAsset SetupTestAsset([CallerMemberName] string callingMethod = "")
{
TestAsset helloWorldAsset = _testAssetsManager
.CopyTestAsset("PortableTool", callingMethod)
.WithSource();


return helloWorldAsset;
}

[Fact]
// this test verifies that we don't regress the 'normal' publish experience accidentally in the
// PackTool.targets
public void It_can_publish_and_has_apphost()
{
var testAsset = SetupTestAsset();
var publishCommand = new PublishCommand(testAsset);

publishCommand.Execute();

publishCommand.GetOutputDirectory(targetFramework: ToolsetInfo.CurrentTargetFramework)
.EnumerateFiles().Should().Contain(f => f.Name == "consoledemo" + Constants.ExeSuffix);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a DirectoryInfo assertion which should give better error messages here. Somethnig like outputDirectory.Should().HaveFile($"consoledemo{Constants.ExeSuffix}");

}
}
}
12 changes: 6 additions & 6 deletions test/Microsoft.NET.ToolPack.Tests/GivenThatWeWantToPackAToolProject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ public GivenThatWeWantToPackAToolProject(ITestOutputHelper log) : base(log)

private string SetupNuGetPackage(bool multiTarget, string packageType = null, [CallerMemberName] string callingMethod = "")
{

string id = $"{callingMethod}-{_targetFrameworkOrFrameworks}";
TestAsset helloWorldAsset = _testAssetsManager
.CopyTestAsset("PortableTool", callingMethod + multiTarget + (packageType ?? ""))
.CopyTestAsset("PortableTool", id)
.WithSource()
.WithProjectChanges(project =>
{
Expand All @@ -39,7 +39,7 @@ private string SetupNuGetPackage(bool multiTarget, string packageType = null, [C

var packCommand = new PackCommand(helloWorldAsset);

var result = packCommand.Execute();
var result = packCommand.Execute($"/bl:{id}-{{}}.binlog");
result.Should().Pass();

return packCommand.GetNuGetPackage();
Expand Down Expand Up @@ -192,9 +192,9 @@ public void It_does_not_contain_apphost_exe(bool multiTarget)
string[] args = multiTarget ? new[] { $"/p:TargetFramework={_targetFrameworkOrFrameworks}" } : Array.Empty<string>();
getValuesCommand.Execute(args)
.Should().Pass();
string runCommandPath = getValuesCommand.GetValues().Single();
runCommandPath
.Should().Be("dotnet", because: "The RunCommand should recognize that this is a non-AppHost tool and should use the muxer to launch it");
var runCommand = new FileInfo(getValuesCommand.GetValues().Single());
runCommand.Name
.Should().StartWith("consoledemo", because: "The RunCommand should recognize that this is an AppHost project and should use the muxer to launch it for non-tool use cases.");
}
}

Expand Down
Loading