-
Notifications
You must be signed in to change notification settings - Fork 678
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
Download and install OmniSharp during extension activation if necessary #106
Conversation
function getOmnisharpAssetName(): string { | ||
switch (process.platform) { | ||
case 'win32': | ||
return 'omnisharp-win-x64-dnx451.zip'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question - I noticed that currently these zip files contain a full .NET Framework. Do we want to do something before RC2 so that we share these things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this specifically about the dnx451 version we're using on Windows? The idea there is to continue to allow MSBuild support on Windows while dropping it for OS X and Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I was looking at one of the gz's (OSX I think) I noticed that them seem to carry a full CoreCLR in them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but the OmniSharp that we use may be on a completely different build. We could do work to share, but I'm not sure it's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will likely be making a "portable" OmniSharp that runs on the shared runtime. However, that will pre-req having the .NET CLI just to get OmniSharp to run, which might not be desirable. On Windows, VS Code will work for csprojs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah our worry, because we've been bitten by it before with dnx, is that without a standalone runtime to boot on top of, we have to do more work from an editor perspective to get things up and running.
- Check for the CLI
- What locations are "standard"
- Is there an environment variable to get those locations? (dnx had one)
- What cli versions are installed?
- What cli version do we need?
- Do we trust new cli's?
- Minor versions only?
If we can't find the cli then we have to download that, then download the OmniSharp package. It's just a whole ton more work, than just running OmniSharp.exe
. Hence why we package them as standalone copies.
As I noted on our slack, I'm not adverse to having a portable
version kicking around, and the plan is to add a new portable drop, but for anyway now we're going to keep the standalone copies.
👍 |
Download and install OmniSharp during extension activation if necessary
I went ahead and merged so that folks can rebase against it for other PRs. But please feel free to continue the discussion about the zips. |
Thanks for the tag - I need to get more into this stuff. |
cc @chuckries and @gregg-miskelly. Also, @Pilchie if he's interested.