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

Bring back live host change #73095

Merged
merged 12 commits into from
Aug 2, 2022
Merged

Bring back live host change #73095

merged 12 commits into from
Aug 2, 2022

Conversation

agocke
Copy link
Member

@agocke agocke commented Jul 29, 2022

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.

@dotnet-issue-labeler
Copy link

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.

@ghost ghost assigned agocke Jul 29, 2022
@agocke agocke marked this pull request as ready for review July 31, 2022 00:50
@agocke
Copy link
Member Author

agocke commented Aug 1, 2022

also @jkoritzinsky

@ViktorHofer
Copy link
Member

@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)')">
Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

@ViktorHofer ViktorHofer left a 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.

@agocke
Copy link
Member Author

agocke commented Aug 2, 2022

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants