Only consider sdks from before the runtime #106

Forgind · 2020-12-07T22:53:33Z

Fixes #92

This is now based on #79.

rainersigwald · 2020-12-09T19:48:22Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

-            if (!int.TryParse(versionMatch.Groups[1].Value, out int major) ||
-                !int.TryParse(versionMatch.Groups[2].Value, out int minor) ||
-                !int.TryParse(versionMatch.Groups[3].Value, out int patch))
+            if (!Version.TryParse(File.ReadAllText(versionPath), out Version version) || version > Environment.Version)


Environment.Version is a strange beast. It's always 4.0.30319.42000 for .NET 4.6 and higher and for .NET Core 1.0-3.1. Starting with .NET 5.0, it returns a value more like you'd probably expect: dotnet/coreclr#22664.

FORTUNATELY, since this PR is mostly about blocking out 5.0 versions from 3.1 hosts (since people mostly didn't complain about 3.1 versions for 2.1 hosts) this simple call should still work. And keep working into the future for 6.0/7.0 on 5.0 and so on.

rainersigwald · 2020-12-09T19:50:10Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

@@ -14,7 +14,6 @@ namespace Microsoft.Build.Locator
    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);


Have you tried your new code against a preview SDK? I think the additional complexity here over just using Version.TryParse is to handle a semver version like 6.0.100-alpha.234622.

…front of the list

ladipro · 2020-12-14T09:02:32Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

+            string parseableVersion = File.ReadAllText(versionPath);
+            int indexOfHyphen = parseableVersion.IndexOf('-');
+            parseableVersion = indexOfHyphen >= 0 ? parseableVersion.Substring(0, indexOfHyphen) : parseableVersion;
+            if (!Version.TryParse(parseableVersion, out Version version) || version > Environment.Version)


Should we first merge #79 and then move the version filtering to RegisterDefaults? I.e. the default behavior would be to pick the latest SDK not newer than the currently running runtime but the developer can implement their own policy if they choose so? There are legitimate use cases where the developer wants to know about all instances even if they are not compatible with the current process -- they might want to launch MSBuild out-of-proc for example.

I'd like to get a minimal 5.0 fix out ASAP but I think this is a good way to go long-term.

I made a change that I think implements this. I tested it with 5.0 and 3.1 runtimes, and they correctly identified 5.0 and 3.1 as the SDKs to use respectively, both ignoring my 2.2 SDK.

…b.com/kurtcodemander/MSBuildLocator into assemblies-older-than-runtime-only

…er-than-runtime-only

Forgind · 2020-12-16T18:19:09Z

src/MSBuildLocator/MSBuildLocator.cs

+            VisualStudioInstance instance = instances.Where(
+                inst => inst.Version.Major < Environment.Version.Major || (inst.Version.Major == Environment.Version.Major && inst.Version.Minor <= Environment.Version.Minor)
+                ).FirstOrDefault() ??
+                instances.FirstOrDefault();


Do you think I should keep this as a fallback? It only ever would return an SDK from after the runtime, but "No instances of MSBuild could be detected" would be misleading. I could also add another error message for "We found MSBuild, but please update the runtime before using it."

rainersigwald · 2020-12-16T21:48:50Z

src/MSBuildLocator/MSBuildLocator.cs

+            VisualStudioInstance instance = instances.Where(
+                inst => inst.Version.Major < Environment.Version.Major || (inst.Version.Major == Environment.Version.Major && inst.Version.Minor <= Environment.Version.Minor)
+                ).FirstOrDefault() ??
+                instances.FirstOrDefault();


I don't think this filtering makes sense for the .NET Framework/desktop MSBuild/VS case. Instead, it should be restricted to the .NET Core/5.0 case.

rainersigwald · 2020-12-16T21:59:49Z

src/MSBuildLocator/MSBuildLocator.cs

@@ -93,7 +93,11 @@ public static IEnumerable<VisualStudioInstance> QueryVisualStudioInstances()
        /// <returns>Instance of Visual Studio found and registered.</returns>
        public static VisualStudioInstance RegisterDefaults()


Why does RegisterDefaults need to change at all? I would expect the change to be entirely in the list returned by GetInstances. I can see from the commit history that you implemented it that way and then changed it, but not why.

I was trying to implement #106 (comment)

Is that wrong?

Ah. Yes, I now think it's not the right design. We could potentially add another overload that takes a bool to "give me everything, I don't care" but I don't think we should filter in RegisterDefaults and I do think we should present only usable instances in the list by default.

Reverted to filtering in DotNetSdkLocationHelper.cs.

I'm a little resistant to add an overload for "give me everything" without anyone explicitly asking for it. I'm guessing most people who use MSBuildLocator use it for relatively straightforward purposes, and it would make it a little harder to pull in my unit testing PR when that comes along.

How would this affect a unit testing PR?

Two reasons: it would add something else I'd probably want to test, and since I'm already planning to add an overload to this, I'd have to split my overload into "give me everything" and "don't give me everything" cases. Not a huge deal, though.

rainersigwald · 2020-12-17T17:02:33Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

-            }
-
-            if (!int.TryParse(versionMatch.Groups[1].Value, out int major) ||
+            if (!versionMatch.Success ||


I prefer the original flow here because the failure conditions are much more obvious in separate ifs:

The version regex didn't match.

We can't parse a version out.

the version doesn't match

Here all together it's hard to see that there are separate failure cases.

rainersigwald · 2020-12-17T17:03:28Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

+                major > Environment.Version.Major ||
+                (major == Environment.Version.Major && minor > Environment.Version.Minor))


This definitely needs a comment! Describe all the things you've learned about why we care about the current runtime version relative to the SDK version, and the SDK policy as we currently understand it.

rainersigwald · 2020-12-17T17:03:43Z

src/MSBuildLocator/MSBuildLocator.cs

@@ -93,7 +93,11 @@ public static IEnumerable<VisualStudioInstance> QueryVisualStudioInstances()
        /// <returns>Instance of Visual Studio found and registered.</returns>
        public static VisualStudioInstance RegisterDefaults()


How would this affect a unit testing PR?

rainersigwald

LGTM. Let's get it out!

rainersigwald · 2020-12-17T18:13:20Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

+
+            // Components of the SDK often have dependencies on the runtime they shipped with, including that several tasks that shipped
+            // in the .NET 5 SDK rely on the .NET 5.0 runtime. Assuming the runtime that shipped with a particular SDK has the same version,
+            // this ensures that we don't choose an SDK that doesn't work with the chosen runtime. This is not guaranteed to always work but


Suggested change

// this ensures that we don't choose an SDK that doesn't work with the chosen runtime. This is not guaranteed to always work but

// this ensures that we don't choose an SDK that doesn't work with the runtime of the current application. This is not guaranteed to always work but

?

@rainersigwald

Suggestion from @rainersigwald

kurtgronbech and others added 8 commits November 29, 2019 20:49

Return all installed DotNetSDK instances

7d57e5c

Correctly check the index returned by FindIndex

36ca39d

Resolved feedback from JoeRobich. Also added support for .NET 5.0.

da34277

Cleaned some whitespace

1eea571

Only consider sdks from before the runtime

3b1f0a0

Merge branch 'master' into get-all-installed-dotnetsdk-instances

5887126

use --list-sdks, fixed instance null-check, removed workingdirectory

b6353cb

Go back to using --info and also get the base path

8069139

rainersigwald reviewed Dec 9, 2020

View reviewed changes

kurtgronbech and others added 6 commits December 9, 2020 22:53

Indentation fix and clearer comment on why we insert basePath at the …

9e91f9e

…front of the list

Removed a couple of test lines which had snuck in

47e602b

Parse versions differently

60cbc46

Updated pattern for SdkRegex

f7573bc

Changed GetDotNetBasePath to be an iterator

afe8a1d

cleaned some whitespace

32a6f0b

ladipro reviewed Dec 14, 2020

View reviewed changes

rainersigwald mentioned this pull request Dec 14, 2020

How to choose .NET Core SDK version? #96

Closed

Forgind added 2 commits December 14, 2020 14:00

Undo version parse change

af73626

Merge branch 'get-all-installed-dotnetsdk-instances' of https://githu…

ae983d3

…b.com/kurtcodemander/MSBuildLocator into assemblies-older-than-runtime-only

Forgind mentioned this pull request Dec 14, 2020

Return all installed DotNetSDK versions #79

Merged

Reverse order of access

ec16d05

Forgind marked this pull request as ready for review December 14, 2020 23:00

Forgind added 2 commits December 14, 2020 15:24

Move check to RegisterDefaults

cd09d13

Merge https://github.com/microsoft/MSBuildLocator into assemblies-old…

75c7f2c

…er-than-runtime-only

Forgind commented Dec 16, 2020

View reviewed changes

rainersigwald requested changes Dec 16, 2020

View reviewed changes

Forgind added 2 commits December 16, 2020 15:14

Restrict filtering to core

3d2ea08

Filter in DotNetSdkLocationHelper

a4af277

rainersigwald requested changes Dec 17, 2020

View reviewed changes

Add comment and split conditions

930ae79

rainersigwald approved these changes Dec 17, 2020

View reviewed changes

Update comment

aa38ab5

Suggestion from @rainersigwald

Forgind merged commit ecf817d into microsoft:master Dec 17, 2020

Forgind deleted the assemblies-older-than-runtime-only branch December 17, 2020 18:34

vxfield mentioned this pull request Dec 29, 2020

Fix to return all installed DotNetSDK versions #117

Closed

rainersigwald mentioned this pull request Jan 15, 2021

Microsoft.Build.Evaluation.Project Constructor throws an exception when dotnet sdk 5.0.100 is installed. dotnet/msbuild#6026

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only consider sdks from before the runtime #106

Only consider sdks from before the runtime #106

Forgind commented Dec 7, 2020 •

edited

Loading

rainersigwald Dec 9, 2020

rainersigwald Dec 9, 2020

ladipro Dec 14, 2020

rainersigwald Dec 14, 2020

Forgind Dec 14, 2020

Forgind Dec 16, 2020 •

edited

Loading

rainersigwald Dec 16, 2020

rainersigwald Dec 16, 2020

Forgind Dec 16, 2020

rainersigwald Dec 16, 2020

Forgind Dec 16, 2020

Forgind Dec 16, 2020 •

edited

Loading

rainersigwald Dec 17, 2020

Forgind Dec 17, 2020

rainersigwald Dec 17, 2020

rainersigwald Dec 17, 2020

rainersigwald Dec 17, 2020

rainersigwald left a comment

rainersigwald Dec 17, 2020

		@@ -93,7 +93,11 @@ public static IEnumerable<VisualStudioInstance> QueryVisualStudioInstances()
		/// <returns>Instance of Visual Studio found and registered.</returns>
		public static VisualStudioInstance RegisterDefaults()

		major > Environment.Version.Major \|\|
		(major == Environment.Version.Major && minor > Environment.Version.Minor))

	// this ensures that we don't choose an SDK that doesn't work with the chosen runtime. This is not guaranteed to always work but
	// this ensures that we don't choose an SDK that doesn't work with the runtime of the current application. This is not guaranteed to always work but

Only consider sdks from before the runtime #106

Only consider sdks from before the runtime #106

Conversation

Forgind commented Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind commented Dec 7, 2020 •

edited

Loading

Forgind Dec 16, 2020 •

edited

Loading

Forgind Dec 16, 2020 •

edited

Loading