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

Move OmniSharp to target net46 only #915

Merged
merged 12 commits into from
Aug 7, 2017

Conversation

DustinCampbell
Copy link
Contributor

@DustinCampbell DustinCampbell commented Jul 14, 2017

Fixes #666

Tagging @david-driscoll and @filipw specifically for a review since this is a significant change. It'd be great if anyone looking at this could build this branch and try it out.

This PR moves OmniSharp to target .NET Framework 4.6 exclusively and removes all of the netcoreapp1.1 builds. With this change, there are now only six flavors of OmniSharp that will be released:

  • Windows builds that run on Desktop CLR.
    • omnisharp-win-x86.zip
    • omnisharp-win-x64.zip
  • A *Nix build that be run on Mono 5.2.0 or greater. (Note that the --assembly-loader=strict flag must be specified when launch this build with Mono).
    • omnisharp-mono.tar.gz
  • Standalone builds for OSX and Linux that include the Mono bits necessary to run OmniSharp.
    • omnisharp-osx.tar.gz
    • omnisharp-linux-x86.tar.gz
    • omnisharp-linux-x64.tar.gz

Important note This change does not affect the project types that OmniSharp supports. OmnISharp still supports .NET Core and .NET Framework, and Xamarin, Unity and other Mono-based projects.

For discussion: I chose to not include libuv.dll in the *Nix builds, but left them in the Windows builds. Allow me to explain my reasoning:

  1. libuv.dll is used by Kestrel, and is only needed when using OmniSharp is an HTTP server. When using OmniSharp with stdio, it isn't necessary at all.
  2. libuv.dll is a platform-specific native dependency. So, it wouldn't be possible to include it in the omnisharp-mono package anyway.
  3. It is mind-numbingly easy to install on *Nix (e.g. brew install libuv or sudo apt-get install libuv1). It is not as easy to acquire on Windows however.

I'm open to discussion on this decision.

@bjorkstromm
Copy link
Member

@DustinCampbell will you not support .NET Core in the future? Is it Full Framework or nothing?

@DustinCampbell
Copy link
Contributor Author

@mholo65:

will you not support .NET Core in the future? Is it Full Framework or nothing?

There two aspects here:

  1. What runtime OmniSharp itself runs on.
  2. What sorts of projects OmniSharp can handle

This PR is only about the first point above. OmniSharp will no longer provide stand-alone builds that run on .NET Core. Primarily, this is because those builds are limited in what sorts of projects they can process. (i.e. MSBuild tasks targeting Full Framework will likely not run on CoreCLR). For OSX/Linux, we're going to be bundling a local Mono runtime so that OmniSharp will still run as a standalone application, without requiring Mono to be installed on the system (see #666 for details).

The second point above is not changing at all. OmniSharp will continue to support processing both .NET Core and Full Framework projects.

Does that answer your question?

@DustinCampbell
Copy link
Contributor Author

Note that this will just be formalizing what we already do in C# for VS Code to run OmniSharp.

@bjorkstromm
Copy link
Member

@DustinCampbell yes! Thanks, this is good news for us in Cake team, now we only need to target full framework for the Cake-plugin!

@DustinCampbell DustinCampbell force-pushed the net46-only branch 2 times, most recently from 812aec7 to 6ebf09f Compare July 20, 2017 20:15
@DustinCampbell
Copy link
Contributor Author

OK. Builds are passing. Tests are still somewhat problematic but they're working at the moment. Next up: packaging.

@DustinCampbell
Copy link
Contributor Author

@david-driscoll & @filipw: I added you two in case you want to start looking at this or would like to try the branch out.

@DustinCampbell
Copy link
Contributor Author

Note: This change does not move our libraries over to netstandard2.0. I think that's possibly the next thing, but it's not needed yet.

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

I'm all for it!

I'm very much eager to try this too.
For example it will help tackle some edge cases in scripting due to how we need to add references to runtime assemblies rather than (as you'd normally expect) to compile assemblies, which can be problematic when running OmniSharp on .NET Core.

@filipw
Copy link
Member

filipw commented Jul 22, 2017

We also have some conditional compilation directives which could be dropped now i.e. here or here

@DustinCampbell
Copy link
Contributor Author

OK. I'm starting over a bit. I've rebased onto master with the new updates to the build.

@DustinCampbell DustinCampbell changed the title [NO MERGE] Convert projects to net46 and get Windows build working [NO MERGE] Move OmniSharp to target net46 only Jul 24, 2017
@bjorkstromm
Copy link
Member

@DustinCampbell any plans on updating target framework for OmniSharp.exe to net462 or net47? Or will this be done once netstandard2.0 is released? We are updating target frameworks for Cake in the next version and want to make sure Cake support (#932) still works as we need to load Cake.Core into OmniSharp in order to get the hostObjectType for scripting (https://github.com/OmniSharp/omnisharp-roslyn/pull/932/files#diff-3f2a987ebd93be29eeb71b6f6d923decR151)

@DustinCampbell
Copy link
Contributor Author

No plans yet. Keeping the target framework it at 4.6 means that it will work on more Windows versions without the user needing to do anything.

@DustinCampbell DustinCampbell changed the title [NO MERGE] Move OmniSharp to target net46 only Move OmniSharp to target net46 only Aug 4, 2017
@eamodio
Copy link

eamodio commented Aug 4, 2017

@DustinCampbell does this mean we won't be able to debug omnisharp on macOS/Linux (via vscode)?

@DustinCampbell
Copy link
Contributor Author

I would expect the Mono Debugger extension to work, though I've never tried it.

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.

LGTM. Do we have a plan to handle new mono versions, some repo or script we can setup to manage that?

@DustinCampbell
Copy link
Contributor Author

Do we have a plan to handle new mono versions, some repo or script we can setup to manage that?

Not yet. We're just using the bits that I regularly cobble together for C# for VS Code. I have a script that I use to copy the bits out of an installation of Mono on OSX. However, for Linux x86/x64, I have VMs where I sudo apt-get the latest mono and copy the runtimes out.

@DustinCampbell
Copy link
Contributor Author

Are you guys OK if I merge this? It's a big change, so I want to be sure.

@filipw
Copy link
Member

filipw commented Aug 7, 2017

absolutely, I can't wait to get rid of .NET Core 😃🏆🍻

@DustinCampbell
Copy link
Contributor Author

OK. Here it goes. 😄

@danmoseley
Copy link

@joperezr

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.

None yet

6 participants