Remove MSBuildSDKsPath hack that blocks MSBuild SDK resolvers #1192
Conversation
This commit removes an old hack in the MSBuild project loader that blocks using custom MSBuild SDKs. Details: The MSBuild project loader has a bit of a hack to try and find the correct .NET Core SDK for a given project. Essentially, it tries to launch "dotnet --info" in the project directory and sets the MSBuildSDKsPath using the path information that's returned. This results in several problems: 1. "dotnet --info" gets run for *every* project that gets loaded, even if they aren't SDK-style projects. 2. This hack overrides custom MSBuild SDK resolvers. This worked OK while there really weren't custom SDK resolvers out there. However, that is no longer the case with MSBuild's new NuGet-based SDK resolver. This SDK resolver is used to acquire and use custom MSBuild SDKs. So, this hack blocks that feature. OmniSharp does provide a default SDK resolver. However, this hack was left in place because that SDK resolver has problems with discovering much older .NET Core SDKs, i.e. <= 1.0.3. It's unlikely that many users are still on such old versions, but for those who are, a new option has been added to restore the hack.
This change moves the DotNetCliService from a singleton class instantiated via MEF to a service that is added to OmniSharp's IServiceProvider. The is a new IDotNetCliService interface in OmniSharp.Abstractions and the DotNetCliService implementation has been moved to OmniSharp.Host. This greatly cleans up how DotNetCliService is instantiated by tests and allows the .NET CLI path to be precomputed for testing. In addition, IEventEmitter is now elevated to a required service in the IServiceProvider.
This change pushes IOmniSharpEnvironment into OmniSharp's IServiceProvider rather than just adding it to the MEF composition.
This change updates the MSBuildLocator to also load assemblies if they match by name, version and public key token. This is necessary to handle situations where an MSBuild SDK resolver has a dependency that is placed in the MSBuild tools path, which is the case on Mono.
|
This should also address #1138, but I'm not able to set up that repro up because I don't where to find the |
|
One other change here that I forgot to mention is the update to the MSBuildLocator AssemblyResolve handler. Now it will try to load assemblies other than the core MSBuild assemblies. However, it will only load them if they match by name, version and public key token. This is necessary to load dependencies for NuGet MSBuild SDK resolver, which live in the MSBuild tools path on Mono. |
There was a problem hiding this comment.
makes a lot of sense 👍thanks a lot for taking the time to describe each commit in such details, it reads very well and is very clear what is happening.
This caught my eye too - The is a new IDotNetCliService interface in OmniSharp.Abstractions and the DotNetCliService implementation has been moved to OmniSharp.Host. - a very nice change indeed.
Also, the embedded Mono bits got bumped, but the way I understand it, the global Mono minimum version stays the same with this PR, correct?
Correct. And, the version of Mono needed to build OmniSharp has not changed. |
Fixes #1190
This change ended up being a bit larger than I'd intended. If you're reviewing, I recommend walking through each commit at a time.
At it's core, this change removes an old hack that set the
MSBuildSDKsPathenvironment variable to a path that was computed by launchingdotnetin the project folder. This hack is an override that blocks MSBuild's SDK resolvers, which is the system that should be used to locate the SDK(s) for a project. However, when we tried removing this hack in the past, we were bit by the fact that the base MSBuild SDK resolve doesn't work with .NET Core SDKs <= 1.0.3. So, we added the hack back. Since there weren't really any custom MSBuild SDK resolvers out there, the override really didn't have much of an impact.Now that MSBuild has shipped the NuGet-based SDK resolver that can download SDKs from NuGet, our override hack is a significant problem. So, I've removed the hack and the base SDK resolver is used instead. If anybody wants to the old behavior (because they're using an old .NET Core SDK), they can set the following option in an
omnisharp.jsonfile:{ "MSBuild": { "UseLegacySdkResolver": true } }Unfortunately, removing the hack forced several other changes, primarily to get tests working without the hack. I had to rationalize the
IDotNetCliServiceand elevatedIOmniSharpEnvironmentandIEventEmitterto required services. Finally, because the base SDK resolver is a netstandard2.0 library, I needed to packagenetstandard.dllwith our standalone Mono install. As part of this, I went ahead and updated the Mono and MSBuild bits for Mono as well.Note that there is further work to package the NuGet MSBuild SDK resolver with our standalone MSBuild bits. However, that's a change for another day. For now, custom MSBuild SDKs should work if the user has a newer VS or Mono installed.
cc @nguerrera.