Return SDKs from all dotnet location #329
Conversation
| { | ||
| 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 🙂
|
it looks good to me, @rainersigwald do you have any concerns? |
| // We want to return the newest SDKs first. Using OfType will remove the null entry added if we found the best SDK. | ||
| var instances = versionInstanceMap.Values.OfType<VisualStudioInstance>().OrderByDescending(i => i.Version); |
There was a problem hiding this comment.
I am finding this sort confusing with respect to your comment above about keeping the precedence order. That is just per-version, but you'd prefer the overall results sorted by version?
Can you go into more detail on your use case?
There was a problem hiding this comment.
I was trying to preserve the behavior from this comment:
// We want to return the newest SDKs first, however, so iterate over the list in reverse order.
But you are right. I cannot guarantee this will maintain the precedence ordering.
We intend to use this from the MSBuildWorkspace buildhost where we are looking for an SDK and we don't really care where it comes from. If they have pinned a version then we would like to find it, otherwise we would take the SDK with the highest version from any install location.
There was a problem hiding this comment.
Yeah that makes sense I think. The question I have is whether we should embed the sort here, or push it to the caller's layer. I don't think I have super strong opinions so this is fine.
Co-authored-by: Rainer Sigwald <[email protected]>
Want me to do that in this PR? or as a follow up? |
I have bumped the version , no worries! |
|
@darikprescott Note that being able to locate the new SDKs does not do anything to make them more loadable; if you wanted to take advantage of this you'd need a big bag of tricks to find and load a newer runtime to load the newer SDK. |
|
It has been released:https://www.nuget.org/packages/Microsoft.Build.Locator/1.8.1 :) |
5 participants
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.