-
Notifications
You must be signed in to change notification settings - Fork 77
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
Fix dotnet sdk resolution #455
Fix dotnet sdk resolution #455
Conversation
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.
Post-commit comments
/// <returns><code>true</code> if a .NET Core SDK could be located, otherwise <code>false</code>.</returns> | ||
public static bool TryResolveDotNetCoreSdk(IEnvironmentProvider environmentProvider, FileInfo dotnetFileInfo, out DirectoryInfo basePath) | ||
public static bool TryResolveDotNetCoreSdk(IEnvironmentProvider environmentProvider, FileInfo dotnetFileInfo, out DevelopmentEnvironment developmentEnvironment) |
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 tightly couples this helper class to your metadata class now.
@@ -78,11 +76,15 @@ public static bool TryResolveDotNetCoreSdk(IEnvironmentProvider environmentProvi | |||
{ | |||
if (!process.Start()) | |||
{ | |||
developmentEnvironment = new DevelopmentEnvironment("Failed to resolve the .NET SDK. Verify the 'dotnet' command is available on the PATH."); |
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.
Seem to be overloading the usage of DevelopmentEnvironment
to just return different error messages?
Fixes #453
The initial fix for 453 only resolved passing the path which allowed it to find the proper location for the SDKs.
However, the actual outcome from the result of calling
hostfxr_resolve_sdk2
was ignored and not passed back to the calling function to use with the tool.The error message was also returned as an empty string and thus was not showing up as the root cause either.
I validated with a private package in our CI that:
I also linked to the .NET repo which contains the native method's documentation as it does things like calls out the CharSet difference, the behavior of the return, and the values for the callback.
Wasn't sure if there was logging available here, could be a good thing for the future, as you can get the resolved version if
global.json
was used and print that out to the console.Could also be good to add smoke tests which use the nuget package to install and run the tool in a scenario in your CI vs. just running the unit tests against the solution file scenarios?