Change the way to look for the default MSBuild instance

src/OmniSharp.Host/MSBuild/Discovery/Extensions.cs

@@ -1,52 +1,104 @@
-using Microsoft.Extensions.Logging;
+using System.IO;
+using Microsoft.Extensions.Logging;
 
 namespace OmniSharp.MSBuild.Discovery
 {
     internal static class Extensions
     {
         public static void RegisterDefaultInstance(this IMSBuildLocator msbuildLocator, ILogger logger)
         {
-            MSBuildInstance instanceToRegister = null;
-            var invalidVSFound = false;
+            var bestInstanceFound = GetBestInstance(msbuildLocator, out var invalidVSFound);
 
-            foreach (var instance in msbuildLocator.GetInstances())
-            {
-                if (instance.IsInvalidVisualStudio())
-                {
-                    invalidVSFound = true;
-                }
-                else
-                {
-                    instanceToRegister = instance;
-                    break;
-                }
-            }
-
-
-            if (instanceToRegister != null)
+            if (bestInstanceFound != null)
             {
                 // Did we end up choosing the standalone MSBuild because there was an invalid Visual Studio?
                 // If so, provide a helpful message to the user.
-                if (invalidVSFound && instanceToRegister.DiscoveryType == DiscoveryType.StandAlone)
+                if (invalidVSFound && bestInstanceFound.DiscoveryType == DiscoveryType.StandAlone)
                 {
-                    logger.LogWarning(@"It looks like you have Visual Studio 2017 RTM installed.
-Try updating Visual Studio 2017 to the most recent release to enable better MSBuild support.");
+                    logger.LogWarning(
+                        @"It looks like you have Visual Studio 2017 RTM installed.
+ Try updating Visual Studio 2017 to the most recent release to enable better MSBuild support."
+                    );
                 }
 
-                msbuildLocator.RegisterInstance(instanceToRegister);
+                msbuildLocator.RegisterInstance(bestInstanceFound);
             }
             else
             {
                 logger.LogError("Could not locate MSBuild instance to register with OmniSharp");
             }
         }
 
+        public static bool HasDotNetSdksResolvers(this MSBuildInstance instance)
+        {
+            const string dotnetSdkResolver = "Microsoft.DotNet.MSBuildSdkResolver";
+
+            return File.Exists(
+                Path.Combine(
+                    instance.MSBuildPath
+                    , "SdkResolvers"
+                    , dotnetSdkResolver
+                    , dotnetSdkResolver + ".dll"
+                )
+            );
+        }
 
         public static bool IsInvalidVisualStudio(this MSBuildInstance instance)
+
             // MSBuild from Visual Studio 2017 RTM cannot be used.
             => instance.Version.Major == 15
-            && instance.Version.Minor == 0
-            && (instance.DiscoveryType == DiscoveryType.DeveloperConsole
-                || instance.DiscoveryType == DiscoveryType.VisualStudioSetup);
+                && instance.Version.Minor == 0
+                && (instance.DiscoveryType == DiscoveryType.DeveloperConsole
+                    || instance.DiscoveryType == DiscoveryType.VisualStudioSetup);
+
+        public static MSBuildInstance GetBestInstance(this IMSBuildLocator msbuildLocator, out bool invalidVSFound)
+        {
+            invalidVSFound = false;
+            MSBuildInstance bestMatchInstance = null;
+            var bestMatchScore = -1;
+
+            // same as (without so many allocations)
+            // var bestMatchInstance =  msbuildLocator
+            //     .GetInstances()
+            //     .Select(i => (instance: i, featureScore: GetFeatureScore(i)))
+            //     .OrderByDescending(i => i.featureScore)
+            //     .ThenByDescending(i => i.instance.Version.Major)
+            //     .Select(i => i.instance)
+            //     .FirstOrDefault();
+
+            foreach (var instance in msbuildLocator.GetInstances())
+            {
+                var score = GetInstanceFeatureScore(instance);
+
+                invalidVSFound = invalidVSFound || instance.IsInvalidVisualStudio();
+
+                if (score > bestMatchScore
+                    || (score == bestMatchScore && instance.Version.Major > (bestMatchInstance?.Version.Major ?? 0)))
+                {
+                    bestMatchInstance = instance;
+                    bestMatchScore = score;
+                }
+            }
+
+            return bestMatchInstance;
+        }
+
+        private static int GetInstanceFeatureScore(MSBuildInstance i)
+        {
+            var score = 0;
+
+            if (i.HasDotNetSdksResolvers())
+                score++;
+
+            if (i.IsInvalidVisualStudio())
+                score--;
+            else
+                score++;
+
+            if (i.DiscoveryType == DiscoveryType.StandAlone)
+                score--;
+
+            return score;
+        }
     }
 }

tests/OmniSharp.MSBuild.Tests/ExtensionsTests.cs

@@ -0,0 +1,132 @@
+using System;
+using OmniSharp.MSBuild.Discovery;
+using TestUtility;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace OmniSharp.MSBuild.Tests
+{
+    public class ExtensionsTests : AbstractTestFixture
+    {
+        public ExtensionsTests(ITestOutputHelper output)
+            : base(output)
+        {
+        }
+
+        [Fact]
+        public void RegisterDefaultInstanceFindsTheBestInstanceAvailable()
+        {
+            var msBuildInstances = new[]
+            {
+                GetInvalidMsBuildInstance()
+
+                // Valid
+                , new MSBuildInstance(
+                    "Test Instance"
+                    , TestIO.GetRandomTempFolderPath()
+                    , Version.Parse("15.1.2.3")
+                    , DiscoveryType.VisualStudioSetup
+                ).AddDotNetCoreToFakeInstance()
+                , GetStandAloneMSBuildInstance()
+            };
+
+            var msbuildLocator = new MSFakeLocator(msBuildInstances);
+            var logger = LoggerFactory.CreateLogger(nameof(RegisterDefaultInstanceFindsTheBestInstanceAvailable));
+
+            // test
+            msbuildLocator.RegisterDefaultInstance(logger);
+
+            Assert.NotNull(msbuildLocator.RegisteredInstance);
+            Assert.Same(msBuildInstances[1], msbuildLocator.RegisteredInstance);
+
+            // clean up
+            msbuildLocator.DeleteFakeInstancesFolders();
+        }
+
+        [Fact]
+        public void RegisterDefaultInstanceFindsTheBestInstanceAvailableEvenWithOtherValidInstances()
+        {
+            var msBuildInstances = new[]
+            {
+                new MSBuildInstance(
+                    "Valid Test Instance"
+                    , TestIO.GetRandomTempFolderPath()
+                    , Version.Parse("15.3.2.1")
+                    , DiscoveryType.VisualStudioSetup
+                )
+                , GetInvalidMsBuildInstance()
+
+                // Valid + Dotnet Core
+                , new MSBuildInstance(
+                    "Another Valid Test Instance"
+                    , TestIO.GetRandomTempFolderPath()
+                    , Version.Parse("15.1.2.3")
+                    , DiscoveryType.VisualStudioSetup
+                ).AddDotNetCoreToFakeInstance()
+                , GetStandAloneMSBuildInstance()
+            };
+
+            var msbuildLocator = new MSFakeLocator(msBuildInstances);
+
+            var logger = LoggerFactory.CreateLogger(
+                nameof(RegisterDefaultInstanceFindsTheBestInstanceAvailableEvenWithOtherValidInstances)
+            );
+
+            // test
+            msbuildLocator.RegisterDefaultInstance(logger);
+
+            Assert.NotNull(msbuildLocator.RegisteredInstance);
+            Assert.Same(msBuildInstances[2], msbuildLocator.RegisteredInstance);
+
+            // clean up
+            msbuildLocator.DeleteFakeInstancesFolders();
+        }
+
+        [Fact]
+        public void RegisterDefaultInstanceStillPrefersTheFirstInstance()
+        {
+            var msBuildInstances = new[]
+            {
+                new MSBuildInstance(
+                    "Test Instance"
+                    , TestIO.GetRandomTempFolderPath()
+                    , Version.Parse("15.1.2.3")
+                    , DiscoveryType.VisualStudioSetup
+                )
+                , GetStandAloneMSBuildInstance()
+            };
+
+            var msbuildLocator = new MSFakeLocator(msBuildInstances);
+            var logger = LoggerFactory.CreateLogger(nameof(RegisterDefaultInstanceStillPrefersTheFirstInstance));
+
+            // test
+            msbuildLocator.RegisterDefaultInstance(logger);
+
+            Assert.NotNull(msbuildLocator.RegisteredInstance);
+            Assert.Same(msBuildInstances[0], msbuildLocator.RegisteredInstance);
+
+            // clean up
+            msbuildLocator.DeleteFakeInstancesFolders();
+        }
+
+        private static MSBuildInstance GetStandAloneMSBuildInstance()
+        {
+            return new MSBuildInstance(
+                "Stand Alone :("
+                , TestIO.GetRandomTempFolderPath()
+                , Version.Parse("99.0.0.0")
+                , DiscoveryType.StandAlone
+            );
+        }
+
+        private static MSBuildInstance GetInvalidMsBuildInstance()
+        {
+            return new MSBuildInstance(
+                "Invalid Instance"
+                , TestIO.GetRandomTempFolderPath()
+                , Version.Parse("15.0.4.2")
+                , DiscoveryType.VisualStudioSetup
+            );
+        }
+    }
+}

tests/TestUtility/MSFakeLocator.cs

@@ -0,0 +1,34 @@
+using System.Collections.Generic;
+using System.Collections.Immutable;
+using System.IO;
+using OmniSharp.MSBuild.Discovery;
+
+namespace TestUtility
+{
+    public class MSFakeLocator : IMSBuildLocator
+    {
+        private readonly ImmutableArray<MSBuildInstance> _instances;
+
+        public MSBuildInstance RegisteredInstance { get; private set; }
+
+        public MSFakeLocator(IEnumerable<MSBuildInstance> instances)
+        {
+            _instances = instances.ToImmutableArray();
+        }
+
+        public void RegisterInstance(MSBuildInstance instance)
+            => RegisteredInstance = instance;
+
+        public ImmutableArray<MSBuildInstance> GetInstances()
+            => _instances;
+
+        public void DeleteFakeInstancesFolders()
+        {
+            foreach (var instance in _instances)
+            {
+                if (Directory.Exists(instance.MSBuildPath))
+                    Directory.Delete(instance.MSBuildPath, true);
+            }
+        }
+    }
+}

tests/TestUtility/TestFolder.cs

@@ -0,0 +1,24 @@
+using System.IO;
+
+namespace TestUtility
+{
+    public static class TestIO
+    {
+        public static string GetRandomTempFolderPath()
+        {
+            var path = Path.Combine(
+                Path.GetTempPath()
+                , Path.GetRandomFileName()
+            );
+
+            Directory.CreateDirectory(path);
+
+            return path;
+        }
+
+        public static void TouchFakeFile(string path)
+        {
+            File.WriteAllText(path, "just testing :)");
+        }
+    }
+}

tests/TestUtility/TestHelpers.cs

@@ -1,12 +1,14 @@
 using System;
 using System.Collections.Generic;
+using System.IO;
 using System.Linq;
 using System.Reflection;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.Scripting.Hosting;
 using Microsoft.Extensions.Logging;
 using OmniSharp;
 using OmniSharp.FileWatching;
+using OmniSharp.MSBuild.Discovery;
 using OmniSharp.Script;
 using OmniSharp.Services;
 
@@ -24,7 +26,7 @@ public static OmniSharpWorkspace CreateCsxWorkspace(TestFile testFile)
         public static void AddCsxProjectToWorkspace(OmniSharpWorkspace workspace, TestFile testFile)
         {
             var references = GetReferences();
-            var scriptHelper = new ScriptProjectProvider(new ScriptOptions(), new OmniSharpEnvironment(), new LoggerFactory(), true);            
+            var scriptHelper = new ScriptProjectProvider(new ScriptOptions(), new OmniSharpEnvironment(), new LoggerFactory(), true);
             var project = scriptHelper.CreateProject(testFile.FileName, references.Union(new[] { MetadataReference.CreateFromFile(typeof(CommandLineScriptGlobals).GetTypeInfo().Assembly.Location) }), testFile.FileName, typeof(CommandLineScriptGlobals), Enumerable.Empty<string>());
             workspace.AddProject(project);
 
@@ -91,5 +93,22 @@ private static IEnumerable<PortableExecutableReference> GetReferences()
 
             return references;
         }
+
+        public static MSBuildInstance AddDotNetCoreToFakeInstance(this MSBuildInstance instance)
+        {
+            const string dotnetSdkResolver = "Microsoft.DotNet.MSBuildSdkResolver";
+
+            var directory = Path.Combine(
+                instance.MSBuildPath
+                , "SdkResolvers"
+                , dotnetSdkResolver
+            );
+
+            Directory.CreateDirectory(directory);
+
+            TestIO.TouchFakeFile(Path.Combine(directory, dotnetSdkResolver + ".dll"));
+
+            return instance;
+        }
     }
 }