From c6c78b1a6a434c3c1e7d4de3dc8aa90610aa38f3 Mon Sep 17 00:00:00 2001 From: filipw Date: Fri, 5 Jul 2019 17:46:14 +0200 Subject: [PATCH 1/6] improve msbuild instance scoring --- src/OmniSharp.Host/MSBuild/Discovery/Extensions.cs | 5 +++++ .../Discovery/Providers/StandAloneInstanceProvider.cs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/OmniSharp.Host/MSBuild/Discovery/Extensions.cs b/src/OmniSharp.Host/MSBuild/Discovery/Extensions.cs index 9325d73039..24e9cf30ac 100644 --- a/src/OmniSharp.Host/MSBuild/Discovery/Extensions.cs +++ b/src/OmniSharp.Host/MSBuild/Discovery/Extensions.cs @@ -90,6 +90,11 @@ private static int GetInstanceFeatureScore(MSBuildInstance i) if (i.DiscoveryType == DiscoveryType.StandAlone) score--; + if (i.Version.Major >= 16) + score++; + else + score--; + return score; } } diff --git a/src/OmniSharp.Host/MSBuild/Discovery/Providers/StandAloneInstanceProvider.cs b/src/OmniSharp.Host/MSBuild/Discovery/Providers/StandAloneInstanceProvider.cs index e7bf372fdb..e0e7b44c82 100644 --- a/src/OmniSharp.Host/MSBuild/Discovery/Providers/StandAloneInstanceProvider.cs +++ b/src/OmniSharp.Host/MSBuild/Discovery/Providers/StandAloneInstanceProvider.cs @@ -65,7 +65,7 @@ public override ImmutableArray 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)); From 894346c89cf40d49ad0313b19df14e86c1d26a19 Mon Sep 17 00:00:00 2001 From: filipw Date: Fri, 5 Jul 2019 18:21:12 +0200 Subject: [PATCH 2/6] include mono --- .../MSBuild/Discovery/Providers/MonoInstanceProvider.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OmniSharp.Host/MSBuild/Discovery/Providers/MonoInstanceProvider.cs b/src/OmniSharp.Host/MSBuild/Discovery/Providers/MonoInstanceProvider.cs index fcfa21e93d..3e57d91736 100644 --- a/src/OmniSharp.Host/MSBuild/Discovery/Providers/MonoInstanceProvider.cs +++ b/src/OmniSharp.Host/MSBuild/Discovery/Providers/MonoInstanceProvider.cs @@ -69,9 +69,9 @@ public override ImmutableArray 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 GetInstances() new MSBuildInstance( nameof(DiscoveryType.Mono), toolsPath, - new Version(15, 0), + new Version(16, 0), DiscoveryType.Mono, propertyOverrides.ToImmutable())); } From 36b6e4a16714c88416c626e43708ddae215ac837 Mon Sep 17 00:00:00 2001 From: filipw Date: Sat, 6 Jul 2019 13:08:21 +0200 Subject: [PATCH 3/6] added tests --- .../MSBuildSelectionTests.cs | 37 ++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/tests/OmniSharp.MSBuild.Tests/MSBuildSelectionTests.cs b/tests/OmniSharp.MSBuild.Tests/MSBuildSelectionTests.cs index d527ee4a62..090d8cb82e 100644 --- a/tests/OmniSharp.MSBuild.Tests/MSBuildSelectionTests.cs +++ b/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() @@ -89,7 +89,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 +108,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 ); } From 5f5f3769d34b1c0cc385067b623c0235e9a781dd Mon Sep 17 00:00:00 2001 From: filipw Date: Wed, 10 Jul 2019 22:58:52 +0200 Subject: [PATCH 4/6] added ability to manually override msbuild path --- .../MSBuild/Discovery/DiscoveryType.cs | 3 +- src/OmniSharp.Host/CompositionHostBuilder.cs | 4 +- .../MSBuild/Discovery/Extensions.cs | 5 +++ .../MSBuild/Discovery/MSBuildLocator.cs | 21 ++++++----- .../Providers/MSBuildOverrideOptions.cs | 13 +++++++ .../Providers/UserOverrideInstanceProvider.cs | 37 +++++++++++++++++++ .../Providers/VisualStudioInstanceProvider.cs | 1 + .../Options/MSBuildOptions.cs | 2 +- 8 files changed, 73 insertions(+), 13 deletions(-) create mode 100644 src/OmniSharp.Host/MSBuild/Discovery/Providers/MSBuildOverrideOptions.cs create mode 100644 src/OmniSharp.Host/MSBuild/Discovery/Providers/UserOverrideInstanceProvider.cs diff --git a/src/OmniSharp.Abstractions/MSBuild/Discovery/DiscoveryType.cs b/src/OmniSharp.Abstractions/MSBuild/Discovery/DiscoveryType.cs index ff10a866c6..fc2ff8d211 100644 --- a/src/OmniSharp.Abstractions/MSBuild/Discovery/DiscoveryType.cs +++ b/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 } } diff --git a/src/OmniSharp.Host/CompositionHostBuilder.cs b/src/OmniSharp.Host/CompositionHostBuilder.cs index 9b86c5c28a..f794bf6d34 100644 --- a/src/OmniSharp.Host/CompositionHostBuilder.cs +++ b/src/OmniSharp.Host/CompositionHostBuilder.cs @@ -135,7 +135,9 @@ public static IServiceProvider CreateDefaultServiceProvider( services.AddSingleton(sp => MSBuildLocator.CreateDefault( loggerFactory: sp.GetService(), - assemblyLoader: sp.GetService())); + assemblyLoader: sp.GetService(), + msbuildConfiguration: configuration.GetSection("msbuild"))); + // Setup the options from configuration services.Configure(configuration); diff --git a/src/OmniSharp.Host/MSBuild/Discovery/Extensions.cs b/src/OmniSharp.Host/MSBuild/Discovery/Extensions.cs index 24e9cf30ac..93d121b678 100644 --- a/src/OmniSharp.Host/MSBuild/Discovery/Extensions.cs +++ b/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,7 @@ private static int GetInstanceFeatureScore(MSBuildInstance i) if (i.DiscoveryType == DiscoveryType.StandAlone) score--; + // VS 2019 should be preferred if (i.Version.Major >= 16) score++; else diff --git a/src/OmniSharp.Host/MSBuild/Discovery/MSBuildLocator.cs b/src/OmniSharp.Host/MSBuild/Discovery/MSBuildLocator.cs index 339206c6c8..0abbf7a4a9 100644 --- a/src/OmniSharp.Host/MSBuild/Discovery/MSBuildLocator.cs +++ b/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 _providers; - private MSBuildInstance _registeredInstance; - public MSBuildInstance RegisteredInstance => _registeredInstance; + public MSBuildInstance RegisteredInstance { get; private set; } private MSBuildLocator(ILoggerFactory loggerFactory, IAssemblyLoader assemblyLoader, ImmutableArray 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( 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}'."); diff --git a/src/OmniSharp.Host/MSBuild/Discovery/Providers/MSBuildOverrideOptions.cs b/src/OmniSharp.Host/MSBuild/Discovery/Providers/MSBuildOverrideOptions.cs new file mode 100644 index 0000000000..18a68f5654 --- /dev/null +++ b/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 PropertyOverrides { get; set; } + + public string MSBuildPath { get; set; } + + public string Name { get; set; } + } +} diff --git a/src/OmniSharp.Host/MSBuild/Discovery/Providers/UserOverrideInstanceProvider.cs b/src/OmniSharp.Host/MSBuild/Discovery/Providers/UserOverrideInstanceProvider.cs new file mode 100644 index 0000000000..30374e9a1a --- /dev/null +++ b/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(); + } + + public override ImmutableArray GetInstances() + { + if (_options == null || string.IsNullOrWhiteSpace(_options.MSBuildPath)) + { + return ImmutableArray.Empty; + } + + var builder = ImmutableArray.CreateBuilder(); + 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(); + + } + } +} diff --git a/src/OmniSharp.Host/MSBuild/Discovery/Providers/VisualStudioInstanceProvider.cs b/src/OmniSharp.Host/MSBuild/Discovery/Providers/VisualStudioInstanceProvider.cs index 57a764efa3..5123637648 100644 --- a/src/OmniSharp.Host/MSBuild/Discovery/Providers/VisualStudioInstanceProvider.cs +++ b/src/OmniSharp.Host/MSBuild/Discovery/Providers/VisualStudioInstanceProvider.cs @@ -3,6 +3,7 @@ using System.IO; using System.Linq; using System.Runtime.InteropServices; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging; using Microsoft.VisualStudio.Setup.Configuration; using OmniSharp.Utilities; diff --git a/src/OmniSharp.MSBuild/Options/MSBuildOptions.cs b/src/OmniSharp.MSBuild/Options/MSBuildOptions.cs index bf03468862..3fb0491ae0 100644 --- a/src/OmniSharp.MSBuild/Options/MSBuildOptions.cs +++ b/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; } From d3edf63a4e108dcba8fdac1339c547e19bdc7ef4 Mon Sep 17 00:00:00 2001 From: filipw Date: Wed, 10 Jul 2019 23:01:47 +0200 Subject: [PATCH 5/6] added test for override of msbuild --- .../MSBuildSelectionTests.cs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/OmniSharp.MSBuild.Tests/MSBuildSelectionTests.cs b/tests/OmniSharp.MSBuild.Tests/MSBuildSelectionTests.cs index 090d8cb82e..069003ebdb 100644 --- a/tests/OmniSharp.MSBuild.Tests/MSBuildSelectionTests.cs +++ b/tests/OmniSharp.MSBuild.Tests/MSBuildSelectionTests.cs @@ -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() { From 42e95f8304eb38b8ced1d1fdd43abe8151d5514e Mon Sep 17 00:00:00 2001 From: Filip W Date: Thu, 11 Jul 2019 19:01:03 +0200 Subject: [PATCH 6/6] revert namespace change --- .../MSBuild/Discovery/Providers/VisualStudioInstanceProvider.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OmniSharp.Host/MSBuild/Discovery/Providers/VisualStudioInstanceProvider.cs b/src/OmniSharp.Host/MSBuild/Discovery/Providers/VisualStudioInstanceProvider.cs index 5123637648..57a764efa3 100644 --- a/src/OmniSharp.Host/MSBuild/Discovery/Providers/VisualStudioInstanceProvider.cs +++ b/src/OmniSharp.Host/MSBuild/Discovery/Providers/VisualStudioInstanceProvider.cs @@ -3,7 +3,6 @@ using System.IO; using System.Linq; using System.Runtime.InteropServices; -using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging; using Microsoft.VisualStudio.Setup.Configuration; using OmniSharp.Utilities;