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.

Sign up for GitHub

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

Jump to bottom

Update versions to use most recent LanguageServer changes. #61496

Merged
merged 4 commits into from
May 26, 2022
Merged

Update versions to use most recent LanguageServer changes. #61496

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16c8bb5
update versioning to use languageserver.client.implementation
akhera99 May 24, 2022
f9aa60b
merge main
akhera99 May 24, 2022
c0e2ecb
fix
akhera99 May 24, 2022
ac5bd25
Simplify LSP reference update
sharwell May 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@
<MicrosoftVisualStudioExtensibilityTestingVersion>0.1.132-beta</MicrosoftVisualStudioExtensibilityTestingVersion>
<!-- 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>
Copy link
Member

Choose a reason for hiding this comment

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

Would this break integration tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

It did in the other PR I made, what can I do to fix that?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@genlu genlu May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I think p1 should be rolling out to machines next week, so it might be reasonable to wait

<ILAsmPackageVersion>5.0.0-alpha1.19409.1</ILAsmPackageVersion>
<ILDAsmPackageVersion>5.0.0-preview.1.20112.8</ILDAsmPackageVersion>
<MicrosoftVisualStudioLanguageServerProtocolPackagesVersion>17.3.15</MicrosoftVisualStudioLanguageServerProtocolPackagesVersion>
<MicrosoftVisualStudioLanguageServerProtocolPackagesVersion>17.3.2017</MicrosoftVisualStudioLanguageServerProtocolPackagesVersion>
<MicrosoftVisualStudioShellPackagesVersion>17.2.32505.113</MicrosoftVisualStudioShellPackagesVersion>
<RefOnlyMicrosoftBuildPackagesVersion>16.5.0</RefOnlyMicrosoftBuildPackagesVersion>
<!-- The version of Roslyn we build Source Generators against that are built in this
Expand Down Expand Up @@ -153,7 +153,8 @@
<MicrosoftVisualStudioLanguageServerProtocolVersion>$(MicrosoftVisualStudioLanguageServerProtocolPackagesVersion)</MicrosoftVisualStudioLanguageServerProtocolVersion>
<MicrosoftVisualStudioLanguageServerProtocolExtensionsVersion>$(MicrosoftVisualStudioLanguageServerProtocolPackagesVersion)</MicrosoftVisualStudioLanguageServerProtocolExtensionsVersion>
<MicrosoftVisualStudioLanguageServerProtocolInternalVersion>$(MicrosoftVisualStudioLanguageServerProtocolPackagesVersion)</MicrosoftVisualStudioLanguageServerProtocolInternalVersion>
<MicrosoftVisualStudioLanguageServerClientVersion>17.0.5165</MicrosoftVisualStudioLanguageServerClientVersion>
<MicrosoftVisualStudioLanguageServerClientVersion>17.3.2062-preview</MicrosoftVisualStudioLanguageServerClientVersion>
<MicrosoftVisualStudioLanguageServerClientImplementationVersion>17.3.2062-preview</MicrosoftVisualStudioLanguageServerClientImplementationVersion>
<MicrosoftVisualStudioLanguageStandardClassificationVersion>$(VisualStudioEditorPackagesVersion)</MicrosoftVisualStudioLanguageStandardClassificationVersion>
<MicrosoftVisualStudioLiveShareVersion>2.18.6</MicrosoftVisualStudioLiveShareVersion>
<MicrosoftVisualStudioLiveShareLanguageServicesVersion>3.0.6</MicrosoftVisualStudioLiveShareLanguageServicesVersion>
Expand Down
3 changes: 3 additions & 0 deletions src/EditorFeatures/Core/Microsoft.CodeAnalysis.EditorFeatures.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
<DefineConstants>$(DefineConstants);EDITOR_FEATURES</DefineConstants>
<ApplyNgenOptimization Condition="'$(TargetFramework)' == 'netstandard2.0'">full</ApplyNgenOptimization>

<NoWarn>$(NoWarn);NU1701;</NoWarn>

<!-- NuGet -->
<PackageId>Microsoft.CodeAnalysis.EditorFeatures.Common</PackageId>
<IsPackable>true</IsPackable>
Expand Down Expand Up @@ -39,6 +41,7 @@
<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" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

@gundermanc gundermanc May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@gundermanc gundermanc May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay cool, so we can do that when the appropriate Roslyn insertion for VS Mac demands it.

<!-- Explicit reference to Shell.15.0 et. al. with NU1701 only until the LSP client targets netstandard2.0 https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1369985/ -->
<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" />
Expand Down