Omnisharp roslyn #213
Omnisharp roslyn #213
Conversation
|
Why the double backend? According to @nosami omnisharp-roslyn should be better then the one we use currently, am I missing something? |
|
I tried it and it is not really usable in its current state. There are too much completion timeouts. Maybe we are doing something wrong. |
|
Yes, that's my experience too. Maybe it would be faster with the new stdio protocol, but that would be a considerable implementation effort. The legacy omnisharp doesn't work on new-style projects, which is a use case for using this new roslyn omnisharp backend. But in general, the performance is not (yet?) there to use exclusively. @nosami Are we doing something wrong here? |
|
In my experience, it's marginally faster than the old server. Are you fetching documentation on each request? That is known to be slow. There is caching on the server to make subsequent requests faster. |
|
I'm making the exact same request. WantDocumentationForEveryCompletionResult set to true, and ForceSemanticCompletion and WantImportableTypes set to false. Maybe it's because omnisharp-roslyn doesn't seem to support either ForceSemanticCompletion or WantImportableTypes? |
|
Try |
|
|
|
Ah I see. I was thinking of something else sorry. In that case the server will act as if that flag was set to true. |
|
Thanks for sending out the PR! I'd rather wait until we can fully switch over to omnisharp-roslyn than have a switchable implementation. |
mispencer
force-pushed
the
OmnisharpRoslyn
branch
from
7c27dad
to
d16f6b4
Compare
|
☔ The latest upstream changes (presumably #226) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Yay, green checkmark from Travis. I assume I can ignore the AppVeyor failure. Ok, so I gross violated the rule of small pull requests. Justification: Travis is a jerk To achieve good performance, we need to use the stdio interface of omnisharp-roslyn. This vastly complicated the C# completer. Additionally, roslyn did not play nicely with the legacy Travis, which necessitated switching to the new Travis infrastructure. Regarding the switchable implementation, as you can see by all the tests I skipped while under it, omnisharp-roslyn is still not a feature complete replacement for OmnisharpServer. Yet, OmnisharpServer doesn't work on the new json-based C# project. So, where possible, I would prefer to use OmnisharpServer, but I still need omnisharp-roslyn available for new projects. |
| self._logger.info( "Omnisharp "+type+": " + line.rstrip() ) | ||
| except Exception: | ||
| self._logger.error( "Read error: " + traceback.format_exc() ) | ||
| #self._logger.info( "Log " + type + " Out done" ) |
There was a problem hiding this comment.
You probably left this by accident.
Yes, the breakage is expected, not everything is fully set up for it.
Oh boy, did you ever. :) I'm going to need help reviewing this monster. @vheon @micbou @puremourning
I think the switchable implementation might be ok if it's temporary. If work is happening upstream in Omnisharpland to get to feature parity and we might get there within 6 months to a year at most, then maybe. But I don't want to continue supporting both configurations. Also, we should now or some time soon declare (in the docs) the legacy Omnisharp support deprecated and on the way out. I'm interested in what the rest of the YCM crew think of this. |
|
☔ The latest upstream changes (presumably #232) made this pull request unmergeable. Please resolve the merge conflicts. |
Did it worked? Isn't necessary to ask Travis to abilitate the repo for the new infrastructure? @Valloric did you ask for it in preparation for OSX testing?
How many feature are missing? maybe @nosami could shed some light on the roadmap for them or tell us if we're doing something wrong. |
| @@ -215,7 +236,10 @@ def OnUserCommand( self, arguments, request_data ): | |||
| raise ValueError( self.UserCommandsHelpMessage() ) | |||
|
|
|||
| command = arguments[ 0 ] | |||
| if command in CsharpSolutionCompleter.subcommands: | |||
| del arguments[ 0 ] | |||
There was a problem hiding this comment.
can we use command = arguments.pop( 0 )?
|
Roslyn is implementing a public intellisense API that should be ready any time now. It will be the exact same API that is used by Visual Studio. Currently, we are using the Roslyn Recommender API which isn't as fully featured yet as either VS or the older NRefactory API. Like I said earlier though, fetching documentation for every single item in the completion list is always going to be slow on the first hit - there can be multiple sources for docs. It's much faster to fetch docs for an individual item on demand .. although you're probably constrained by the way that Vim does completion. |
@nosami so the API is going to change from the current one? If so we should wait... |
|
@vheon The OmniSharp API won't change, but the Roslyn API underneath it will. edit - The OmniSharp API is almost the same now with the Roslyn engine as the old NRefactory powered version (omnisharp-server). We try very hard not to break existing consumers. |
|
@nosami ok thanks. At this point though, if the roslyn work
I would wait and see if we could use just the Roslyn version. |
|
@mispencer What are your thoughts given the information @nosami presented? Review status: 0 of 13 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. Comments from the review on Reviewable.io |
|
@nosami The issue was the http interface was slow. When I switched to the stdio interface, the performance issues went away. @vheon No, I didn’t have to do anything to use the new container infrastructure for travis. I briefly had it building on OS X, and that didn’t require me to request access either. The only missing feature in omnisharp-roslyn that blocking switching to it for everything, for me, is the lack of support for WantImportableTypes. @Valloric I don’t urgently need this for anything. However, I’ve already had to fix merge conflicts twice, and I would prefer not have to do it again… Review status: 0 of 13 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending. ycmd/completers/cs/cs_completer.py, line 239 [r5] (raw file): ycmd/completers/cs/cs_completer.py, line 620 [r5] (raw file): ycmd/completers/cs/cs_completer.py, line 723 [r5] (raw file): Comments from the review on Reviewable.io |
|
@mispencer Sorry about the merge conflicts, that sounds painful. We need to keep the bigger picture in mind though; if @nosami says that the new API should be ready "any time now," it might be best to just hold off on this PR until then. We could then just fully switch to the new implementation instead of supporting switchable confs. So let's wait for now. And here's a cut-off date so it's not indeterminate: if by December of this year the new intellisense API in omnisharp is not ready to the point that we can fully switch to it, then let's go with the switchable approach. Review status: 0 of 13 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. Comments from the review on Reviewable.io |
|
☔ The latest upstream changes (presumably #240) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Well, I have resolved the merge conflict and updated to the latest OmnisharpRoslyn. However, OmnisharpRoslyn doesn't seem to build without errors in either Windows or OS X, so I guess this is still on hold. It also seems a little flakey. |
|
@mispencer did you tried it on your machine? Are the performance issues for the http interface resolved? |
|
@mispencer There's been a lot of work on omnisharp-roslyn recently (see the DEV branch) to work on Windows/OS X/Linux with the new .NET CLI bits. And that issue that you mentioned that you're blocked by has now been closed. There's also been a new OmniSharp Slack team that has been created, and everyone is free to invite themselves into. Just visit the omnisharp-roslyn wiki if you would like an invite. |
mispencer
force-pushed
the
OmnisharpRoslyn
branch
4 times, most recently
from
69e6b21
to
7c15099
Compare
|
☔ The latest upstream changes (presumably #465) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@mispencer What's the state of this PR now? @willl mentioned blockers being fixed. |
|
I have updated to the latest release, but I am still having problems.
|
mispencer
force-pushed
the
OmnisharpRoslyn
branch
from
e8f2d36
to
507be75
Compare
|
☔ The latest upstream changes (presumably #506) made this pull request unmergeable. Please resolve the merge conflicts. |
mispencer
force-pushed
the
OmnisharpRoslyn
branch
from
507be75
to
3967ee1
Compare
|
☔ The latest upstream changes (presumably #601) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Is this PR now abandoned? We probably need to migrate to the roslyn backend at some point, and the issues @mispencer encountered might not be present anymore. |
|
I attempted to get this working a few months ago without any more success. |
|
@mispencer: 🔑 Insufficient privileges: Collaborator required |
|
@mispencer: 🔑 Insufficient privileges: and not in try users |
|
@mispencer Thanks for the heads-up. Also, ignore zzbot, it's misbehaving. |
|
Let's close this PR then since it's not making progress. |
|
@mispencer Is it omnisharp-roslyn related issues you're having? If so, perhaps jump into their slack team and see if you could get some help. There is work underway for omnisharp-roslyn to support the language server protocol natively (also see), there are also plans underway for the server to run on top of a embedded copy of mono. The move to embedded mono should help resolve a lot of the runtime related issues. And LSP support may benefit if you if this project were an LSP client. I don't know how far off these things are but I'm sure they will probably help in the long run when they're ready. |
We will eventually have support for consuming LSP APIs. There's work being done on this already. |
|
Could this PR be re-opened? I have had a need for this new backend, so I have been working on getting this working. |
|
@mispencer You'll have to open a new pull request. "Reopen and comment is disabled because "The OmnisharpPoslyn" branch was force-pushed or recreated." |
Allow using omnisharp-roslyn. See #209 (comment).
Omnisharp-roslyn tests do not currently pass on my system, and I'm still receiving a path too long error. Once it is built, it seems to work, though it seems slow.
This pull request does not make roslyn the default (yet). Rather, it must be switched to the roslyn implementation by running the
UseRoslynOmnisharpcompleter subcommand. This is per-solution, and pre-runtime.