Skip to content
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

Return all installed DotNetSDK versions #79

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 46 additions & 10 deletions src/MSBuildLocator/DotNetSdkLocationHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@ internal static class DotNetSdkLocationHelper
{
private static readonly Regex DotNetBasePathRegex = new Regex("Base Path:(.*)$", RegexOptions.Multiline);
private static readonly Regex VersionRegex = new Regex(@"^(\d+)\.(\d+)\.(\d+)", RegexOptions.Multiline);
private static readonly Regex SdkRegex = new Regex(@"(\S+) \[(.*?)]$", RegexOptions.Multiline);

public static VisualStudioInstance GetInstance(string workingDirectory)
{
string dotNetSdkPath = GetDotNetBasePath(workingDirectory);

public static VisualStudioInstance GetInstance(string dotNetSdkPath)
{
if (string.IsNullOrWhiteSpace(dotNetSdkPath))
{
return null;
Expand Down Expand Up @@ -58,10 +57,20 @@ public static VisualStudioInstance GetInstance(string workingDirectory)
discoveryType: DiscoveryType.DotNetSdk);
}

private static string GetDotNetBasePath(string workingDirectory)
public static IEnumerable<VisualStudioInstance> GetInstances(string workingDirectory)
{
foreach (var basePath in GetDotNetBasePaths(workingDirectory))
{
var dotnetSdk = GetInstance(basePath);
if (dotnetSdk != null)
yield return dotnetSdk;
}
}

private static IEnumerable<string> GetDotNetBasePaths(string workingDirectory)
{
const string DOTNET_CLI_UI_LANGUAGE = nameof(DOTNET_CLI_UI_LANGUAGE);

Process process;
try
{
Expand All @@ -83,12 +92,12 @@ private static string GetDotNetBasePath(string workingDirectory)
catch
{
// when error running dotnet command, consider dotnet as not available
return null;
yield break;
}

if (process.HasExited)
{
return null;
yield break;
}

var lines = new List<string>();
Expand All @@ -109,10 +118,37 @@ private static string GetDotNetBasePath(string workingDirectory)
var matched = DotNetBasePathRegex.Match(outputString);
if (!matched.Success)
{
return null;
yield break;
}

var basePath = matched.Groups[1].Value.Trim();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: GetDotNetBasePaths could be an iterator method and simply yield return basePath here and then yield return path in the loop below. Just a thought, looks good either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea and have submitted a new commit with this change. Looks quite nice and clean.


return matched.Groups[1].Value.Trim();
yield return basePath; // We return the version in use at the front of the list in order to ensure FirstOrDefault always returns the version in use.

var lineSdkIndex = lines.FindIndex(line => line.Contains("SDKs installed"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


if (lineSdkIndex != -1)
{
lineSdkIndex++;

while (lineSdkIndex < lines.Count && !string.IsNullOrEmpty(lines[lineSdkIndex]))
{
var sdkMatch = SdkRegex.Match(lines[lineSdkIndex]);

if (!sdkMatch.Success)
break;

var version = sdkMatch.Groups[1].Value.Trim();
var path = sdkMatch.Groups[2].Value.Trim();

path = Path.Combine(path, version) + Path.DirectorySeparatorChar;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't clear with what I meant on this—sorry about that. I meant that this is not a public-facing method, and when we use basePaths in GetInstance, we use Path.Combine to combine it with other things, so it doesn't matter if it has a slash at the end or not. I noticed the base path from dotnet --info does have the slash, though, so you'd also have to change path.Equals (below) to StartsWith. Ultimately, doesn't really matter either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, but at the bottom in GetInstance we set dotNetSdkPath directly into new VisualStudioInstance(..) and the VisualStudioInstance constructor saves it directly to VisualStudioRootPath and MSBuildPath so that's where it would break the existing functionality having a trailing separator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed that. You're right.


if (!path.Equals(basePath))
yield return path;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of me wants to order this list by version descending (dotnet --info returns results in ascending order). But we already extract the global.json-or-highest version as basePath first, and that ordering is most critical. And we don't order VS instances. So I think the current approach is good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still useful to sort the results even with the basePath first because of #106. If the default SDK is from after the runtime in use, it'll be disqualified, and we'd go with the second in the list. If you don't clean your machine regularly and still have a 1.0 SDK lying around, that's what we'd find and register with RegisterDefaults.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm convinced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I remove the new iterator/yield code and go back to the code using List with Reverse(), or do we have a better solution based on the current code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the best solution. I already wrote up the change in #106, so I wouldn't worry about remaking that change here.


lineSdkIndex++;
}
}
}
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/MSBuildLocator/MSBuildLocator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,7 @@ private static IEnumerable<VisualStudioInstance> GetInstances(VisualStudioInstan
#endif

#if NETCOREAPP
var dotnetSdk = DotNetSdkLocationHelper.GetInstance(options.WorkingDirectory);
if (dotnetSdk != null)
foreach (var dotnetSdk in DotNetSdkLocationHelper.GetInstances(options.WorkingDirectory))
yield return dotnetSdk;
#endif
}
Expand Down
2 changes: 1 addition & 1 deletion version.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "1.2",
"version": "1.3",
"assemblyVersion": "1.0.0.0",
"publicReleaseRefSpec": [
"^refs/heads/release/.*"
Expand Down