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

Receive information that a project was loaded #849

Closed

Conversation

Melandel
Copy link
Contributor

💌 Motivation: OmniSharpReloadProject would otherwise throw because it does not find key job.projects_loaded

In order to fill this property, I tried to figure out why it did not exist. Turns out that on my codebases (dotnet6 maybe?) don't detect loaded project events.

echomsg-based debugging told me we did receive MSBuildProjectDiagnostics events, on which I started listening.

Omnisharp-roslyn seems to confirm this is a reliable source of truth (moreso than the existing detection method): https://github.com/OmniSharp/omnisharp-roslyn/blob/e5606354c5c652fc8ecc83d877a2b00d32656d5d/src/OmniSharp.MSBuild/ProjectManager.cs#L327

@Melandel
Copy link
Contributor Author

@microsoft-github-policy-service agree

@nickspoons
Copy link
Member

Thanks @Melandel, sorry for the slow response time.

I've had a look and this looks fine. In dotnet6 project the MsBuildProjectDiagnostics is received, it is not received in multi-target projects. I'd be curious to see an example of a project where the existing system was not working for you, I haven't come across that.

Before I merge, can you please force push a clean commit message that doesn't include emoji or excessive whitespace? "Receive information that a project was loaded" is plenty.

@Melandel
Copy link
Contributor Author

Melandel commented Sep 23, 2023

Sure!

I'd be curious to see an example of a project where the existing system was not working for you

https://github.com/Melandel/Zuc.Tests.SourceCodeExperiments

can you please force push a clean commit message that doesn't include emoji or excessive whitespace?

will do :)

@Melandel Melandel changed the title 🐛 fix (projects-loading): receive information that a project was loaded Receive information that a project was loaded Sep 23, 2023
@Melandel Melandel force-pushed the fix-loaded-projects-acknowledgement branch from 5c8053a to eb80a73 Compare September 23, 2023 22:45
@nickspoons
Copy link
Member

https://github.com/Melandel/Zuc.Tests.SourceCodeExperiments

I am able to open this solution without issues in both linux and Windows.

Apart from not being able to reproduce the original issue, the problem I'm having with this PR is that s:AcknowledgeLoadedProject() is called twice for each project, so while loading the solution my statusbar (which displays the project load status) says e.g. "Zuc.Tests.SourceCodeExperiments (20 of 12)". This can be resolved with this line as the first line of s:AcknowledgeLoadedProject, to exit early when the project has already been loaded:

  if index(a:job.loading, a:project) < 0 | return | endif

💌 Motivation: `OmniSharpReloadProject` throws because it does not find key `job.projects_loaded`
@Melandel Melandel force-pushed the fix-loaded-projects-acknowledgement branch from eb80a73 to 9cf4a72 Compare September 25, 2023 19:19
@Melandel
Copy link
Contributor Author

Melandel commented Sep 25, 2023

Hmm.

When I add if index(a:job.loading, a:project) < 0 | return | endif,
OmniSharpReloadProject echoes Reloading project.csproj in Xs but does not echo Reloaded project.csproj in Ys

When I don't add if index(a:job.loading, a:project) < 0 | return | endif,
OmniSharpReloadProject echoes Reloading project.csproj in Xs then echoes Reloaded project.csproj in Ys

PS: Alright, I can confirm that OmniSharpReloadProject does not trigger Successfully loaded project/Failed to load project responses, but does trigger MsBuildProjectDiagnostics response

…sponses and `OmniSharpReloadProject` project load responses
@Melandel
Copy link
Contributor Author

The last commit should be a better solution to both our issues.

Let me know what you think!

@nickspoons
Copy link
Member

nickspoons commented Sep 25, 2023

I don't have the problems you are having. On my system, the Successfully loaded project/Failed to load project responses are fired on both server start and project reload, and so is the MsBuildProjectDiagnostics response (although the MsBuildProjectDiagnostics event is only fired for dotnet projects not Framework or multi-target solutions, so can't be used alone).

Edit: I accidentally wrote "project load" instead of "project reload" which was confusing

@nickspoons
Copy link
Member

nickspoons commented Sep 25, 2023

OK after more testing I realise I was wrong in my previous comment, the MsBuildProjectDiagnostics event is getting fired. The strange thing in your case is just that the Successfully loaded project/Failed to load project responses messages are not received for you during project-reload, but apparently are received when the server is started (and so presumably when running :OmniSharpRestartServer).

@Melandel
Copy link
Contributor Author

Melandel commented Sep 25, 2023

Found it.

  • :OmniSharpReloadProject does not get Successfully loaded project when let g:OmniSharp_loglevel = 'none'
  • :OmniSharpReloadProject DOES get Successfully loaded project when let g:OmniSharp_loglevel = 'info'

Wild guess from omnisharp-roslyn's code.

So, this apparently means the currently implemented triggers are dependant on having let g:OmniSharp_loglevel = 'warning', otherwise projects that failed to load won't be detected

@nickspoons
Copy link
Member

Ahhh that makes sense

@Melandel
Copy link
Contributor Author

Melandel commented Sep 25, 2023

Hehe.

Should we parse MSBuildProjectDiagnostics's warnings and errors from now on in order to detect success/failure to load projects, failure being defined as the presence of errors? (or something?)

@nickspoons
Copy link
Member

I actually think it might be safer to override the value of g:OmniSharp_loglevel that is sent to the server, and send 'Info' if it is set to 'none', and just not write anything to the log

@nickspoons
Copy link
Member

@Melandel I've made a very simple PR to handle this, could you please give #851 a try and confirm that it resolves the issue for you?

@nickspoons
Copy link
Member

Resolved in #851

@nickspoons nickspoons closed this Oct 11, 2023
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.

2 participants