-
-
Notifications
You must be signed in to change notification settings - Fork 934
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
[Editor] Upgrade RolsynPad.Roslyn to v4.8.0 #2124
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might work but it feels very hackish. We shouldn't need all of that to make it work.
Instead, we should either properly reimplement the custom workspace/host or reuse the one provided by RoslynPad library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file? It seems unrelated to Roslyn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/microsoft/MSBuildSdks/blob/main/src/NoTargets/README.md ("This can be useful for utility projects that just copy files, ...")
This is a helper project to copy requred dll from nuget folder
~.nuget\packages\microsoft.codeanalysis.languageserver.protocol\4.8.0-7.23558.1\lib\net7.0\Microsoft.CodeAnalysis.LanguageServer.Protocol.dll
We use it because now this dll is deprecated on nuget
https://www.nuget.org/packages/Microsoft.CodeAnalysis.LanguageServer.Protocol/3.9.0-5.21120.8
sources/editor/Stride.Assets.Presentation/AssetEditors/ScriptEditor/RoslynWorkspace.cs
Outdated
Show resolved
Hide resolved
sources/editor/Stride.Assets.Presentation/AssetEditors/ScriptEditor/RoslynHost.cs
Outdated
Show resolved
Hide resolved
sources/editor/Stride.Assets.Presentation/AssetEditors/ScriptEditor/RoslynHost.cs
Outdated
Show resolved
Hide resolved
<RestoreSources> | ||
https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json; | ||
https://pkgs.dev.azure.com/azure-public/vside/_packaging/vssdk/nuget/v3/index.json; | ||
https://pkgs.dev.azure.com/azure-public/vside/_packaging/vs-impl/nuget/v3/index.json; | ||
https://api.nuget.org/v3/index.json | ||
</RestoreSources> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are those sources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took this from RoslynPad here https://github.com/roslynpad/roslynpad/blob/main/src/RestoreHelper/RestoreHelper.csproj
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.Features" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp.Features" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp.EditorFeatures" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.Scripting" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp.Scripting" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.Workspaces" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp.Workspaces" /> | ||
<InternalsAssemblyName Include="Microsoft.CodeAnalysis.LanguageServer.Protocol" /> | ||
|
||
<InternalsAssemblyName Include="RoslynPad.Roslyn" /> | ||
<InternalsAssemblyName Include="RoslynPad.Roslyn.Windows" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it needed, and what does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It allows to use internal implementation from other assemblies https://github.com/aelij/IgnoresAccessChecksToGenerator.
E.g implementation of the IDiagnosticsService
interface from RoslynPad looks like this https://github.com/roslynpad/roslynpad/blob/main/src/RoslynPad.Roslyn/Diagnostics/DiagnosticsService.cs#L7.
It is internal and I think MEF is not able to find it so we see this error: #2043 (comment)
System.Composition.Hosting.CompositionFailedException: 'No export was found for the contract 'IDiagnosticService'.
Same thing RoslynPad is doing with Microsoft.CodeAnalysis
dependencies https://github.com/roslynpad/roslynpad/blob/main/src/RoslynPad.Roslyn/RoslynPad.Roslyn.csproj#L32.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed not used internals, left only required.
@@ -2,6 +2,9 @@ | |||
<PropertyGroup> | |||
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally> | |||
</PropertyGroup> | |||
<PropertyGroup> | |||
<RoslynPrivateVersion>4.8.0-7.23558.1</RoslynPrivateVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be needed if the <PackageVersion>
version is defined in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not defined in this file but we could move this to Stride.RestoreHelper.csproj
and set it directly there.
<Target Name="GetLibReferences" Outputs="@(Reference)"> | ||
<ItemGroup> | ||
<Reference Include="$(PkgMicrosoft_CodeAnalysis_LanguageServer_Protocol)/lib/net7.0/*.dll" /> | ||
</ItemGroup> | ||
</Target> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work? Where is the reference (documentation) for this trick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered in another comment, but here are more details https://duanenewman.net/blog/post/a-better-way-to-override-references-with-packagereference/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't do. The RoslynPad workspace is not a MSBuild workspace operating on a solution with many projects like Stride does. RoslynPad creates a simple Roslyn workspace per document (as they are independent from one another), and is way simpler than what we have in Stride. Also it has no need to detect external changes, as RoslynPad has all of this "project" structure in memory. At least, that's the way it was implemented the last time I checked (last year). I have a prototype I made some time ago where I integrated RoslynPad inside Stride (i.e. not as an assembly reference but as code, to ease customization and get only the needed parts). I had to reimplement some parts of the Stride workspace mixing it with RoslynPad's. |
When I was working on this PR I wondered why some code looks almost the same as in RoslynPad repository and why it wasn't used. But from this #2124 (comment) explained it. In this PR I just fixed issue by issue that was happening after upgrading |
i cant run this PR , ive ran but i end up with these errors |
Probably you don't have this package in the .nuget folder. I think it was added there by some of the .NET SDKs as it is not available on nuget website, here is my SDKs:
|
|
Try to remove api.nuget.org source from the I see this package in another source: https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json All versions of this package: https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/d1622942-d16f-48e5-bc83-96f4539e7601/nuget/v3/flat2/Microsoft.CodeAnalysis.LanguageServer.Protocol/index.json` Version used by RolsynPad: https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/d1622942-d16f-48e5-bc83-96f4539e7601/nuget/v3/registrations2-semver2/Microsoft.CodeAnalysis.LanguageServer.Protocol/4.8.0-7.23558.1.json In the response from the second link there is a direct link to nupkg file. |
PR Details
Upgraded
RoslynPad.Roslyn*
packages to version 4.8.0Description
Upgraded required packages, added ability for MEF to access internal implementations of contracts using
IgnoresAccessChecksToGenerator
.To get access to some required unpublished as Nuget packages contracts from
Microsoft.CodeAnalysis.LanguageServer.Protocol
namespace addedStride.RestoreHelper
. CopiedGetLibReferences
to other places whereStride.props
andStride.Core.props
were not referenced to fix build.The approach was the same as was used by RoslynPad in relation to
Microsoft.CodeAnalysis.*
internal dependencies. On Stride side same actions were required in relation toRoslynPad.Roslyn
internal dependencies.! Commented out
RoslynWorkspace.EnableDiagnostics()
(same happens withDiagnosticProvider.Enable()
) call because of error:Probably MEF is resolving RoslynPad's workspace type instead of Stride's.
! So maybe some things related to diagnostics might stop working.
Related Issue
2043 Fixed issue mentioned in this comment
Motivation and Context
Fixes some functionality related to code analysis in script editor.
Types of changes
Checklist