-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Installer: Get channels from release.json, and other improvements #51485
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
Installer: Get channels from release.json, and other improvements #51485
Conversation
|
What do we need hostfxr for on this pathway? We will not have access to it at all in AOT mode IIRC, we may need to use something like the library @ericstj pointed me at earlier today to get a managed equivalent to what hostfxr provides. |
Right now we are using it to validate that an SDK or runtime has installed correctly: sdk/src/Installer/dnup/ArchiveInstallationValidator.cs Lines 74 to 106 in 11265f1
I think we were thinking about also using it to find what versions are installed. |
|
Here's the library I mentioned: https://github.com/ericstj/dotnetlocator - it's not production but seems to be the way we're starting to think about this kind of shared resource. I think its expected surface area would be able to satisfy what hostfxr is being used for in the code you linked? |
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.
Thanks, good call out on the AOT portion and the SDK tests not running dnup tests. I've left a few comments.
| public IEnumerable<string> GetSupportedChannels() | ||
| { | ||
|
|
||
| return ["latest", "preview", "lts", "sts", |
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 think we should keep this in a data structure of special keyword channels we support.
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>$(ResolverTargetFramework);net472</TargetFrameworks> | ||
| <IsAotCompatible Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">true</IsAotCompatible> |
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.
For my understanding, why did we use net8.0 here?
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.
When I didn't have the condition it gave me an error saying the property wasn't supported for .NET Framework, and the error suggested this condition. I think the property is maybe supported on .NET 6 and higher.
| for (var i = 0; i < (int)infoStruct.sdk_count; i++) | ||
| { | ||
| var pointer = new IntPtr(infoStruct.sdks.ToInt64() + i * Marshal.SizeOf(typeof(hostfxr_dotnet_environment_sdk_info))); | ||
| var pointer = new IntPtr(infoStruct.sdks.ToInt64() + i * Marshal.SizeOf<hostfxr_dotnet_environment_sdk_info>()); |
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 were some NativeAOT issues with Microsoft.DotNet.NativeWrapper. I've fixed the warnings but I'm not sure how loading the hostfxr libraries is supposed to work in a NativeAOT app. The code expects to find hostfxr.dll in the app base directory. So we may need some more changes there.
There are also trim warnings from Spectre.Console which cause the NatievAOT publish to fail. However, I haven't yet figured out what those warnings actually are.
We should hook up a publish of dnup to the pipelines so that we are validating we can actually produce a NativeAOT app.