improve msbuild instance scoring (#1545)

* improve msbuild instance scoring

* include mono

* added tests

* added ability to manually override msbuild path

* added test for override of msbuild

* revert namespace change

src/OmniSharp.Abstractions/MSBuild/Discovery/DiscoveryType.cs

@@ -5,6 +5,7 @@ public enum DiscoveryType
         StandAlone = 0,
         DeveloperConsole = 1,
         VisualStudioSetup = 2,
-        Mono = 3
+        Mono = 3,
+        UserOverride = 4
     }
 }

src/OmniSharp.Host/CompositionHostBuilder.cs

@@ -135,7 +135,9 @@ public static IServiceProvider CreateDefaultServiceProvider(
             services.AddSingleton<IMSBuildLocator>(sp =>
                 MSBuildLocator.CreateDefault(
                     loggerFactory: sp.GetService<ILoggerFactory>(),
-                    assemblyLoader: sp.GetService<IAssemblyLoader>()));
+                    assemblyLoader: sp.GetService<IAssemblyLoader>(),
+                    msbuildConfiguration: configuration.GetSection("msbuild")));
+
 
             // Setup the options from configuration
             services.Configure<OmniSharpOptions>(configuration);

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

@@ -79,6 +79,10 @@ private static int GetInstanceFeatureScore(MSBuildInstance i)
         {
             var score = 0;
 
+            // user override gets highest priority
+            if (i.DiscoveryType == DiscoveryType.UserOverride)
+                return int.MaxValue;
+
             if (i.HasDotNetSdksResolvers())
                 score++;
 
@@ -90,6 +94,12 @@ private static int GetInstanceFeatureScore(MSBuildInstance i)
             if (i.DiscoveryType == DiscoveryType.StandAlone)
                 score--;
 
+            // VS 2019 should be preferred
+            if (i.Version.Major >= 16)
+                score++;
+            else
+                score--;
+
             return score;
         }
     }

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

@@ -3,6 +3,7 @@
 using System.IO;
 using System.Reflection;
 using System.Text;
+using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.Logging;
 using OmniSharp.MSBuild.Discovery.Providers;
 using OmniSharp.Services;
@@ -21,9 +22,8 @@ internal class MSBuildLocator : DisposableObject, IMSBuildLocator
         private readonly ILogger _logger;
         private readonly IAssemblyLoader _assemblyLoader;
         private readonly ImmutableArray<MSBuildInstanceProvider> _providers;
-        private MSBuildInstance _registeredInstance;
 
-        public MSBuildInstance RegisteredInstance => _registeredInstance;
+        public MSBuildInstance RegisteredInstance { get; private set; }
 
         private MSBuildLocator(ILoggerFactory loggerFactory, IAssemblyLoader assemblyLoader, ImmutableArray<MSBuildInstanceProvider> providers)
         {
@@ -34,20 +34,21 @@ private MSBuildLocator(ILoggerFactory loggerFactory, IAssemblyLoader assemblyLoa
 
         protected override void DisposeCore(bool disposing)
         {
-            if (_registeredInstance != null)
+            if (RegisteredInstance != null)
             {
                 AppDomain.CurrentDomain.AssemblyResolve -= Resolve;
-                _registeredInstance = null;
+                RegisteredInstance = null;
             }
         }
 
-        public static MSBuildLocator CreateDefault(ILoggerFactory loggerFactory, IAssemblyLoader assemblyLoader)
+        public static MSBuildLocator CreateDefault(ILoggerFactory loggerFactory, IAssemblyLoader assemblyLoader, IConfiguration msbuildConfiguration)
             => new MSBuildLocator(loggerFactory, assemblyLoader,
                 ImmutableArray.Create<MSBuildInstanceProvider>(
                     new DevConsoleInstanceProvider(loggerFactory),
                     new VisualStudioInstanceProvider(loggerFactory),
                     new MonoInstanceProvider(loggerFactory),
-                    new StandAloneInstanceProvider(loggerFactory, allowMonoPaths: true)));
+                    new StandAloneInstanceProvider(loggerFactory, allowMonoPaths: true),
+                    new UserOverrideInstanceProvider(loggerFactory, msbuildConfiguration)));
 
         public static MSBuildLocator CreateStandAlone(ILoggerFactory loggerFactory, IAssemblyLoader assemblyLoader, bool allowMonoPaths)
             => new MSBuildLocator(loggerFactory, assemblyLoader,
@@ -56,12 +57,12 @@ public static MSBuildLocator CreateStandAlone(ILoggerFactory loggerFactory, IAss
 
         public void RegisterInstance(MSBuildInstance instance)
         {
-            if (_registeredInstance != null)
+            if (RegisteredInstance != null)
             {
                 throw new InvalidOperationException("An MSBuild instance is already registered.");
             }
 
-            _registeredInstance = instance ?? throw new ArgumentNullException(nameof(instance));
+            RegisteredInstance = instance ?? throw new ArgumentNullException(nameof(instance));
 
             foreach (var assemblyName in s_msbuildAssemblies)
             {
@@ -120,7 +121,7 @@ private Assembly Resolve(object sender, ResolveEventArgs e)
 
         private Assembly LoadAssemblyByNameOnly(string assemblyName)
         {
-            var assemblyPath = Path.Combine(_registeredInstance.MSBuildPath, assemblyName + ".dll");
+            var assemblyPath = Path.Combine(RegisteredInstance.MSBuildPath, assemblyName + ".dll");
             var result = File.Exists(assemblyPath)
                 ? _assemblyLoader.LoadFrom(assemblyPath)
                 : null;
@@ -135,7 +136,7 @@ private Assembly LoadAssemblyByNameOnly(string assemblyName)
 
         private Assembly LoadAssemblyByFullName(AssemblyName assemblyName)
         {
-            var assemblyPath = Path.Combine(_registeredInstance.MSBuildPath, assemblyName.Name + ".dll");
+            var assemblyPath = Path.Combine(RegisteredInstance.MSBuildPath, assemblyName.Name + ".dll");
             if (!File.Exists(assemblyPath))
             {
                 _logger.LogDebug($"FAILURE: Could not locate '{assemblyPath}'.");

src/OmniSharp.Host/MSBuild/Discovery/Providers/MSBuildOverrideOptions.cs

@@ -0,0 +1,13 @@
+using System.Collections.Generic;
+
+namespace OmniSharp.MSBuild.Discovery.Providers
+{
+    internal class MSBuildOverrideOptions
+    {
+        public Dictionary<string, string> PropertyOverrides { get; set; }
+
+        public string MSBuildPath { get; set; }
+
+        public string Name { get; set; }
+    }
+}

src/OmniSharp.Host/MSBuild/Discovery/Providers/MonoInstanceProvider.cs

@@ -69,9 +69,9 @@ public override ImmutableArray<MSBuildInstance> GetInstances()
                 return NoInstances;
             }
 
-            if (monoVersion < new Version("5.2.0"))
+            if (monoVersion < new Version("5.18.1"))
             {
-                Logger.LogDebug($"Found Mono MSBuild but it could not be used because it is version {monoVersion} and in needs to be >= 5.2.0");
+                Logger.LogDebug($"Found Mono MSBuild but it could not be used because it is version {monoVersion} and in needs to be >= 5.18.1");
                 return NoInstances;
             }
 
@@ -112,7 +112,7 @@ public override ImmutableArray<MSBuildInstance> GetInstances()
                 new MSBuildInstance(
                     nameof(DiscoveryType.Mono),
                     toolsPath,
-                    new Version(15, 0),
+                    new Version(16, 0),
                     DiscoveryType.Mono,
                     propertyOverrides.ToImmutable()));
         }

src/OmniSharp.Host/MSBuild/Discovery/Providers/StandAloneInstanceProvider.cs

@@ -65,7 +65,7 @@ public override ImmutableArray<MSBuildInstance> GetInstances()
                 new MSBuildInstance(
                     nameof(DiscoveryType.StandAlone),
                     toolsPath,
-                    new Version(15, 0),
+                    new Version(16, 0), // we now ship with embedded MsBuild 16.x
                     DiscoveryType.StandAlone,
                     propertyOverrides.ToImmutable(),
                     setMSBuildExePathVariable: true));

src/OmniSharp.Host/MSBuild/Discovery/Providers/UserOverrideInstanceProvider.cs

@@ -0,0 +1,37 @@
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Logging;
+using System;
+using System.Collections.Immutable;
+
+namespace OmniSharp.MSBuild.Discovery.Providers
+{
+    internal class UserOverrideInstanceProvider : MSBuildInstanceProvider
+    {
+        private readonly MSBuildOverrideOptions _options;
+
+        public UserOverrideInstanceProvider(ILoggerFactory loggerFactory, IConfiguration configuration)
+            : base(loggerFactory)
+        {
+            _options = configuration.GetSection("msbuildoverride").Get<MSBuildOverrideOptions>();
+        }
+
+        public override ImmutableArray<MSBuildInstance> GetInstances()
+        {
+            if (_options == null || string.IsNullOrWhiteSpace(_options.MSBuildPath))
+            {
+                return ImmutableArray<MSBuildInstance>.Empty;
+            }
+
+            var builder = ImmutableArray.CreateBuilder<MSBuildInstance>();
+            builder.Add(
+                new MSBuildInstance(
+                    _options.Name ?? $"Overridden MSBuild from {_options.MSBuildPath}",
+                    _options.MSBuildPath,
+                    new Version(99, 0),
+                    DiscoveryType.UserOverride,
+                    _options.PropertyOverrides?.ToImmutableDictionary()));
+            return builder.ToImmutable();
+
+        }
+    }
+}

src/OmniSharp.MSBuild/Options/MSBuildOptions.cs

@@ -1,6 +1,6 @@
 namespace OmniSharp.Options
 {
-    internal class MSBuildOptions
+    public class MSBuildOptions
     {
         public string ToolsVersion { get; set; }
         public string VisualStudioVersion { get; set; }

tests/OmniSharp.MSBuild.Tests/MSBuildSelectionTests.cs

@@ -23,7 +23,7 @@ public void RegisterDefaultInstanceFindsTheBestInstanceAvailable()
                 new MSBuildInstance(
                     "Test Instance",
                     TestIO.GetRandomTempFolderPath(),
-                    Version.Parse("15.1.2.3"),
+                    Version.Parse("16.1.2.3"),
                     DiscoveryType.VisualStudioSetup
                 ).AddDotNetCoreToFakeInstance(),
                 GetStandAloneMSBuildInstance()
@@ -50,7 +50,7 @@ public void RegisterDefaultInstanceFindsTheBestInstanceAvailableEvenWithOtherVal
                 new MSBuildInstance(
                     "Valid Test Instance",
                     TestIO.GetRandomTempFolderPath(),
-                    Version.Parse("15.3.2.1"),
+                    Version.Parse("16.3.2.1"),
                     DiscoveryType.VisualStudioSetup
                 ),
                 GetInvalidMsBuildInstance(),
@@ -59,7 +59,7 @@ public void RegisterDefaultInstanceFindsTheBestInstanceAvailableEvenWithOtherVal
                 new MSBuildInstance(
                     "Another Valid Test Instance",
                     TestIO.GetRandomTempFolderPath(),
-                    Version.Parse("15.1.2.3"),
+                    Version.Parse("16.1.2.3"),
                     DiscoveryType.VisualStudioSetup
                 ).AddDotNetCoreToFakeInstance(),
                 GetStandAloneMSBuildInstance()
@@ -81,6 +81,53 @@ public void RegisterDefaultInstanceFindsTheBestInstanceAvailableEvenWithOtherVal
             msbuildLocator.DeleteFakeInstancesFolders();
         }
 
+        [Fact]
+        public void RegisterDefaultInstanceFindsUserOverrideAvailableEvenWithOtherValidInstances()
+        {
+            var msBuildInstances = new[]
+            {
+                new MSBuildInstance(
+                    "Valid Test Instance",
+                    TestIO.GetRandomTempFolderPath(),
+                    Version.Parse("16.3.2.1"),
+                    DiscoveryType.VisualStudioSetup
+                ),
+                GetInvalidMsBuildInstance(),
+
+                // Valid + Dotnet Core
+                new MSBuildInstance(
+                    "Another Valid Test Instance",
+                    TestIO.GetRandomTempFolderPath(),
+                    Version.Parse("16.1.2.3"),
+                    DiscoveryType.VisualStudioSetup
+                ).AddDotNetCoreToFakeInstance(),
+                GetStandAloneMSBuildInstance(),
+
+                // user override
+                new MSBuildInstance(
+                    "Manually Overridden",
+                    TestIO.GetRandomTempFolderPath(),
+                    Version.Parse("99.0.0"),
+                    DiscoveryType.UserOverride
+                ).AddDotNetCoreToFakeInstance(),
+            };
+
+            var msbuildLocator = new MSFakeLocator(msBuildInstances);
+
+            var logger = LoggerFactory.CreateLogger(
+                nameof(RegisterDefaultInstanceFindsUserOverrideAvailableEvenWithOtherValidInstances)
+            );
+
+            // test
+            msbuildLocator.RegisterDefaultInstance(logger);
+
+            Assert.NotNull(msbuildLocator.RegisteredInstance);
+            Assert.Same(msBuildInstances[4], msbuildLocator.RegisteredInstance);
+
+            // clean up
+            msbuildLocator.DeleteFakeInstancesFolders();
+        }
+
         [Fact]
         public void RegisterDefaultInstanceStillPrefersTheFirstInstance()
         {
@@ -89,7 +136,7 @@ public void RegisterDefaultInstanceStillPrefersTheFirstInstance()
                 new MSBuildInstance(
                     "Test Instance",
                     TestIO.GetRandomTempFolderPath(),
-                    Version.Parse("15.1.2.3"),
+                    Version.Parse("16.1.2.3"),
                     DiscoveryType.VisualStudioSetup
                 ),
                 GetStandAloneMSBuildInstance()
@@ -108,12 +155,39 @@ public void RegisterDefaultInstanceStillPrefersTheFirstInstance()
             msbuildLocator.DeleteFakeInstancesFolders();
         }
 
+        [Fact]
+        public void StandAloneIsPreferredOverVS2017()
+        {
+            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(StandAloneIsPreferredOverVS2017));
+
+            // test
+            msbuildLocator.RegisterDefaultInstance(logger);
+
+            Assert.NotNull(msbuildLocator.RegisteredInstance);
+            Assert.Same(msBuildInstances[1], msbuildLocator.RegisteredInstance);
+
+            // clean up
+            msbuildLocator.DeleteFakeInstancesFolders();
+        }
+
         private static MSBuildInstance GetStandAloneMSBuildInstance()
         {
             return new MSBuildInstance(
                 "Stand Alone :(",
                 TestIO.GetRandomTempFolderPath(),
-                Version.Parse("99.0.0.0"),
+                Version.Parse("16.0.0.0"),
                 DiscoveryType.StandAlone
             );
         }