Update versions to use most recent LanguageServer changes.#61496
Update versions to use most recent LanguageServer changes.#61496akhera99 merged 4 commits intodotnet:main-vs-depsfrom
Conversation
| <!-- CodeStyleAnalyzerVersion should we updated together with version of dotnet-format in dotnet-tools.json --> | ||
| <CodeStyleAnalyzerVersion>4.2.0-2.final</CodeStyleAnalyzerVersion> | ||
| <VisualStudioEditorPackagesVersion>17.2.3194</VisualStudioEditorPackagesVersion> | ||
| <VisualStudioEditorPackagesVersion>17.3.37-preview</VisualStudioEditorPackagesVersion> |
There was a problem hiding this comment.
Would this break integration tests?
There was a problem hiding this comment.
It did in the other PR I made, what can I do to fix that?
There was a problem hiding this comment.
We'd need to reinstate -vs-deps branch, so this won't affect people works in main. I also have a change requires new Editor package.
There was a problem hiding this comment.
is this in preview1 or preview2? Preview1 should be rolling out to machines soonish, so if its present there we can avoid a -vs-deps branch
There was a problem hiding this comment.
hmm, looks like rel/d17.3 has 17.3.41-preview, so I guess it will be available soon?
https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/VS?path=%2F.corext%2FConfigs%2FVS-Platform%2FVS-Platform.packageconfig&version=GBrel%2Fd17.3&_a=contents
There was a problem hiding this comment.
I think p1 should be rolling out to machines next week, so it might be reasonable to wait
|
#61494 this just merged, so you'd need a sync :) |
| End Function | ||
| End Class | ||
| End Namespace | ||
| End Namespace No newline at end of file |
|
@akhera99 |
| <PackageReference Include="Microsoft.VisualStudio.Language" Version="$(MicrosoftVisualStudioLanguageVersion)" /> | ||
| <!-- No warn on NU1701 until the LSP client targets netstandard2.0 https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1369985/ --> | ||
| <PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client" Version="$(MicrosoftVisualStudioLanguageServerClientVersion)" PrivateAssets="all" NoWarn="NU1701" /> | ||
| <PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client.Implementation" Version="$(MicrosoftVisualStudioLanguageServerClientImplementationVersion)" PrivateAssets="all" NoWarn="NU1701" /> |
There was a problem hiding this comment.
@gundermanc Does this pose a problem for VS Mac? Currently Microsoft.VisualStudio.LanguageServer.Client.Implementation is excluded from the VS Mac MEF composition, but I'm not sure if thats due to technical limitations or just history.
There was a problem hiding this comment.
Currently Microsoft.VisualStudio.LanguageServer.Client.Implementation is excluded from the VS Mac MEF composition
It is? My most recent understanding is that it should be part of the composition as it contains the LSP client implementation on Mac.
Note that though they share the same name, Microsoft.VisualStudio.LanguageServer.Client.Implementation on VS Mac is a VS Mac and Cocoa specific build with different implementations. The public surface area is similar to that of VS Windows and the assembly reference will resolve but some public types and members available via the Windows implementation do not exist in the Mac one.
There was a problem hiding this comment.
My most recent understanding is that it should be part of the composition
Maybe I'm reading the XML wrong. Its in the composition, but won't be scanned. Perhaps that just means it won't be scanned for [Import]s but can still participate in [Export]s? @KirillOsenkov probably knows.
The public surface area is similar to that of VS Windows
How similar? Is it a bad idea to reference it from a platform agnostic library? ie, is there a chance that this will break VS Mac because the snippet expansion provider isn't present there?
There was a problem hiding this comment.
Maybe I'm reading the XML wrong. Its in the composition, but won't be scanned. Perhaps that just means it won't be scanned for [Import]s but can still participate in [Export]s
IDK what that means specifically. On my Mac it appears to light up correctly.
How similar?
The goal is that all non-obsolete public members and features should eventually be present on both the Mac and Windows hosts but it may take some time to get there. Here's an example of legacy stuff that's excluded: https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient?path=/src/product/RemoteLanguage/Def/ILanguageServiceBroker.cs&version=GBdevelop&line=29&lineEnd=53&lineStartColumn=1&lineEndColumn=7&lineStyle=plain&_a=contents
It looks like LanguageServerSnippetExpander does indeed exist on both Mac and Windows builds. The one thing to be aware of is that @akhera99 made a breaking change to this API May 12th and it probably hasn't yet been inserted to Mac, so we'll have to rev the LSP client before we can insert this version of Roslyn: https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/398270
There was a problem hiding this comment.
If it's in the /MonoDevelop/Ide/Composition, it will be in MEF. You can only be in MEF or not in MEF, so can't have Exports without Imports :)
Scanning is for Mono.Addins, I believe (??) and should be unrelated for our purposes.
There was a problem hiding this comment.
Also worth mention that all of the 'APIs' on Microsoft.VisualStudio.LanguageServer.Client.Implementation.dll are extremely crude and/or suspect. They were originally introduced for LiveShare support and haven't aged well with broader adoption. @MariaSolOs has a thread open with Editor team about its redesign: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1542016
There was a problem hiding this comment.
Okay, false alarm, sorry. I saw an exclude in MonoDevelop.Ide.addin.xml for it here but thats not the MEF composition. Looks like it is in MEF here
so we'll have to rev the LSP client before we can insert this version of Roslyn:
Thank you, this is very good to know. How does that process work?
The other option, potentially, is of course to rearchitect the Roslyn bits here and move this dependency to EditorFeatures.Wpf, but thats worse long term, and probably will take longer anyway.
There was a problem hiding this comment.
How does that process work?
LSP in VSfM is still new and I'm still working on the process. At the moment it's just rev to a consistent set of new packages. Kirill usually recommends picking a package version that matches those inserted in VS Windows.
There was a problem hiding this comment.
Okay cool, so we can do that when the appropriate Roslyn insertion for VS Mac demands it.
|
@akhera99 I see the change to main-vs-deps branch. Are we trying to get this in before CI updates with 17.3 P1 images? Or would it no longer work with P1? |
| <PackageReference Include="Microsoft.VisualStudio.Shell.15.0" Version="$(MicrosoftVisualStudioShell150Version)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" /> | ||
| <PackageReference Include="Microsoft.VisualStudio.Shell.Framework" Version="$(MicrosoftVisualStudioShellFrameworkVersion)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" /> | ||
| <PackageReference Include="Microsoft.VisualStudio.GraphModel" Version="$(MicrosoftVisualStudioGraphModelVersion)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" /> | ||
| <PackageReference Include="Microsoft.VisualStudio.Imaging" Version="$(MicrosoftVisualStudioImagingVersion)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" /> |
This is a helper PR for #61491