Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use MSBuild SDK Resolvers #974

Merged
merged 7 commits into from
Oct 4, 2017

Conversation

DustinCampbell
Copy link
Contributor

@DustinCampbell DustinCampbell commented Oct 3, 2017

Currently, we launch dotnet --info for every MSBuild project. This allows us to detect the .NET Core SDK that is used for that project without having to replicate a lot of code to search for a global.json, or look in various places. Once we've have the output of dotnet --info, we look for the "Base Path" property and use that to decide where the location of the "Sdks" are within the relevant .NET Core SDK's directory. Once we've got that, we set the MSBuildSDKsPath environment variable to tell MSBuild where the SDKs are before we load the project.

This approach has several problems:

  1. Setting an environment variable is fragile! It means that we can't load MSBuild projects in parallel because the projects might target a different .NET Core SDK. Also, another process (even another OmniSharp process) could change or unset it.
  2. Parsing dotnet --info is fragile! The .NET Core SDK's informational output is subject to change, and there are proposals to do it.

To avoid this problem, OmniSharp will deploy the same "SDK Resolvers" that are deployed with Visual Studio. This is a bit of code that knows how to resolve a project to the correct .NET Core SDK. In fact, it uses the same native binary that "dotnet" does to find the SDK. Once present, MSBuild uses the resolver to locate the .NET Core SDK for any project that contains an Sdk attribute.

For tests, we still set the MSBuildSDKsPath environment variable so that we can use the .NET Core SDKs that the OmniSharp build scripts install locally during our test runs. We could write our own SDK Resolver to resolve our local .NET Core SDKs during testing, but that's probably overkill. If it becomes an issue, we can look into that.

With the SDK Resolvers included in OmniSharp, we no longer need to set the MSBuildSDKsPath
environment variable for each project. Instead, the SDK Resolvers should do that work.

For tests, we now ensure that MSBuild is always initialized in standalone mode and we
set the MSBuildSDKsPath environement variable specifically to ensure that the .NET Core SDK(s)
downloaded by the OmniSharp build are used in tests.
@DustinCampbell DustinCampbell changed the title [WIP] Use MSBuild SDK Resolvers Use MSBuild SDK Resolvers Oct 3, 2017
@DustinCampbell DustinCampbell self-assigned this Oct 4, 2017
@DustinCampbell
Copy link
Contributor Author

I just tested on OSX when Mono is installed and when it isn't, and it's working after a small bug fix. I also updated the Mono runtime and MSBuild to newer builds.

This is good for review now.

@DustinCampbell
Copy link
Contributor Author

}
else if (Platform.Current.IsLinux)
{
CopyDotNetHostResolver(env, "linux", "x64", "libhostfxr.so", msbuildSdkResolverFolder, copyToArchSpecificFolder: false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the lack of linux-x86 no longer an issue for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that it's kind of a non-issue since .NET Core isn't supported on linux-x86 anyway. Since I can't install a .NET Core SDK on linux-x86, I can't see a need to resolve to it on linux-x86. 😄

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@DustinCampbell DustinCampbell merged commit 9a8afe7 into OmniSharp:master Oct 4, 2017
@DustinCampbell
Copy link
Contributor Author

Thanks!

@DustinCampbell DustinCampbell deleted the sdk-resolver branch October 4, 2017 21:38
@@ -4,6 +4,6 @@
<clear />
<add key="NuGet" value="https://api.nuget.org/v3/index.json" />
<add key="dotnet-core" value="https://dotnet.myget.org/F/dotnet-core/api/v3/index.json" />
<add key="cli-deps" value="https://dotnet.myget.org/F/cli-deps/api/v3/index.json" />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, we actually want to terminate the cli-deps feed, since it was actually built to contain all the packages the CLI depends on, but we have moved away from that approach since then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was an old feed from long ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants