-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 incorrect app host used in build when PublishSingleFile=true #37114
Conversation
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets
Outdated
Show resolved
Hide resolved
20d5788
to
1d6b6d3
Compare
1d6b6d3
to
fee8213
Compare
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
} | ||
|
||
[Fact] | ||
public void It_builds_using_regular_apphost_with_PublishSingleFile() |
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.
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?
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.
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.
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.
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?
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.
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.
@nagilson any other concerns / questions here? |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
@dotnet-policy-service rerun |
Thanks a bunch @elinor-fung! |
apphost
for buildsinglefilehost
part of publishFixes #29795