-
Notifications
You must be signed in to change notification settings - Fork 1.1k
NET tool roll forward error experience #38210
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
base: release/9.0.2xx
Are you sure you want to change the base?
Conversation
break; | ||
|
||
// RuntimeConfigDetectionResult incompatible | ||
// TBD: if include -g in sample command; which version should be included in the install; --force? |
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.
Tests should be included for --force
and IsRuntimeConfigCompatible
functions
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.
It looks like --force
was added but maybe not IsRuntimeConfigCompatible
.
src/Resolvers/Microsoft.DotNet.NativeWrapper/NETCoreSdkResolverNativeWrapper.cs
Outdated
Show resolved
Hide resolved
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.
Looking good!
src/Cli/dotnet/commands/dotnet-tool/install/LocalizableStrings.resx
Outdated
Show resolved
Hide resolved
src/Resolvers/Microsoft.DotNet.NativeWrapper/NETCoreSdkResolverNativeWrapper.cs
Outdated
Show resolved
Hide resolved
src/Tests/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs
Outdated
Show resolved
Hide resolved
IntPtr parametersPtr = Marshal.AllocHGlobal(Marshal.SizeOf(parameters)); | ||
Marshal.StructureToPtr(parameters, parametersPtr, false); |
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.
The parameters are not used for anything... do we need this?
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 still needs to be removed.
[StructLayout(LayoutKind.Sequential)] | ||
internal struct hostfxr_initialize_parameters | ||
{ | ||
|
||
} |
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 would be invalid if used - the structure must have at least the first size
field - it's used effectively as a versioning mechanism. And it MUST be set before calling the initialize function.
https://github.com/dotnet/runtime/blob/a79c62ddc8089cf2879ed36eac9aa333b32bde5f/src/native/corehost/hostfxr.h#L78
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 also still needs to be done.
src/Resolvers/Microsoft.DotNet.NativeWrapper/NETCoreSdkResolverNativeWrapper.cs
Outdated
Show resolved
Hide resolved
src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs
Outdated
Show resolved
Hide resolved
src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
Outdated
Show resolved
Hide resolved
var executableFilePath = toolPackageInstance.Commands[0].Executable; | ||
var runtimeConfigFilePath = Path.ChangeExtension(executableFilePath.ToString(), ".runtimeconfig.json"); | ||
|
||
// Check if the runtimeconfig.json file is compatible with the current runtime |
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.
compatible with the current runtime
Just want to call out exactly what this means. The hostfxr_initialize_for_runtime_config
is being run in-proc here, so it will check for compatibility with the already loaded runtime.
- The current/loaded runtime in this context would be what the SDK command is runnning on.
- The SDK chosen is affected by global.json settings and then the runtime is chosen based on the SDK's dotnet.runtimeconfig.json, which specifies the runtime matching the SDK version.
- This means current runtime may not be the latest installed runtime.
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.
Actually, I think this is a larger problem. Checking for compatibility with the loaded runtime means checking for frameworks - not just the core runtime. Since dotnet.dll
only depends on the core runtime, it will only have that loaded.
Tools could depend on other frameworks like AspNetCore or WindowsDesktop, right? I think that is a test case that should be covered. I expect any tools like that would fail the compatibility check 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.
Yes, tool could be depend on other frameworks.
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.
The new hostfxr_resolve_frameworks_for_runtime_config
API considers other types of frameworks. We should call that and see if it returns 0.
src/Resolvers/Microsoft.DotNet.NativeWrapper/NETCoreSdkResolverNativeWrapper.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…rr msg Co-authored-by: Daniel Plaisted <[email protected]>
This pull request addresses the issue where the installation of a tool with an incompatible runtime results in failure. The proposed changes aim to provide clearer instructions for users encountering this issue and offer alternative solutions for resolution.
Changes made for error message:
Instructions with --allow-roll-forward flag: Included a step-by-step guide for users to install the tool with the --allow-roll-forward flag. The provided command (dotnet tool install --allow-roll-forward footool) is copy/pastable for user convenience.
Added guidance on installing .NET: Provided a link to download and install .NET , which is required by the tool. Users encountering compatibility issues are advised to install the required runtime using the provided link. Note that the given prompt leads to a general .NET downloading page https://aka.ms/dotnet/download/sdk/
Instructions with --force flag: Included instructions for users to install the tool with the --force flag and manually configure it to run. The provided command (dotnet tool install --force footool) allows users to force the installation and provides an alternative approach to resolving compatibility issues.
User experience: When installing a footool with incompatible runtime
Note that the global flag command prompt
-g
contains based on global or local tools install. Note that the provided prompt for downloading new .NET framework leads to a general .NET downloading page.