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 instructions for use with .NET 6.0 #4205

Merged
merged 3 commits into from
Dec 28, 2023
Merged

Conversation

dkaszews
Copy link
Contributor

@dkaszews dkaszews commented Dec 10, 2023

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

  • I have read and understood YCM's CONTRIBUTING document.
  • I have read and understood YCM's CODE_OF_CONDUCT document.
  • I have included tests for the changes in my PR. If not, I have included a
    rationale for why I haven't.
  • I understand my PR may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Why this change is necessary and useful

Adds instructions for using YCM with .NET 6.0 and newer, as it is currently not supported out of the box and configuring it has difficult to diagnose gotchas. Switching defaults to .NET 6.0 version of OmniSharp-Roslyn for out-of-the-box support breaks a bunch of tests in ycmd, lacking resources to fix.

Removal of necessisity for solution files is possible, but requires some future work as YCM internally associates files with their solutions and therefore OmniSharp server instances; removing them causes issues.

After merge, add the following to FAQ linking to the new section:

Q: My C# projects compile, but YCM shows false-positive syntax errors
A: You may be trying to use features from .NET 6.0 or newer, which is not supported out of the box, see README

Q: My C# projects compile without solution files, but YCM just shows "Missing solution file":
A: YCM currently uses solution files for internal bookkeeping, see README

Q: YCM only provides autocomplete for C# system modules, but not for any nuget dependencies
A: Your solution file may be missing references to some projects, see README

Q: YCM shows my C# file is fine, but it fails to actually compile
A: Your solution file may be missing references to some projects, see README

No tests - documentation only.


This change is Reviewable

@dkaszews dkaszews marked this pull request as draft December 10, 2023 20:20
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Thanks.

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dkaszews)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dkaszews)

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Merging #4205 (79e3753) into master (302d41e) will increase coverage by 1.07%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4205      +/-   ##
==========================================
+ Coverage   88.82%   89.90%   +1.07%     
==========================================
  Files          34       34              
  Lines        4404     4408       +4     
==========================================
+ Hits         3912     3963      +51     
+ Misses        492      445      -47     

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Finally got time to review stuff!

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dkaszews)


README.md line 1409 at r1 (raw file):

    1. Run `dotnet sln add <project1.csproj> <project2.csproj> ...`
    for all of your projects
1. Run `:YcmRestartServer`

I'm going to nitpick a little.

Having :YcmRestartServer at the end implies, at least to me, that you can switch to .NET 6.0 without restarting vim. However, changing g:ycm_roslyn_binary_path will require a restart. We only read the global config once.

Suggestions:

  • For step 2, suggest putting g:ycm_roslyn_binary_path into vimrc. You need not, but then you need to use vim --cmd 'let g:ycm_roslyn_binary_path=whatever', which is clumsy.
  • Either drop the last step or suggest a complete vim restart.

Copy link
Contributor Author

@dkaszews dkaszews left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)


README.md line 1409 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I'm going to nitpick a little.

Having :YcmRestartServer at the end implies, at least to me, that you can switch to .NET 6.0 without restarting vim. However, changing g:ycm_roslyn_binary_path will require a restart. We only read the global config once.

Suggestions:

  • For step 2, suggest putting g:ycm_roslyn_binary_path into vimrc. You need not, but then you need to use vim --cmd 'let g:ycm_roslyn_binary_path=whatever', which is clumsy.
  • Either drop the last step or suggest a complete vim restart.

Are you sure about that? Because I have checked the instructions I wrote and at least the option in question is reloaded by :YcmRestartServer:

:YcmDebugInfo
--   OmniSharp executable: /usr/bin/mono /home/dkaszews/.local/share/nvim/plugged/YouCompleteMe/third_party/ycmd/ycmd/completers/cs/../../../third_party/omnisharp-roslyn/omnisharp/OmniSharp.exe -p 45591 -s /home/dkaszews/.local/share/nvim/plugged/YouCompleteMe/third_party/ycmd/ycmd/tests/cs/testdata/testy/testy.sln

:let g:ycm_roslyn_binary_path = '/home/dkaszews/code/omnisharp/omnisharp.http-linux-x64-net6.0/OmniSharp'
:YcmRestartServer
:YcmDebugInfo
--   OmniSharp executable: /home/dkaszews/code/omnisharp/omnisharp.http-linux-x64-net6.0/OmniSharp -p 59517 -s /home/dkaszews/.local/share/nvim/plugged/YouCompleteMe/third_party/ycmd/ycmd/tests/cs/testdata/testy/testy.sln

@dkaszews dkaszews marked this pull request as ready for review December 27, 2023 19:46
Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

:lgtm:

This pull request updates the ycmd submodule. Here's the ycmd changelog:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @dkaszews)


README.md line 1409 at r1 (raw file):

Are you sure about that?

Obviously I'm not. You were right and this change is fine.

@puremourning
Copy link
Member

Vim tests now failing?

@dkaszews
Copy link
Contributor Author

Don't really know how to read the logs, which errors are expected and which not, but it kinda looks like the error is in a C++ test around line 34000, so unlikely to be caused by my most recent changes.

Easiest diagnostic step would be to trigger tests for each of the new ycmd commits, can't do it myself since this is my first commit to this repo.

@puremourning
Copy link
Member

Looks like a slight change in the diagnostics output :

command line..script /__w/YouCompleteMe/YouCompleteMe/test/lib/run_test.vim[402]..function RunTheTest[68]..Test_ShowDetailedDiagnostic_CmdLine line 9: Expected 'Format specifies type ''char *'' but the argument has type ''int'' (fix available)' but got 'Format specifies type ''char *'' but the argument has type ''int'' (fix available) [-Wformat]'

Likely caused by:

ycm-core/ycmd#1720 - Provide diagnostic kind in detailed diagnostic.

@puremourning
Copy link
Member

pushed a fix

@puremourning
Copy link
Member

OK it seems the problems are more deep rooted!

@puremourning
Copy link
Member

ok, fixed a ton more issues

@puremourning puremourning force-pushed the readme-net6 branch 2 times, most recently from 233d54d to 3cd2228 Compare December 28, 2023 11:29
dkaszews and others added 3 commits December 28, 2023 11:32
Broken by ycm-core/ycmd#1720 - Add diagnostic type to output
Inexplicable syntax errors
Missing semantic_highlighting groups lead to unexpected output
Improve diagnostic test failure messages - print the popup positions
@bstaletic
Copy link
Collaborator

Looks like Test_Completion_FixIt() is still broken...

Found errors in Test_Completion_FixIt(). Retrying.
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check1[3]..CheckCompletionItemsHasItems line 15: Didn't find item with abbr="do_a_thing(Thing thing)" in completion list: ['std::memory_order_acquire', 'std::memory_order_acq_rel', 'std::atomic_thread_fence(memory_order m)', 'std::memory_order_relaxed', 'std::memory_order_release']
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check1[3]..CheckCompletionItemsHasItems line 15: Didn't find item with abbr="do_another_thing()" in completion list: ['std::memory_order_acquire', 'std::memory_order_acq_rel', 'std::atomic_thread_fence(memory_order m)', 'std::memory_order_relaxed', 'std::memory_order_release']
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check2[2]..CheckCurrentLine line 1: Expected 'do_a_thing' but got 'std::memory_order_acquire'
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check2[3]..CheckCompletionItemsHasItems line 15: Didn't find item with abbr="do_a_thing(Thing thing)" in completion list: ['std::memory_order_acquire', 'std::memory_order_acq_rel', 'std::atomic_thread_fence(memory_order m)', 'std::memory_order_relaxed', 'std::memory_order_release']
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check2[3]..CheckCompletionItemsHasItems line 15: Didn't find item with abbr="do_another_thing()" in completion list: ['std::memory_order_acquire', 'std::memory_order_acq_rel', 'std::atomic_thread_fence(memory_order m)', 'std::memory_order_relaxed', 'std::memory_order_release']
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check3[2]..CheckCurrentLine line 1: Expected 'do_a_thing(' but got 'std::memory_order_acquire('
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check3 line 3: Expected '#include "auto_include.h"' but got '#include <atomic>'

@puremourning
Copy link
Member

Looks like Test_Completion_FixIt() is still broken...

Found errors in Test_Completion_FixIt(). Retrying.
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check1[3]..CheckCompletionItemsHasItems line 15: Didn't find item with abbr="do_a_thing(Thing thing)" in completion list: ['std::memory_order_acquire', 'std::memory_order_acq_rel', 'std::atomic_thread_fence(memory_order m)', 'std::memory_order_relaxed', 'std::memory_order_release']
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check1[3]..CheckCompletionItemsHasItems line 15: Didn't find item with abbr="do_another_thing()" in completion list: ['std::memory_order_acquire', 'std::memory_order_acq_rel', 'std::atomic_thread_fence(memory_order m)', 'std::memory_order_relaxed', 'std::memory_order_release']
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check2[2]..CheckCurrentLine line 1: Expected 'do_a_thing' but got 'std::memory_order_acquire'
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check2[3]..CheckCompletionItemsHasItems line 15: Didn't find item with abbr="do_a_thing(Thing thing)" in completion list: ['std::memory_order_acquire', 'std::memory_order_acq_rel', 'std::atomic_thread_fence(memory_order m)', 'std::memory_order_relaxed', 'std::memory_order_release']
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check2[3]..CheckCompletionItemsHasItems line 15: Didn't find item with abbr="do_another_thing()" in completion list: ['std::memory_order_acquire', 'std::memory_order_acq_rel', 'std::atomic_thread_fence(memory_order m)', 'std::memory_order_relaxed', 'std::memory_order_release']
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check3[2]..CheckCurrentLine line 1: Expected 'do_a_thing(' but got 'std::memory_order_acquire('
function RunTheTest[68]..Test_Completion_FixIt[36]..FeedAndCheckMain[2]..Check3 line 3: Expected '#include "auto_include.h"' but got '#include <atomic>'

but.. only on old vim ._0

@bstaletic
Copy link
Collaborator

Green as well!

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 1 of 3 files at r4, 3 of 3 files at r7, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @dkaszews)

@puremourning puremourning added the Ship It! Manual override to merge a PR by maintainer label Dec 28, 2023
Copy link
Contributor

mergify bot commented Dec 28, 2023

Thanks for sending a PR!

@mergify mergify bot merged commit ae07211 into ycm-core:master Dec 28, 2023
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants