Skip to content
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

Jump to bottom

Fix incorrect app host used in build when PublishSingleFile=true #37114

Merged
merged 6 commits into from
Dec 6, 2023
Merged

Fix incorrect app host used in build when PublishSingleFile=true #37114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fee8213
Fix incorrect app host used in build when PublishSingleFile=true
elinor-fung Nov 22, 2023
15d20c6
Update src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publ…
elinor-fung Nov 27, 2023
44cd883
Apply suggestions from code review
elinor-fung Nov 27, 2023
4eada30
Fix IsTargetFrameworkCompatible condition
elinor-fung Nov 27, 2023
cdd5f16
Apply suggestions from code review
elinor-fung Dec 6, 2023
302472c
Consolidate properties
elinor-fung Dec 6, 2023
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
38 changes: 36 additions & 2 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ Copyright (c) .NET Foundation. All rights reserved.
</ItemDefinitionGroup>

<Target Name="_ComputeToolPackInputsToProcessFrameworkReferences" BeforeTargets="ProcessFrameworkReferences">
<!-- Keep this in sync with the warnings produced for RequiresILLinkPack in ProcessFrameworkReferences.cs.
<!-- Keep this in sync with the warnings produced for RequiresILLinkPack in ProcessFrameworkReferences.cs.
Must be set late enough to see the inferred value of EnableSingleFileAnalyzer. -->
<PropertyGroup>
<_RequiresILLinkPack Condition="'$(_RequiresILLinkPack)' == '' And (
Expand Down Expand Up @@ -101,7 +101,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- Don't warn if the library multitargets and includes TFM that's supported by trimming/AOT and is supported by the min non-EOL TFM that supports trimming/AOT. -->
<_SilenceIsTrimmableUnsupportedWarning Condition="'$(_SilenceIsTrimmableUnsupportedWarning)' == '' And
@(_TargetFrameworkToSilenceIsTrimmableUnsupportedWarning->Count()) > 0">true</_SilenceIsTrimmableUnsupportedWarning>
<_SilenceIsAotCompatibleUnsupportedWarning Condition="'$(_SilenceIsAotCompatibleUnsupportedWarning)' == '' And
<_SilenceIsAotCompatibleUnsupportedWarning Condition="'$(_SilenceIsAotCompatibleUnsupportedWarning)' == '' And
@(_TargetFrameworkToSilenceIsAotCompatibleUnsupportedWarning->Count()) > 0">true</_SilenceIsAotCompatibleUnsupportedWarning>
<_SilenceEnableSingleFileAnalyzerUnsupportedWarning Condition="'$(_SilenceEnableSingleFileAnalyzerUnsupportedWarning)' == '' And
@(_TargetFrameworkToSilenceEnableSingleFileAnalyzerUnsupportedWarning->Count()) > 0">true</_SilenceEnableSingleFileAnalyzerUnsupportedWarning>
Expand Down Expand Up @@ -694,6 +694,30 @@ Copyright (c) .NET Foundation. All rights reserved.
</ItemGroup>
</Target>

<!--
============================================================
_CreateSingleFileHost
Create the single-file host for the publish scenario if app is being published as a self-contained single-file .net5+ app
============================================================
-->
<Target Name="_CreateSingleFileHost"
Inputs="@(IntermediateAssembly);$(SingleFileHostSourcePath)"
Outputs="$(SingleFileHostIntermediatePath)"
DependsOnTargets="_GetAppHostPaths;_GetAppHostCreationConfiguration"
Condition="'$(_UseSingleFileHostForPublish)' == 'true' and
Exists('@(IntermediateAssembly)') and
Exists('$(SingleFileHostSourcePath)')">
<CreateAppHost AppHostSourcePath="$(SingleFileHostSourcePath)"
AppHostDestinationPath="$(SingleFileHostIntermediatePath)"
AppBinaryName="$(AssemblyName)$(TargetExt)"
IntermediateAssembly="@(IntermediateAssembly->'%(FullPath)')"
WindowsGraphicalUserInterface="$(_UseWindowsGraphicalUserInterface)"
Retries="$(CopyRetryCount)"
RetryDelayMilliseconds="$(CopyRetryDelayMilliseconds)"
EnableMacOSCodeSign="$(_EnableMacOSCodeSign)"
/>
</Target>

<!--
============================================================
_ComputeCopyToPublishDirectoryItems
Expand Down Expand Up @@ -736,6 +760,7 @@ Copyright (c) .NET Foundation. All rights reserved.
KeepDuplicateOutputs=" '$(MSBuildDisableGetCopyToPublishDirectoryItemsOptimization)' == '' "
DependsOnTargets="AssignTargetPaths;
DefaultCopyToPublishDirectoryMetadata;
_CreateSingleFileHost;
_SplitProjectReferencesByFileExistence;
_GetProjectReferenceTargetFrameworkProperties">

Expand Down Expand Up @@ -826,6 +851,15 @@ Copyright (c) .NET Foundation. All rights reserved.
Condition="'%(_NoneWithTargetPath.CopyToPublishDirectory)'=='PreserveNewest'"/>
</ItemGroup>

<ItemGroup Condition="'$(_UseSingleFileHostForPublish)' == 'true' and Exists('$(SingleFileHostIntermediatePath)')">
<!-- Remove non-single-file apphost from items to publish -->
<_SourceItemsToCopyToPublishDirectoryAlways Remove="$(AppHostIntermediatePath)" />
<_SourceItemsToCopyToPublishDirectory Remove="$(AppHostIntermediatePath)" />

<!-- Add the single-file host created as part of publish -->
<_SourceItemsToCopyToPublishDirectoryAlways Include="$(SingleFileHostIntermediatePath)" CopyToOutputDirectory="Always" TargetPath="$(AssemblyName)$(_NativeExecutableExtension)" />
</ItemGroup>

<ItemGroup>
<AllPublishItemsFullPathWithTargetPath Include="@(_SourceItemsToCopyToPublishDirectoryAlways->'%(FullPath)');@(_SourceItemsToCopyToPublishDirectory->'%(FullPath)')"/>
</ItemGroup>
Expand Down
53 changes: 24 additions & 29 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,27 @@ Copyright (c) .NET Foundation. All rights reserved.
</CompileDependsOn>
</PropertyGroup>

<!--
============================================================
_GetAppHostPaths
Gets the path to apphost (restored via packages or in an apphost pack),
and computes the path for the destination apphost.
============================================================
-->
<Target Name="_GetAppHostCreationConfiguration"
Condition="'$(UseAppHost)' == 'true' and '$(_IsExecutable)' == 'true'">
<PropertyGroup>
<_UseWindowsGraphicalUserInterface Condition="($(RuntimeIdentifier.StartsWith('win')) or $(DefaultAppHostRuntimeIdentifier.StartsWith('win'))) and '$(OutputType)'=='WinExe'">true</_UseWindowsGraphicalUserInterface>
<_EnableMacOSCodeSign Condition="'$(_EnableMacOSCodeSign)' == '' and $([MSBuild]::IsOSPlatform(`OSX`)) and Exists('/usr/bin/codesign') and
($(RuntimeIdentifier.StartsWith('osx')) or $(AppHostRuntimeIdentifier.StartsWith('osx')))">true</_EnableMacOSCodeSign>
<_UseSingleFileHostForPublish Condition="'$(PublishSingleFile)' == 'true' and
'$(SelfContained)' == 'true' and
'$(SingleFileHostSourcePath)' != '' and
'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and
$([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0))">true</_UseSingleFileHostForPublish>
</PropertyGroup>
</Target>

<!--
============================================================
_CreateAppHost
Expand All @@ -700,17 +721,11 @@ Copyright (c) .NET Foundation. All rights reserved.
<Target Name="_CreateAppHost"
Inputs="@(IntermediateAssembly);$(AppHostSourcePath)"
Outputs="$(AppHostIntermediatePath)"
DependsOnTargets="_ChooseAppHost;CoreCompile"
DependsOnTargets="_GetAppHostPaths;_GetAppHostCreationConfiguration;CoreCompile"
Condition="'$(ComputeNETCoreBuildOutputFiles)' == 'true' and
'$(AppHostSourcePath)' != '' and
Exists('@(IntermediateAssembly)') and
Exists('$(AppHostSourcePath)')">
<PropertyGroup>
<_UseWindowsGraphicalUserInterface Condition="($(RuntimeIdentifier.StartsWith('win')) or $(DefaultAppHostRuntimeIdentifier.StartsWith('win'))) and '$(OutputType)'=='WinExe'">true</_UseWindowsGraphicalUserInterface>
<_EnableMacOSCodeSign Condition="'$(_EnableMacOSCodeSign)' == '' and $([MSBuild]::IsOSPlatform(`OSX`)) and Exists('/usr/bin/codesign') and
($(RuntimeIdentifier.StartsWith('osx')) or $(AppHostRuntimeIdentifier.StartsWith('osx')))">true</_EnableMacOSCodeSign>
</PropertyGroup>

<CreateAppHost AppHostSourcePath="$(AppHostSourcePath)"
AppHostDestinationPath="$(AppHostIntermediatePath)"
AppBinaryName="$(AssemblyName)$(TargetExt)"
Expand All @@ -722,26 +737,6 @@ Copyright (c) .NET Foundation. All rights reserved.
/>
</Target>

<!--
============================================================
_ChooseAppHost
Choose the correct app-host for the build scenario:
* SingleFileHost if an app being published as a self-contained single-file .net5+ app
* AppHost otherwise
============================================================
-->
<Target Name="_ChooseAppHost"
DependsOnTargets="_GetAppHostPaths"
Condition="'$(UseAppHost)' == 'true' and '$(_IsExecutable)' == 'true'">
<PropertyGroup Condition="'$(PublishSingleFile)' == 'true' and
'$(SelfContained)' == 'true' and
'$(_TargetFrameworkVersionWithoutV)' >= '5.0' and
'$(SingleFileHostSourcePath)' != ''">
<AppHostSourcePath>$(SingleFileHostSourcePath)</AppHostSourcePath>
<AppHostIntermediatePath>$([System.IO.Path]::GetFullPath('$(IntermediateOutputPath)singlefilehost$(_NativeExecutableExtension)'))</AppHostIntermediatePath>
</PropertyGroup>
</Target>

<!--
============================================================
_GetAppHostPaths
Expand All @@ -763,6 +758,7 @@ Copyright (c) .NET Foundation. All rights reserved.
</PropertyGroup>
<PropertyGroup Condition="'$(UseAppHostFromAssetsFile)' == 'false' Or '@(_NativeRestoredAppHostNETCore)' != ''">
<AppHostIntermediatePath>$([System.IO.Path]::GetFullPath('$(IntermediateOutputPath)apphost$(_NativeExecutableExtension)'))</AppHostIntermediatePath>
<SingleFileHostIntermediatePath>$([System.IO.Path]::GetFullPath('$(IntermediateOutputPath)singlefilehost$(_NativeExecutableExtension)'))</SingleFileHostIntermediatePath>
</PropertyGroup>

</Target>
Expand Down Expand Up @@ -878,7 +874,7 @@ Copyright (c) .NET Foundation. All rights reserved.
============================================================
-->
<Target Name="_ComputeNETCoreBuildOutputFiles"
DependsOnTargets="_ChooseAppHost;_GetComHostPaths;_GetIjwHostPaths"
DependsOnTargets="_GetAppHostPaths;_GetComHostPaths;_GetIjwHostPaths"
BeforeTargets="AssignTargetPaths"
Condition="'$(ComputeNETCoreBuildOutputFiles)' == 'true'">

Expand All @@ -895,7 +891,6 @@ Copyright (c) .NET Foundation. All rights reserved.
</None>
</ItemGroup>


<ItemGroup Condition="'$(AppHostIntermediatePath)' != '' or '$(_CopyAndRenameDotnetHost)' == 'true'">
<!--
If not copying local lock file assemblies, copy the host policy and host fxr libraries for self-contained builds.
Expand Down
50 changes: 49 additions & 1 deletion src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildASelfContainedApp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,55 @@ public void It_does_not_build_SelfContained_due_to_PublishSelfContained_being_tr
.Pass();

var outputDirectory = buildCommand.GetOutputDirectory(targetFramework);
outputDirectory.Should().NotHaveFile("hostfxr.dll"); // This file will only appear if SelfContained.
outputDirectory.Should().NotHaveFile($"hostfxr{FileNameSuffixes.CurrentPlatform.DynamicLib}"); // This file will only appear if SelfContained.
}

[Fact]
public void It_builds_using_regular_apphost_with_PublishSingleFile()
Copy link
Member

@nagilson nagilson Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Make creating single-file app host using singlefilehost part of publish

It_builds_using_regular_apphost_with_PublishSingleFile

I am confused, this test seems to validate that it does the opposite thing I think it should do? I am sorry for my misunderstanding.

I would expect the single file host to be used when the app is self contained, net 5+, and published as single file... so why does this test that it uses the regular app host and not the single file host and seem to prove that this is true?

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue here was that we were using the single-file host during build rather than only for publish. That resulted in poorly constructed output where it'd be a self-contained layout with the runtime in the output directory, but with the single-file host instead of the regular host. The intention of this change is to make build always use the regular app host and make publish use the single-file host (when applicable).

So this is testing that build with PublishSingleFile=true should use regular app host and not single-file host.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, derp -- I totally see now :) Thank you for clarifying, I should've noticed that this was build. I assume there is already a test for publish that checks that it uses a single file host?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's tests for publish that check the output only has the expected files (so no runtime) and that the app runs properly, which it wouldn't if it weren't the single file host. I figured that was reasonable existing validation, but I can add something to explicitly check the msbuild items if you want.

{
var tfm = ToolsetInfo.CurrentTargetFramework;
var project = new TestProject()
{
IsExe = true,
TargetFrameworks = tfm,
AdditionalProperties = {
{ "PublishSingleFile", "true"},
{ "SelfContained", "true" } }
};
var asset = _testAssetsManager.CreateTestProject(project);

// Validate apphost is used, not singlefilehost
var command = new GetValuesCommand(Log,
Path.Combine(asset.Path, project.Name),
project.TargetFrameworks,
"AllItemsFullPathWithTargetPath",
GetValuesCommand.ValueType.Item);
command.DependsOnTargets = "GetCopyToOutputDirectoryItems";
command.MetadataNames.Add("TargetPath");
command.Execute().Should().Pass();

var itemsWithTargetPaths = command.GetValuesWithMetadata();
itemsWithTargetPaths.Should().NotContain((i) => i.value.EndsWith($"singlefilehost{Constants.ExeSuffix}"));
itemsWithTargetPaths.Should().Contain((i) =>
i.value.EndsWith($"apphost{Constants.ExeSuffix}")
&& i.metadata["TargetPath"] == $"{project.Name}{Constants.ExeSuffix}");

// Validate it builds and runs
var buildCommand = new BuildCommand(asset);
buildCommand
.Execute()
.Should()
.Pass();

string exePath = Path.Combine(
buildCommand.GetOutputDirectory(tfm, runtimeIdentifier: EnvironmentInfo.GetCompatibleRid(tfm)).FullName,
$"{project.Name}{Constants.ExeSuffix}");
new RunExeCommand(Log, exePath)
.Execute()
.Should()
.Pass()
.And
.HaveStdOutContaining("Hello World!");
}

[Theory]
Expand Down