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

Fix EA layering for Razor.ExternalAccess #77927

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fix EA layering for Razor.ExternalAccess #77927

wants to merge 8 commits into from

Conversation

ryzngard
Copy link
Contributor

Moves things to /Shared to ship in both MS.CA.EA.Razor and MS.CA.EA.Razor.Features

  • At some point we can rename MS.CA.EA.Razor to MS.CA.EA.Razor.EditorFeatures but I didn't bother with that in this PR
  • Shipping in both to not require dual insertion. We can fix this after a consuming change in Razor is inserted

@ryzngard ryzngard requested a review from a team as a code owner March 31, 2025 21:28
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 31, 2025
davidwengier
davidwengier approved these changes Mar 31, 2025
View reviewed changes
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

I assume you've tested this with cohosting in VS and things work? The service hub IVTs have been particularly (and non-sensically) fussy in my experience. But if it works it works.

I would definitely hold until after the snap though, just in case.

...uageServer/ProtocolUnitTests/Microsoft.CodeAnalysis.LanguageServer.Protocol.UnitTests.csproj
@@ -23,6 +23,7 @@
<ProjectReference Include="..\..\Compilers\Core\Portable\Microsoft.CodeAnalysis.csproj" />
<ProjectReference Include="..\..\Compilers\CSharp\Portable\Microsoft.CodeAnalysis.CSharp.csproj" />
<ProjectReference Include="..\..\Compilers\VisualBasic\Portable\Microsoft.CodeAnalysis.VisualBasic.vbproj" />
<ProjectReference Include="..\..\EditorFeatures\Core\Microsoft.CodeAnalysis.EditorFeatures.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

@dibarbet / @tmat / @CyrusNajmabadi is this a layering violation? Language Server is in "features", but not sure if it matters since it's just tests.

Copy link
Member

Choose a reason for hiding this comment

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

yes this is a violation - if we didn't have the reference before we should avoid adding it if possible. What triggered the inclusion here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already existed. You can see I fixed some of the violation code in src/LanguageServer/ProtocolUnitTests/Completion/CompletionResolveTests.cs that was referencing Microsoft.VisualStudio.Text.Adornments

I don't immediately remember how this was included transitively before but I can get a binlog from main and look

Copy link
Member

Choose a reason for hiding this comment

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

It was probably Microsoft.CodeAnalysis.Remote.ServiceHub.csproj referencing Microsoft.CodeAnalysis.ExternalAccess.Razor.csproj which references Microsoft.CodeAnalysis.EditorFeatures.csproj

I'm fine with this then as it is a pre-existing violation then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yea that's definitely it

ryzngard
ryzngard commented Mar 31, 2025
View reviewed changes
src/Workspaces/Remote/ServiceHub/Microsoft.CodeAnalysis.Remote.ServiceHub.csproj Outdated
@@ -15,7 +15,7 @@
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\Features\Core\Portable\Microsoft.CodeAnalysis.Features.csproj" />
<ProjectReference Include="..\..\..\Tools\ExternalAccess\Razor\EditorFeatures\Microsoft.CodeAnalysis.ExternalAccess.Razor.csproj" />
<ProjectReference Include="..\..\..\Tools\ExternalAccess\Razor\Features\Microsoft.CodeAnalysis.ExternalAccess.Razor.Features.csproj" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi this should fix #76674 but will require a dual insertion if we do this now. I'm not sure what the guidelines for 17.15 are. We can also delay this until Razor inserts with the new EA layering dependencies...

dibarbet
dibarbet requested changes Mar 31, 2025
View reviewed changes
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

only blocking on understanding the EditorFeatures reference in protocol.unittests

dibarbet
dibarbet approved these changes Mar 31, 2025
View reviewed changes
...uageServer/ProtocolUnitTests/Microsoft.CodeAnalysis.LanguageServer.Protocol.UnitTests.csproj
@@ -23,6 +23,7 @@
<ProjectReference Include="..\..\Compilers\Core\Portable\Microsoft.CodeAnalysis.csproj" />
<ProjectReference Include="..\..\Compilers\CSharp\Portable\Microsoft.CodeAnalysis.CSharp.csproj" />
<ProjectReference Include="..\..\Compilers\VisualBasic\Portable\Microsoft.CodeAnalysis.VisualBasic.vbproj" />
<ProjectReference Include="..\..\EditorFeatures\Core\Microsoft.CodeAnalysis.EditorFeatures.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

It was probably Microsoft.CodeAnalysis.Remote.ServiceHub.csproj referencing Microsoft.CodeAnalysis.ExternalAccess.Razor.csproj which references Microsoft.CodeAnalysis.EditorFeatures.csproj

I'm fine with this then as it is a pre-existing violation then

@build-analysis build-analysis bot mentioned this pull request Mar 31, 2025
Open
3 tasks
@ryzngard ryzngard enabled auto-merge (squash) April 2, 2025 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwengier davidwengier davidwengier approved these changes

@dibarbet dibarbet dibarbet approved these changes

Assignees
No one assigned
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ryzngard @davidwengier @dibarbet @CyrusNajmabadi