Merged
Conversation
jeffkl
approved these changes
Feb 21, 2023
michael-hawker
commented
Feb 21, 2023
Contributor
Author
michael-hawker
left a comment
There was a problem hiding this comment.
Post-commit comments
| /// <param name="developmentEnvironment">Receives a <see cref="DevelopmentEnvironment" /> instance containing details of the .NET Core SDK if one is found.</param> | ||
| /// <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) |
Contributor
Author
There was a problem hiding this comment.
This tightly couples this helper class to your metadata class now.
| { | ||
| if (!process.Start()) | ||
| { | ||
| developmentEnvironment = new DevelopmentEnvironment("Failed to resolve the .NET SDK. Verify the 'dotnet' command is available on the PATH."); |
Contributor
Author
There was a problem hiding this comment.
Seem to be overloading the usage of DevelopmentEnvironment to just return different error messages?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_sdk2was 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.jsonwas 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?