Re-architect how MSBuild is loaded by OmniSharp #988

DustinCampbell · 2017-10-25T16:55:12Z

With MSBuild 15, locating MSBuild on the user's box can be a significant challenge. OmniSharp provides a version of MSBuild as a fallback, but that can often only work with .NET Core/Standard SDK-style projects; projects targeting .NET Framework can run into significant issues.

This PR introduces an IMSBuildLocator service that is used to discover instances of MSBuild 15 installed on the user's machine. It can discover MSBuild 15 using a few methods:

(Windows only) Using the COM-based Visual Studio setup API.
(Windows only) Checking environment variables that are set by a Visual Studio Developer Prompt
(OSX/Linux only) Looking for a global Mono installation that contains MSBuild 15.
If MSBuild 15 can't be discovered using the three methods above, it falls back to the standalone MSBuild included in OmniSharp.

Once MSBuild is located, OmniSharp will install an AppDomain.AssemblyResolve event to ensure that it loads the Microsoft.Build.* assemblies from that location. This ensures that the MSBuild used by OmniSharp will be compatible with whatever MSBuild toolset is used to process the user's project. In addition, this can speed up processing depending on whether the Microsoft.Build.* assemblies are NGen'd or AOT'd in the location they're loaded from.

Note that, because we're using AssemblyResolve to load Microsoft.Build.* assemblies, the event must be hooked before OmniSharp is composed with MEF. Otherwise, MEF will fail to compose.

In addition, as part of this change, I've reorganized the .msbuild folder to layout more closely to how it's laid out in Visual Studio.

A few things I want to do before merging this:

Delete the MSBuildEnvironment class. This is still there because there's still logic in this class that hasn't been replicated in an MSBuild discoverer yet.
More testing on OSX/Linux with and without Mono installed.
Testing on OSX/Linux when an older Mono is installed.
Add more logging in MSBuild discovery to help identify failures (especially with Mono).

Final note: There's an issue with Mono and AssemblyResolve where it can get into a bad state an attempt is made to load an assembly before the AssemblyResolve event is hooked. On desktop CLR, this works as expected. The core issue here is that xUnit uses Assembly.Load on every assembly in the directory to try and locate xUnit types and that happens before our AssemblyResolve is installed. Currently, this is working OK on OSX/Linux, but it's going to be a problem at some point. I've filed the following bug on Mono to get this fixed: https://bugzilla.xamarin.com/show_bug.cgi?id=60100.

DustinCampbell · 2017-10-25T16:56:37Z

.travis.yml

+    if [ "$TRAVIS_OS_NAME" == "osx" ]; then
+      rvm install ruby-2.3.3
+    fi
+


This is old code. Will remove this.

rchande · 2017-10-25T22:00:56Z

src/OmniSharp.Host/CompositionHostBuilder.cs

+            var msbuildLocator = _serviceProvider.GetRequiredService<IMSBuildLocator>();
+            var instances = msbuildLocator.GetInstances();
+            var instance = instances.FirstOrDefault();
+            if (instance != null)


Are we supposed to do anything if we couldn't locate any instances?

We should probably report an error to the log. Will add that.

rchande · 2017-10-25T22:02:49Z

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

+                }
+            }
+
+            return null;


For my own edification, what happens if Resolve returns null?

The runtime will continue to the next AssemblyResolve handler it finds. From https://msdn.microsoft.com/en-us/library/system.appdomain.assemblyresolve%28v=vs.110%29.aspx?f=255&MSPPError=-2147217396:

If more than one event handler is registered for this event, the event handlers are called in order until an event handler returns a value that isn't null. Subsequent event handlers are ignored.

rchande · 2017-10-25T22:18:21Z

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

+            return msbuildDirectory;
+        }
+
+        private static string FindLocalMSBuildFromSolution()


What about adding a command line argument that we can pass while debugging that specifies the standalone msbuild location? That way we don't have to talk about "OmniSharp.sln" in the product code.

Maybe, but where would it get populated from? Note that the product code already referenced "OmniSharp.sln". This PR just moved that code to here.

We would add the argument to the arguments passed by the debugger or by tests. Not a huge deal either way.

I suppose we could add some MSBuild logic to add an argument passed to the debugger (which still worked when other arguments were added). I'm not sure how it'd work for tests, but I'm guessing we could figure something out. I'm just not sure it's all that valuable.

rchande · 2017-10-25T22:21:32Z

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

+                var instances = new ISetupInstance[1];
+                do
+                {
+                    instanceEnum.Next(1, instances, out fetched);


Does this only enumerate Dev15 versions?

That's all it can do. This is the API for the Willow installer.

Which gets picked if someone has dev15 and a dev15 preview installed?

It's a bit arbitrary. We don't have a way for the user to select one, so it's whichever is first in the list. However, we check for the Visual Studio tied to a developer command prompt first. So, if the user wants to launch OmniSharp with a specific Visual Studio they can launch it from the command prompt that was installed for that VS. Alternatively, they can set the environment variables that the developer command prompt uses.

rchande · 2017-10-25T22:27:32Z

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

+
+            return ImmutableArray.Create(
+                new MSBuildInstance(
+                    "DEVCONSOLE",


nameof(DiscoveryType.DevConsole)?

This change re-organizes the layout of the MSBuild folder to more closely match the layout of Visual Studio: * "msbuild" - This will be the MSBuildExtensionsPath * "msbuild\15.0" - common props go here * "msbuild\15.0\bin" - The MSBuild tools path, where much of the MSBuild actually goes. In addition, the MSBuild runtime and library packages have been updated to the latest, and the package references for the libraries are marked as `PrivateAssets="all"` in the OmniSharp.MSBuild project. This last change means that OmniSharp no longer runs, since the MSBuild libraries aren't copied to the OmniSharp base application path any longer. Later commits will fix this.

The MSBuildLocator service has provider model for discovering MSBuild in various scenarios: * DevConsole * VS 2017 installation * Mono * Stand alone (i.e. the MSBuild that we ship with OmniSharp) Once an MSBuild instance is discovered and registered, an AssemblyResolve event is used to ensure that various MSBuild assemblies are used from that instance rather than using MSBuild assemblies included with OmniSharp.

…ated

…d on the installed Mono

DustinCampbell · 2017-10-27T11:37:34Z

@david-driscoll, @filipw, @rchande : I've done a fair amount of manual testing with this change on OSX and am feeling pretty confident about it. The core MSBuild project scenarios are still working, some scenarios work better, and some new scenarios have starting working. Here are the scenarios I tested:

No mono installed + run OmniSharp standalone:
- Omnisharp-roslyn does not work (expected due to .NETFramework, 4.6 being missing)
- .NET Core MVC project works with .NET Core SDK installed
- Unity does not work (expected due to .NETFramework 2.0 being missing)
Mono 4.8 installed + run OmniSharp standalone:
- Omnisharp-roslyn works
- .NET Core MVC project works
- Unity project works
Mono 5.2 installed + run OmniSharp standalone:
- Omnisharp-roslyn works
- .NET Core MVC project works
- Unity project works
- PCL project does not work (unfortunately, this is expected at the moment - cf. omnisharp still having a hard time with xamarin dotnet/vscode-csharp#1779)
- Xamarin forms project with iOS and Android heads works
Mono 5.2 installed + run OmniSharp on installed Mono:
- Omnisharp-roslyn works
- .NET Core MVC project works
- Unity project works
- PCL project works (omnisharp still having a hard time with xamarin dotnet/vscode-csharp#1779)
- Xamarin forms project with iOS and Android heads works
Mono 5.4 installed + run OmniSharp standalone:
- Omnisharp-roslyn works
- .NET Core MVC project works
- Unity project works
- PCL project does not work (unfortunately, this is expected at the moment - cf. omnisharp still having a hard time with xamarin dotnet/vscode-csharp#1779)
- Xamarin forms project with iOS and Android heads works
Mono 5.4 installed + run OmniSharp on installed Mono:
- Omnisharp-roslyn works
- .NET Core MVC project works
- Unity project works
- PCL project works (omnisharp still having a hard time with xamarin dotnet/vscode-csharp#1779)
- Xamarin forms project with iOS and Android heads works

filipw · 2017-10-27T12:02:02Z

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

+            {
+                var assemblyPath = Path.Combine(_registeredInstance.MSBuildPath, assemblyName.Name + ".dll");
+                var result = File.Exists(assemblyPath)
+                    ? Assembly.LoadFrom(assemblyPath)


should this use https://github.com/OmniSharp/omnisharp-roslyn/blob/master/src/OmniSharp.Host/Services/AssemblyLoader.cs#L62 ?

Yes, you're right. I didn't use it initially because I was still trying to figure out when/how the IMSBuildLocator would be initialized. New commit adds this.

filipw · 2017-10-27T12:20:50Z

Looks like a massive improvement 👍👍👍

One question, should this also recognize that I have just the standalone MsBuild Tools installed, even if I don't have VS installed?
Maybe it's edge scenario, but the reason I mention that is that this would be the set up on Build Agents (where no VS is installed). Personally, I'd often run VS Code + C# extension on those machines, in case some failing builds need debugging or closer look.

DustinCampbell · 2017-10-27T14:51:25Z

@filipw: I haven't specifically tested that scenario, but it should already work. The MSBuild 15.0 build tools use the same installer as VS 2017. So, the MSBuild discovery that looks for Visual Studio using the setup API should also work for the Build Tools as well.

david-driscoll · 2017-10-27T15:06:12Z

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

+                var instances = new ISetupInstance[1];
+                do
+                {
+                    instanceEnum.Next(1, instances, out fetched);


Which gets picked if someone has dev15 and a dev15 preview installed?

rchande · 2017-10-27T15:11:07Z

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

@@ -20,6 +21,32 @@ public override ImmutableArray<MSBuildInstance> GetInstances()
                return NoInstances;
            }

+            // Don't resolve to MSBuild assemblies under the installed Mono unless OmniSharp
+            // is actually running under the installed Mono.
+            var monoRuntimePath = PlatformHelper.GetMonoRuntimePath();


Is this obvious? Maybe explain a bit further what goes wrong if this is not the case?

None of this is obvious. 😄 But you're right, I'll add a little bit more detail about how it can go wrong.

rchande · 2017-10-27T15:12:25Z

Thanks for testing this so thoroughly and documenting what you tested @DustinCampbell. cc @piotrpMSFT #988 (comment) explains the size of the test matrix.

DustinCampbell · 2017-10-27T15:25:45Z

FWIW, I don't normally test all of those scenarios, but they were necessary for this particular change. Too often, changes like these have broken unity support.

DustinCampbell · 2017-10-27T18:03:25Z

OK. I think it's good now. I'm going to go ahead and merge. Let me know if you think of anything else.

filipw · 2017-10-27T18:19:39Z

excellent!

enricosada · 2017-10-27T18:50:26Z

awesome. i am going to steal it. thx @DustinCampbell !
in f# land we just really need the location of msbuild.exe and is tricky to find

jetersen · 2017-10-27T23:44:20Z

@DustinCampbell Did you try mono mdk? the package provided by Xamarin people through brew? 👍

DustinCampbell · 2017-10-27T23:48:17Z

I installed the Mono MDK with the installer

jetersen · 2017-10-27T23:48:58Z

👍 Awesome

DustinCampbell · 2017-10-29T00:39:33Z

FWIW, I'll check brew install cask mono-mdk on Monday.

jetersen · 2017-10-29T00:41:03Z

SAME 👍

corngood · 2017-10-30T14:23:53Z

This seems to have broken mono/windows, see #1001

DustinCampbell · 2017-10-30T14:39:12Z

@corngood: Why are you running OmniSharp on Mono/Windows? We've never tested that scenario. The assumption has generally been that desktop CLR on Windows is the way to go for OmniSharp.

DustinCampbell · 2017-10-30T14:39:37Z

@Casz: I just checked brew cask install mono-mdk and it works fine.

corngood · 2017-10-30T14:47:07Z

@DustinCampbell I'm working around a bug w/ net46. We can discuss on #1001

jetersen · 2017-10-30T15:26:39Z

@DustinCampbell I tested it with our dotnet 4.5 project and it did not work 😢

DustinCampbell · 2017-10-30T15:30:00Z

@Casz: Bummer! Could you file an issue with your OmniSharp log?

jetersen · 2017-10-30T15:43:21Z

@DustinCampbell did I have to do something special or was downloading c# extension enough for VS Code?
Using 1.12.1 c# extension in vscode

jetersen · 2017-10-30T15:44:04Z

Oh found the beta install guide... my bad

jetersen · 2017-10-30T15:57:29Z

@DustinCampbell it works almost dotnet/vscode-csharp#1821

DustinCampbell requested review from david-driscoll, filipw and rchande October 25, 2017 16:55

DustinCampbell commented Oct 25, 2017

View reviewed changes

DustinCampbell force-pushed the locate-msbuild branch from 8f1a4d2 to 7aa51bc Compare October 25, 2017 20:54

rchande reviewed Oct 25, 2017

View reviewed changes

DustinCampbell added 15 commits October 27, 2017 03:51

Remove Travis script code to install newer Ruby

0a0264a

Ensure that 'CscToolPath' and 'CscToolExe' are set on Mono

f73d789

Improve MSBuild discovery robustness and delete MSBuildEnvironment class

8a1a11c

Set 'CscToolPath' to OmniSharp's local MSBuild on Mono

06a41c3

Log MSBuild property overrides

4305d28

Use correct path to MSBuild Roslyn folder on Mono

ad85cd6

MSBuild discovery on Mono should only succeed if Mono >= 5.2.0 is loc…

23d32e2

…ated

Introduce helper for 'ImmutableArray<MSBuildInstance>.Empty'

c9488a2

Use nameof in a few places

74c21f7

Add a bit more logging to MSBuild discovery

14a273a

Fix build to kill OmniSharp unix processes after running scripts

7c4b7c8

Update Mono MSBuild discovery to only succeed if OmniSharp is launche…

f3f2b68

…d on the installed Mono

Increase indent slightly in output

1c5f21f

DustinCampbell force-pushed the locate-msbuild branch from 3de0818 to 1c5f21f Compare October 27, 2017 11:06

filipw reviewed Oct 27, 2017

View reviewed changes

david-driscoll approved these changes Oct 27, 2017

View reviewed changes

rchande approved these changes Oct 27, 2017

View reviewed changes

DustinCampbell added 2 commits October 27, 2017 08:47

Use IAssemblyLoader.LoadFrom(...)

7f263dc

Add more detailed comment about Mono MSBuild discovery

9bdf658

DustinCampbell self-assigned this Oct 27, 2017

DustinCampbell merged commit f0fc64e into OmniSharp:master Oct 27, 2017

enricosada mentioned this pull request Oct 27, 2017

MsBuild madness ionide/FsAutoComplete#176

Closed

DustinCampbell deleted the locate-msbuild branch November 2, 2017 17:08

filipw mentioned this pull request Jan 15, 2019

Why Omnisharp Roslyn isn't .NET Core base on OSX/Linux ? #1384

Closed

filipw mentioned this pull request Feb 8, 2020

Is it possible to have omnisharp-roslyn target .NET Core? #1703

Closed

Re-architect how MSBuild is loaded by OmniSharp #988

Re-architect how MSBuild is loaded by OmniSharp #988

Conversation

DustinCampbell commented Oct 25, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell Oct 26, 2017 • 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

Choose a reason for hiding this comment

DustinCampbell commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipw commented Oct 27, 2017

DustinCampbell commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande commented Oct 27, 2017

DustinCampbell commented Oct 27, 2017

DustinCampbell commented Oct 27, 2017

filipw commented Oct 27, 2017

enricosada commented Oct 27, 2017 • edited Loading

jetersen commented Oct 27, 2017

DustinCampbell commented Oct 27, 2017

jetersen commented Oct 27, 2017

DustinCampbell commented Oct 29, 2017

jetersen commented Oct 29, 2017

corngood commented Oct 30, 2017

DustinCampbell commented Oct 30, 2017

DustinCampbell commented Oct 30, 2017

corngood commented Oct 30, 2017

jetersen commented Oct 30, 2017

DustinCampbell commented Oct 30, 2017

jetersen commented Oct 30, 2017

jetersen commented Oct 30, 2017

jetersen commented Oct 30, 2017

DustinCampbell commented Oct 25, 2017 •

edited

Loading

DustinCampbell Oct 26, 2017 •

edited

Loading

enricosada commented Oct 27, 2017 •

edited

Loading