Skip to content

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

Merged
elinor-fung merged 6 commits into
dotnet:mainfrom
elinor-fung:build-apphostAlways
Dec 6, 2023
Merged

Fix incorrect app host used in build when PublishSingleFile=true#37114
elinor-fung merged 6 commits into
dotnet:mainfrom
elinor-fung:build-apphostAlways

Conversation

@elinor-fung
Copy link
Copy Markdown
Member

  • Always use apphost for build
  • Make creating single-file app host using singlefilehost part of publish
    • Only if single-file, self-contained, and .NET 5+ - otherwise just uses the app host created during by build

Fixes #29795

@elinor-fung elinor-fung requested review from a team and AntonLapounov as code owners November 22, 2023 23:36
@ghost ghost added the untriaged Request triage from a team member label Nov 22, 2023
@elinor-fung elinor-fung force-pushed the build-apphostAlways branch 2 times, most recently from 20d5788 to 1d6b6d3 Compare November 23, 2023 01:40
Comment thread src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets Outdated
Comment thread src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets Outdated
@baronfel baronfel requested a review from a team November 27, 2023 21:10
Comment thread src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets Outdated
}

[Fact]
public void It_builds_using_regular_apphost_with_PublishSingleFile()
Copy link
Copy Markdown
Member

@nagilson nagilson Nov 28, 2023

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@elinor-fung
Copy link
Copy Markdown
Member Author

@nagilson any other concerns / questions here?

Comment thread src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets Outdated
Comment thread src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets Outdated
Comment thread src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets Outdated
Comment thread src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets Outdated
elinor-fung and others added 2 commits December 5, 2023 16:22
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
@elinor-fung
Copy link
Copy Markdown
Member Author

@dotnet-policy-service rerun

@elinor-fung elinor-fung merged commit 27d55b2 into dotnet:main Dec 6, 2023
@elinor-fung elinor-fung deleted the build-apphostAlways branch December 6, 2023 20:01
@dsplaisted
Copy link
Copy Markdown
Member

Thanks a bunch @elinor-fung!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Single-File untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If PublishSingleFile=true is set in the project file it changes the behavior of dotnet build and similar

5 participants