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

Run OmniSharp on an embedded Mono runtime and add support for MSBuild-based .NET Core projects #886

Merged
merged 4 commits into from
Nov 11, 2016

Conversation

DustinCampbell
Copy link
Member

@DustinCampbell DustinCampbell commented Nov 7, 2016

This change does not really do justice to the update since most of the churn is in the packages that are downloaded. Essentially, this switches the C# extension over to using an embedded Mono runtime and framework to run OmniSharp on. It also moves OmniSharp over to the latest MSBuild 15.0 bits which enable MSBuild-based .NET Core projects.

Fixes #881

@DustinCampbell
Copy link
Member Author

Note: I'm not intending to merge this until after 1.5. There is other work that needs to happen for MSBuild-based .NET Core projects (like updating launch.json generation).

@wjk
Copy link
Contributor

wjk commented Nov 8, 2016

@DustinCampbell May I ask why we need a Mono-based OmniSharp? I would prefer to use the Core-based one, if only then because I know I’m using the same code to build my project as VSCode uses to parse it.

@DustinCampbell
Copy link
Member Author

@wjk:

There a number of reasons why we're moving over to an embedded Mono.

  1. Desktop/Mono MSBuild tasks will not run on CoreCLR. This is a big problem for OmniSharp and one of the key reasons we've never gotten MSBuild working on the .NET Core builds of OmniSharp.
  2. We can't determine which OmniSharp to launch in an "all .csproj" world. When .NET Core projects were based on project.json, we could launch a different flavor of OmniSharp based on project format. So, we'd launch a standalone .NET Core OmniSharp for project.json and a Mono OmniSharp for .csproj/.sln. Now, we don't have a good way to figure that out. Even worse, an .sln might contain a mix of .NET Core and Desktop/Mono projects. In that case, depending on which OmniSharp we would launch, it might only be able to read some of the projects and not others. The only way to ensure that OmniSharp can process all .csproj projects, is to run it as a net46 app rather than netcoreapp1.0.
  3. With .NET Core, we can only run OmniSharp on OSs and distros that .NET Core supports. This is not the main reason for moving, but it's an ongoing problem. By moving to Mono, we can support most distros and versions without having to wait for .NET Core to support them and rebuild OmniSharp.

So, there are several benefits to moving to Mono. However, that also comes with the headache of needing to have Mono installed as a system component. So, an embedded Mono is the way to go. Does that make sense?

@tonerdo
Copy link

tonerdo commented Nov 10, 2016

@DustinCampbell on this point We can't determine which OmniSharp to launch in an "all .csproj" world, a possible workaround will be to check the TargetFramework framework tag in the .csproj and launch a .NET Core OmniSharp if it has a value of netcoreapp1.0 or netstandard1.*.

This approach doesn't handle other scenarios but I guess it's something to think about

@DustinCampbell
Copy link
Member Author

DustinCampbell commented Nov 10, 2016

@tsolarin: Yes, that was discussed. The problem is that it wouldn't ever be guaranteed to be accurate since the TargetFramework can be pushed to a different targets file. If that happens, which do we launch? Also, for the case of a mixed solution, we would need to parse the solution (in TypeScript code within the extension), and check every project to figure out which to launch. And, if it turns out to be mixed, which do we launch?

All of these problems fall away by unifying on running OmniSharp as a net46 application. On Windows, we'll use the desktop's .NET runtime. On OSX/Linux, we'll use the Mono runtime. For the Mono case, we can avoid pre-req'ing a system-wide Mono install by embedding it with OmniSharp.

@DustinCampbell
Copy link
Member Author

Note: I'm produced a new preview release of the C# extension that includes this change. Please feel free to grab it and kick the tires!

@tonerdo
Copy link

tonerdo commented Nov 10, 2016

@DustinCampbell alright then your approach does make a lot of sense. I'll take the preview release for a spin. I'll report whatever issues I find on this PR since it's not an "official" release.
Thanks a lot!

@DustinCampbell DustinCampbell merged commit 12714e5 into dotnet:master Nov 11, 2016
@DustinCampbell DustinCampbell deleted the mono-work branch January 31, 2017 20:45
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.

5 participants