-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Bring back live host change #73095
Bring back live host change #73095
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
also @jkoritzinsky |
@agocke in general looks good but can you please point me to the fix (in this change) for the previous regression? |
@@ -118,6 +118,16 @@ | |||
<MonoIncludeFiles Include="$(MonoArtifactsPath)\include\**\*.*" /> | |||
</ItemGroup> | |||
|
|||
<!-- Host files. Mobile uses a different hosting model, so we don't include the .NET host components there. --> | |||
<ItemGroup Condition="'$(TargetsMobile)' != 'true' and Exists('$(DotNetHostBinDir)')"> |
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.
@ViktorHofer This is the problematic part. By moving the host files to RuntimeFiles they ended up getting copied into the shared framework pack, when they were only supposed to be in the runtime pack. Keeping this in the Microsoft.NetCore.App package fixed the problem.
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.
Also, the changes in externals.proj
basically replicate the behavior that was intended with the above change.
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.
If it works, awesome 👍
@vitek-karas do you know if the host packages (Microsoft.NETCore.DotNetHost and Microsoft.NETCore.DotNetHostPolicy) are still consumed by anyone? Asking as this PR removes our own dependency on them.
@elinor-fung Mentioned that the host packages aren't directly depended on at the moment, but they're our vehicle for symbols for the hosting components. I'd want to bring the change through to stop building them separately, and confirm we still have symbols. |
The previous problem seems to be the usage of RuntimeFiles as the ItemGroup for the host files instead of NativeRuntimeAsset, and the lack of
PackOnly
, which prevents hostfxr from being included in the shared framework package.