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

Add OmniSharp ExternalAccess #52907

Merged
merged 17 commits into from May 4, 2021
Merged

Conversation

333fred
Copy link
Member

@333fred 333fred commented Apr 25, 2021

We had a discussion some months ago about adding an ExternalAccess layer for OmniSharp, as they use a decent amount of private reflection for various services. I've gone through the O# codebase and looked for everything that is using reflection to work. This is the first time I've set up one of these layers, so please let me know what I need to change.

@CyrusNajmabadi
Copy link
Member

Was this modeled after the f# ea impl? Thanks!

@333fred
Copy link
Member Author

333fred commented Apr 26, 2021

Was this modeled after the f# ea impl? Thanks!

Yes. I can't say for certain it was modelled correctly, but the intent was to mirror how F# does it. From what I could see though, F# mostly plugs into existing Roslyn infrastructure (which is then used by VS), whereas O# does a bit of that, and a bit of calling infrastructure itself, serving as the VS side here.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Apr 26, 2021

I think a big part of the f# layer here (if I understand it correctly) is that it's actually a separate dll for the ea layer. This differs from ts, where the ea layer is mixed into the main code for that layer.

@333fred
Copy link
Member Author

333fred commented Apr 26, 2021

I think a big part of the f# layer here (if I understand it correctly) is that it's actually a separate dll for the ea layer. This differs from ts, where the ea layer is mixed into the main code for that layer.

The intention with my change here is that we have a dedicated MS.CA.EA.OS dll.

@CyrusNajmabadi
Copy link
Member

Great. Thanks for the clarification. I can review tomorrow once @sharwell Looks. Thanks!

@333fred
Copy link
Member Author

333fred commented Apr 27, 2021

@sharwell addressed feedback. I also created a separate .CSharp dll for OmniSharp, as they also have a layering system like we do where their .Roslyn project only references the common base, and .Roslyn.CSharp is where they add in the .CSharp roslyn dlls.

@333fred 333fred marked this pull request as ready for review April 28, 2021 22:58
@333fred 333fred requested review from a team as code owners April 28, 2021 22:58
@333fred 333fred requested a review from a team as a code owner April 29, 2021 01:52
@333fred
Copy link
Member Author

333fred commented Apr 29, 2021

I rebased this to squash the fixup commits. The two commits at the end, adding a unit test project to assert we're keeping enums in sync, are new.

@333fred
Copy link
Member Author

333fred commented Apr 29, 2021

I think the main thing this is waiting for is a strong-name key for omnisharp. Their assemblies are unsigned today, which doesn't work with IVT as roslyn is signed. I'll work on getting them signed in the next couple of days and add the public key info here when I do.

@333fred
Copy link
Member Author

333fred commented May 1, 2021

OmniSharp/omnisharp-roslyn#2143 is now merged, and this PR is updated to add OmniSharp's signing key directly.

@333fred
Copy link
Member Author

333fred commented May 1, 2021

@jasonmalinowski @JoeRobich you had a couple of questions I requested more info on: is there anything I need to do? Or is this ready for merge?

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

So generally OK with this but want two things:

  1. Would like a signoff from @JoeRobich or @dibarbet that the NuGet upload handling is correct, or at least as correct as this PR can have it if it's still under investigation.
  2. We get something written up in the docs folder explaining what the ownership process is for this.

For that second item specifically, what the breaking change policy is. For some of our existing external access projects, we can't make any breaking change until we've checked with the team to make sure it's not being used, or we do a complicated dance; the IDE team is well aware of that process since by now I think everybody has personally failed to do it at least once and learned the hard way. Since Omnisharp still chooses to upgrade to newer Roslyn versions (right?) I presume the process here is something more where if we're making a change we'd want to notify them (and we should clarify who "them" is and what communication medium is chosen) so at least they have a heads up that it's coming so they can adapt on next upgrade.

And the docs here can be a single sentence of "ping on GitHub, wait until somebody gives you a thumbs up, and you're good". Just want something written down here.

{
[Shared]
[ExportWorkspaceService(typeof(ISymbolRenamedCodeActionOperationFactoryWorkspaceService))]
class OmniSharpSymbolRenamedCodeActionOperationFactoryWorkspaceService : ISymbolRenamedCodeActionOperationFactoryWorkspaceService
Copy link
Member

Choose a reason for hiding this comment

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

How was Omnisharp using this? We have a PR that will ultimately remove this (#47579) and mostly curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Specify access modifier?

@333fred
Copy link
Member Author

333fred commented May 4, 2021

@jasonmalinowski added a doc.

{
[Shared]
[ExportWorkspaceService(typeof(ISymbolRenamedCodeActionOperationFactoryWorkspaceService))]
class OmniSharpSymbolRenamedCodeActionOperationFactoryWorkspaceService : ISymbolRenamedCodeActionOperationFactoryWorkspaceService
Copy link
Member

Choose a reason for hiding this comment

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

Specify access modifier?

@jasonmalinowski
Copy link
Member

@333fred looks great, thanks!

@333fred 333fred merged commit 5750c2d into dotnet:main May 4, 2021
@ghost ghost added this to the Next milestone May 4, 2021
@333fred 333fred deleted the osharp-externalaccess branch May 4, 2021 03:45
@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants