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.

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

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

elinor-fung
Copy link
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

@dotnet-issue-labeler dotnet-issue-labeler bot 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
@baronfel baronfel requested a review from a team November 27, 2023 21:10
}

[Fact]
public void It_builds_using_regular_apphost_with_PublishSingleFile()
Copy link
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
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.

@elinor-fung
Copy link
Member Author

@nagilson any other concerns / questions here?

elinor-fung and others added 2 commits December 5, 2023 16:22
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
@elinor-fung
Copy link
Member Author

@dotnet-policy-service rerun

@elinor-fung elinor-fung merged commit 27d55b2 into dotnet:main Dec 6, 2023
16 checks passed
@elinor-fung elinor-fung deleted the build-apphostAlways branch December 6, 2023 20:01
@dsplaisted
Copy link
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