-
Notifications
You must be signed in to change notification settings - Fork 83
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
upgrade to net6.0 version + add explicit apphost loading for specific platforms #224
upgrade to net6.0 version + add explicit apphost loading for specific platforms #224
Conversation
@@ -11,6 +11,7 @@ | |||
<SignAssembly>true</SignAssembly> | |||
<AssemblyOriginatorKeyFile>key.snk</AssemblyOriginatorKeyFile> | |||
<EnableDefaultNoneItems>false</EnableDefaultNoneItems> | |||
<RuntimeIdentifier Condition="'$(RuntimeIdentifier)' == ''">$(NETCoreSdkRuntimeIdentifier)</RuntimeIdentifier> |
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.
This makes it work when building locally (NETCoreSdkRuntimeIdentifier
is what the SDK is running on) but it's probably not enough to build a package that can be used everywhere.
Can we easily enumerate the problematic RIDs and package up the native dependency for all of them?
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.
I wasn't able to enumerate them easily, but listing runtime assemblies works as expected
@@ -25,6 +25,11 @@ | |||
<PackageReference Include="Microsoft.VisualStudio.SDK.EmbedInteropTypes" Version="15.0.36" PrivateAssets="all" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="'$(TargetFramework)'=='net6.0'"> | |||
<!-- These package references help to resolve assemblies in runtime for platforms, where these are not loaded to AppContext by default (e.g MacOS). --> | |||
<PackageReference Include="Microsoft.NETCore.DotNetAppHost" Version="7.0.9" /> |
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.
MSBuildLocator targeting net6.0 suggests to me that we should be using the latest 6.0.x version of this package - how do consumers on different TFMs need to resolve this package? Is the DotNetAppHost package something that needs to be 'linked' to the final caller's TFM, so that net8.0 maps to 8.0 packages, net7.0 to 7.0.x packages, etc?
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.
I just wanted to avoid adding this package for Framework target.
Probably either @rainersigwald or @ladipro can answer these questions.
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.
Presumably the latest hosting library works with older versions of the runtime, as I think for us it is a requirement to be able to locate a .NET 7 MSBuild using MSBuildLocator built against .NET 6. @vitek-karas, can you please confirm?
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.
Thet nethost
(which is the one thing used from this package) is definitely fully backward compatible. There's obviously no 100% forward compatibility forever, but in general we will try really hard to keep it forward compatible for as long as possible.
So in that sense, use the latest version of the package regardless of what TFM you target (just like it's a good idea to use the latest SDK to build your app regardless of the TFM) - and upgrade this often (once 8 ships, upgrade to that and so on).
@ladipro , @rainersigwald is it ok that we have started to generate this folder in output? How will it be shipped? |
@YuliiaKovalova this looks expected, the native libraries should not be taking too much extra space.
It should become part of the Microsoft.Build.Locator package. |
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.
Can you please check that the hostfxr
P/Invoke works when MSBuildLocator is used via a PackageReference
. i.e. that the newly added native dependencies are correctly packed?
Also, is it possible to cover the P/Invoke with a test? Something that would have previously failed on Mac or Linux.
Actually, it should still be failing because we're currently not making the call to |
It works as expected now because the source issue with the missed lib is resolved. Previously, it was failing here https://github.com/YuliiaKovalova/MSBuildLocator/blob/a7e81c92e079900d3403e71542c59d6cc767daaf/src/MSBuildLocator/DotNetSdkLocationHelper.cs#L116C36-L116C56 with
There is no need to request the path. |
That's odd, it looks like we're not even loading the |
Problem
Fixes: #dotnet/msbuild#9038
MSBuildLocator expects that hostfxr lib is already loaded in the process - that is true for Windows load library, but not for all platforms (macOS, for example).
Some of our customers face an issue like:
Solution
Microsoft.NETCore.DotNetAppHost is a metadata package that loads a platform-specific assembly in runtime.
Specifying reference to it explicitly
<PackageReference Include="runtime.$(RuntimeIdentifier).Microsoft.NETCore.DotNetAppHost" Version="7.0.9" />
helps to copy the content in project/bin folder , so missed library can be resolved.