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

Add omnisharp language server #291

Merged
merged 5 commits into from
Aug 30, 2020
Merged

Add omnisharp language server #291

merged 5 commits into from
Aug 30, 2020

Conversation

damienpontifex
Copy link
Contributor

Getting inspiration from https://github.com/coc-extensions/coc-omnisharp and other lsp sources here to add omnisharp lsp to the available language servers

@damienpontifex
Copy link
Contributor Author

Build is failing for macOS at "Setup build dependencies".

I also noticed it failed on another PR in the last week so maybe unrelated to this work specifically? But happy to fix if needed to make PR go through if there's some insights into what the root issue is there

@adelarsq
Copy link
Contributor

@damienpontifex Did you manage to make it work? My main blocker to submit a pull request is the root_dir = util.root_pattern(".csproj", ".sln", ".git");, that don't find the root dir.

@Iron-E
Copy link
Contributor

Iron-E commented Jul 2, 2020

I'll echo @adelarsq's comments about the root_dir configuration (he and I have briefly spoken about this before). I've been playing with it for a couple of days now to no avail. I linked to this PR from another issue where I have a very similar configuration to the proposed default and it is not working for me because of the root_dir.

@adelarsq
Copy link
Contributor

adelarsq commented Jul 2, 2020

I did find the reason. root_pattern use the pattern to match the whole file name on the directory. So for a pattern like .csproj will try to find a file with name .csproj, not with a pattern like *.csproj.

For now my workaround is to use like that:

root_dir = util.root_pattern("Makefile");

Just put a Makefile there and life goes on.

A better approach is to make root_pattern accept patterns, but I still didn't have time to create a solution.

@damienpontifex
Copy link
Contributor Author

I believe I did have it working, but testing this morning I agree with issues here. I'll have another look over the weekend

@Iron-E
Copy link
Contributor

Iron-E commented Jul 3, 2020

I guess if it's matching exact patterns, we can use nvim_lsp.util.find_git_ancestor(fname) (as shown here) to attempt to find a git project as a best-case scenario (since at least for me, most of my C# projects are maintained by git).

Edit: this does not work. Even though I'm using code from the examples in the readme:

['root_dir'] = function(fname)
	return nvim_lsp.util.find_git_ancestor(fname) or nvim_lsp.util.root_pattern('.sln') or vim.loop.os_homedir()
end,

I get this error:

Error executing vim.schedule lua callback: Vim(lua):E5108: Error executing lua ....local/share/nvim/plugged/nvim-lsp/lua/nvim_lsp/util.lua:131: bad argument
#1 to 'fs_stat' (string expected, got function)

So I don't think we can count on that.

@adelarsq
Copy link
Contributor

adelarsq commented Jul 3, 2020

Not always the .git path is the right, since a repository can have many projects.

Also for me don't really solve, since I use with subversion... yes, sad!

I'm thinking about ways to resolve the pattern, so we solve this problem for others language servers (like for F#).

I did see some configs using getcwd(), but this don't really solve the problem, since we are hoping that some plugin will solve for us or need to set manually.

@Iron-E
Copy link
Contributor

Iron-E commented Jul 3, 2020

Yeah, you're right… didn't think of that. I was intrigued by what you said about the pattern matching and I started looking at the code for this repository. It seems that the pattern matching occurs around these lines:

Unfortunately the vim.loop module (which is what they are using) doesn't have great documentation in the :help files. It does refer to here and here but I haven't had time to look at those yet. Perhaps there's a regex or globbing function somewhere in there?

It might take some additions to nvim-lsp in another PR to get this functionality going


Hmm... I can verify that by using find_git_ancestor that nvim_lsp is finding the right root directory. However, the client still seems to be shutting down.

Try this:

['root_dir'] = function(fname)
	local path = nvim_lsp.util.find_git_ancestor(fname) or nvim_lsp.util.root_pattern('.sln') or vim.loop.os_homedir()
	vim.cmd('split foo | normal i'..path)
	return path
end,

@adelarsq
Copy link
Contributor

adelarsq commented Jul 3, 2020

The problem is with M.path.join(path, pattern) that just make a concatenation with path and pattern.

Perhaps there's a regex or globbing function somewhere in there?

I don't think. I was search for one but didn't find any.

It might take some additions to nvim-lsp in another PR to get this functionality going

I will do that on this weekend (a correction for the M.root_pattern). I also need to send a request for F# language server that I'm using.

@adelarsq
Copy link
Contributor

adelarsq commented Jul 4, 2020

@Iron-E I did create a pull request with a solution for the root_pattern. I did some tests with util.root_pattern("*.sln") and works as expected. Can you do some test?

See #296

@Iron-E
Copy link
Contributor

Iron-E commented Jul 4, 2020

I can verify that using nvim_lsp.util.root_pattern('*.sln') causes the nvim-lsp to find the correct root directory. On top of that, the server starts up and things seem to be working.

A couple of strange things are happening though:

  • diagnostic-nvim isn't updating the onscreen diagnostics.
    • e.g. remove a semicolon, an error pops up, add it back, the error stays.
  • The Lsp* highlight groups are reset back to their defaults.
    • This isn't too much of a problem, I can just make an after/ftplugin file to resource my colorscheme. It is weird that it's happening though.

Are these things happening for you too?

@adelarsq
Copy link
Contributor

adelarsq commented Jul 4, 2020

diagnostic-nvim isn't updating the onscreen diagnostics.

Also did notice this days ago (I was using a Makefile to set the root path). Sometimes works, sometimes not. I think that is something with the language server it self.

The Lsp* highlight groups are reset back to their defaults.

Didn't notice this. I have only LspDiagnostics*. Are there anothers?

@damienpontifex damienpontifex marked this pull request as draft July 4, 2020 04:47
@damienpontifex
Copy link
Contributor Author

I was having a play around with the root_pattern function (second commit here) and have it working with a new console app dotnet new console.

I've also converted PR to draft as this is a bit more far reaching than my initial change and I'm in no near way do I have strong knowledge in lua or the impact on other packages so trying things out to make it work

@Iron-E
Copy link
Contributor

Iron-E commented Jul 4, 2020

Yeah, the LspDiagnostics* are the ones that reset. If you execute colorscheme before running .setup{} on all of your servers, it resets the highlight groups. After doing more testing I think this is an issue with nvim-lsp though, not OmniSharp.

@damienpontifex
Copy link
Contributor Author

I missed your other PR @adelarsq (#296) .... seems to be very similar idea as what I did, but I'm very happy to defer to your change there if it's good and rebase this onto that to pick it up :)

@adelarsq
Copy link
Contributor

adelarsq commented Jul 4, 2020

Thanks @damienpontifex. I did some tests with some language servers and its working. I have some issues with Elm language server but isn't related. Did some tests with the previous source code and the same issues occurs.

@adelarsq
Copy link
Contributor

adelarsq commented Jul 4, 2020

Tomorrow I will make some more tests with Java and Elixir language servers. Its 3 AM and I need to sleep 😄

@Iron-E
Copy link
Contributor

Iron-E commented Jul 4, 2020

Someone else is having the same issues that we are with the diagnostics not updating correctly. It seems like it may not be diagnostic-nvims problem, since it's happening with ALE too.

@adelarsq
Copy link
Contributor

adelarsq commented Jul 7, 2020

@damienpontifex I just did replace my omnisharp config (just the omnisharp.lua file) by yours and notice that is missing a extracted_dir on the path local install_dir = util.path.join{util.base_install_dir, server_name}. Just change that and will work.

My diagnostics stopped to work completely for omnisharp (not by the reason above). It's fine for others LSPs implementations but not for C#. Was because a huge code base that I was refactoring (8 millions lines).

I just have finded why its failing. Neovim don't close the language server on exit:

adelar           16091 100,0  3,3  4616552 278540 s008  R     4:39     0:29.43 /Users/adelar/.cache/nvim/nvim_lsp/omnisharp/bin/mono /Users/adelar/.cache/nvim/nvim_lsp/omnisharp/omnisharp/OmniSharp.exe --languageserver --verbose
adelar           15689 100,0  2,6  4572244 215056 s008  R     4:38     0:31.49 /Users/adelar/.cache/nvim/nvim_lsp/omnisharp/bin/mono /Users/adelar/.cache/nvim/nvim_lsp/omnisharp/omnisharp/OmniSharp.exe --languageserver --verbose

@svermeulen
Copy link
Contributor

Thanks for your work here, I was able to get this working on macos, except that I had to change the line extracted_dir = 'darwin' to just extracted_dir = ''

Also, as others have mentioned, diagnostics doesn't work and the omnisharp servers seem to persist indefinitely, but otherwise it's working well

@adelarsq
Copy link
Contributor

adelarsq commented Jul 9, 2020

Also, as others have mentioned, diagnostics doesn't work and the omnisharp servers seem to persist indefinitely, but otherwise it's working well

Yes. Its pretty strange how diagnostics behaves. For me sometimes do works, sometimes not. But it's helping on daily work at least. Better performance then others options.

@Iron-E
Copy link
Contributor

Iron-E commented Jul 10, 2020

I've noted a temporary workaround for some of the diagnostic-related bugs here for any that are using this in the meantime.

@svermeulen
Copy link
Contributor

svermeulen commented Jul 13, 2020

What has worked well for me (so far) was to add this change to the roslyn server and this change to diagnostic-nvim.

Update: After more digging, the former change to the roslyn server seems like a really bad idea

@damienpontifex
Copy link
Contributor Author

Sorry I haven't had the time to pick this up again due to work - if there are fixes and someone is happy to add them and create another PR with those fixes? Sorry again

@adelarsq
Copy link
Contributor

@damienpontifex no problem. I can create another and link this as source. I will create tomorrow since today is too late. 😴

@damienpontifex damienpontifex marked this pull request as ready for review July 25, 2020 03:59
@damienpontifex
Copy link
Contributor Author

@adelarsq I updated the few path issues and grabbed the code of yours from #296 to make this work (pending that being merged)
I've tested a few teams starting with removing omnisharp folder under ~/.cache/nvim/nvim_lsp and just a sample console app and I'm getting completion with Console.Writ as expected. When selecting the completion, it completes the full Console.WriteLine(); and puts the cursor after the semi colon which is weird, but maybe something as part of what omnisharp is providing vs the LSP setup here...

@adelarsq
Copy link
Contributor

@damienpontifex Thanks! I was planning to work on this this weekend.

I've tested a few teams starting with removing omnisharp folder under ~/.cache/nvim/nvim_lsp and just a sample console app and I'm getting completion with Console.Writ as expected. When selecting the completion, it completes the full Console.WriteLine(); and puts the cursor after the semi colon which is weird, but maybe something as part of what omnisharp is providing vs the LSP setup here...

Yes. I think so.

@Iron-E
Copy link
Contributor

Iron-E commented Jul 31, 2020

The OmniSharp LSP has been working well on my machine recently. Anyone else?

@adelarsq
Copy link
Contributor

@Iron-E I've been using daily and it's working well. Just notice some issues with diagnostics, but I think that is because I'm working with a huge code base (8 millions of lines).

@felschr
Copy link

felschr commented Aug 27, 2020

It's working great for me.

@adelarsq
Copy link
Contributor

adelarsq commented Aug 27, 2020

Did notice an issue with it on macOS. After to download the LSP need to change permissions for the run file to be able to start the server:

chmod +x ~/.cache/nvim/nvim_lsp/omnisharp/run

EDITED:
@damienpontifex Can you add this step for macOS and Linux? After this change I think that it's good to go.

@justinmk justinmk merged commit 28a3982 into neovim:master Aug 30, 2020
@damienpontifex
Copy link
Contributor Author

@adelarsq we'll have to create another PR with the small fix for chmod now that this one is merged

@damienpontifex damienpontifex deleted the omnisharp branch August 31, 2020 00:36
@justinmk
Copy link
Member

note that in #334 i've proposed removing/deprecating the "install" routines in this repo, instead favoring documentation of install steps.

@adelarsq
Copy link
Contributor

Nice! Thanks @justinmk

note that in #334 i've proposed removing/deprecating the "install" routines in this repo, instead favoring documentation of install steps.

I think that is a nice move. Would be a lot easier to maintain.

@damienpontifex So let's wait for #334 so you add this step on the documentation.

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.

6 participants