improve msbuild instance scoring

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
  );
  }