Return SDKs from all dotnet location #329
Conversation
| { | ||
| break; | ||
| } | ||
| NativeMethods.hostfxr_get_available_sdks(exe_dir: dotnetPath, result: (key, value) => resolvedPaths = value); |
There was a problem hiding this comment.
How about this?
| NativeMethods.hostfxr_get_available_sdks(exe_dir: dotnetPath, result: (key, value) => resolvedPaths = value); | |
| int rc = NativeMethods.hostfxr_get_available_sdks(exe_dir: dotnetPath, result: (key, value) => resolvedPaths = value); | |
| if (rc == 0 && resolvedPaths != null && resolvedPaths.Length > 0) | |
| { | |
| foreach (string path in resolvedPaths) | |
| { | |
| yield return path; | |
| } | |
| } |
There was a problem hiding this comment.
Ah, thanks in my head it was accumulating them but I see now it isn't. Will update.
There was a problem hiding this comment.
Definitely a bit confusing with how the native APIs work 🙂 I struggled with that for a bit when I wrote the first version of this.
| { | ||
| string? bestSdkPath; | ||
| string[] allAvailableSdks; | ||
| try |
There was a problem hiding this comment.
Inlined GetDotNetBasePaths
| string? bestSDK = GetSdkFromGlobalSettings(workingDirectory); | ||
| if (!string.IsNullOrEmpty(bestSDK)) | ||
| // Returns the list of all available SDKs ordered by ascending version. | ||
| static string[] GetAllAvailableSDKs() |
There was a problem hiding this comment.
This method and GetSdkFromGlobalSettings were only referenced from the inlined GetDotNetBasePaths so I made them local functions.
| for (int i = dotnetPaths.Length - 1; i >= 0; i--) | ||
| { | ||
| if (dotnetPaths[i] != bestSDK) | ||
| if (rc == 0 && resolvedPaths != null) |
There was a problem hiding this comment.
Previous behavior was that we would not throw if resolvedPaths was an empty array.
There was a problem hiding this comment.
I think this would work the way I'd expect! @baronfel mentioned a concern about diverging from dotnet.exe's behavior in not using other dotnet.exes, but I would want to better understand his reasoning, as I'd argue that MSBuildLocator is (and always has been) more flexible with respect to which MSBuild it ultimately loads than dotnet.exe is, so I see no reason it shouldn't also be more flexible as far as which it finds. That said, we should still solicit feedback from MSBuild, as I don't actually own this product 🙂
| // Only add an SDK once, even if it's installed in multiple locations. | ||
| if (!versionInstanceMap.ContainsKey(dotnetSdk.Version)) | ||
| { | ||
| versionInstanceMap.Add(dotnetSdk.Version, dotnetSdk); | ||
| } |
There was a problem hiding this comment.
Tiny nit:
I think we can skip to:
| // Only add an SDK once, even if it's installed in multiple locations. | |
| if (!versionInstanceMap.ContainsKey(dotnetSdk.Version)) | |
| { | |
| versionInstanceMap.Add(dotnetSdk.Version, dotnetSdk); | |
| } | |
| // Only add an SDK once, even if it's installed in multiple locations. | |
| versionInstanceMap[dotnetSdk.Version] = dotnetSdk; |
(One fewer dictionary access)
There was a problem hiding this comment.
So I am unsure if it is a benefit or not, by keeping the first SDK for each discovered version we maintain a preference for SDKs in the following install location ordering.
- DOTNET_ROOT
- Current process path if MSBuild.Locator is called from dotnet.exe
- DOTNET_HOST_PATH
- DOTNET_MSBUILD_SDK_RESOLVER_CLI_DIR
- PATH
Review with whitespace off as the formatter removed trailing whitespace.
The method which finds all SDKs has an early exit which stops it from finding SDKs from more than one dotnet install. This PR removes the early exit and filters the returned instances by their reported version number. This should deduplicate the SDKs.