Remove the EditorFeatures.Wpf dependency from Remote.ServiceHub #45115
Conversation
| @@ -32,6 +32,7 @@ | |||
| <PackageReference Include="Microsoft.VisualStudio.Imaging" Version="$(MicrosoftVisualStudioImagingVersion)" /> | |||
| <PackageReference Include="Microsoft.VisualStudio.Language.StandardClassification" Version="$(MicrosoftVisualStudioLanguageStandardClassificationVersion)" /> | |||
| <PackageReference Include="Microsoft.VisualStudio.Language.Intellisense" Version="$(MicrosoftVisualStudioLanguageIntellisenseVersion)" /> | |||
| <PackageReference Include="Microsoft.VisualStudio.RemoteControl" Version="$(MicrosoftVisualStudioRemoteControlVersion)" /> | |||
There was a problem hiding this comment.
This may be problematic, since this now means VS for Mac needs to ship this binary if they're not. Do we need to move this up a layer to the VS layer rather than down?
There was a problem hiding this comment.
This is a small, netstandard2.0-compatible binary. Today it's used by our Remote.ServiceHub implementation; perhaps we could reorganize the code to remove this requirement but if it doesn't cause a problem it might not be worth the hurdles.
There was a problem hiding this comment.
We already ship RemoteControl. Here's the list of all assemblies in VSMac for reference: https://gist.github.com/KirillOsenkov/7d829534a037c2b3545c95d569f34469
There was a problem hiding this comment.
@KirillOsenkov: even if you're shipping it, does it function? It looks like the code we see uses a bunch of registry and Windows-specific APIs -- do you have a cross-plat version of it or something else?
There was a problem hiding this comment.
Registry is implemented under Mono. The PInvokes are only called on Windows, under guarded runtime checks. I've checked the code under the VSRemoteControl repo, and it all looks okay.
There was a problem hiding this comment.
It doesn't add anything to the MEF composition, so the only way this could get called on Mac is if the code is being invoked explicitly.
There was a problem hiding this comment.
If we want to make sure, we can take this change to guard against potential misuse in the future.
sharwell#2
There was a problem hiding this comment.
@genlu Can you submit that as a follow-up pull request?
There was a problem hiding this comment.
Yea, sure. Will do if we think it'd help.
|
@sharwell |
|
Any update on this one? Is it still blocked? @sharwell @jasonmalinowski |
The problem concerns two features:
Both of these features rely on both of the following dependencies:
Both of those dependencies support execution on .NET Core, but both of these dependencies use Windows APIs which make them runtime agnostic but not OS agnostic (i.e. they will build for Mac but they will not run on Mac). We have several options for moving forward, none of which is particularly great:
For me, the easiest option is (2), while the cleanest option is (1). |
|
Is it possible to encapsulate access to those two dependencies into MEF services and keep it in EditorFeatures.Wpf? So if we can't find the import in EditorFeature layer, then the feature will just be unavailable? |
This reverts commit f755e8a. That change meant our EditorFeatures code was using DllImports in the cross-platform binary, and we weren't sure what that would do in VS for Mac if that code ran. It also brought a dependency on Microsoft.VisualStudio.RemoteControl, which also seems to depend on various Windows specific APIs. We'll finish up the decision of what to do in dotnet#45115
|
This will need to be targeted back to master |
@dibarbet GitHub will automatically retarget this after dev16.8-preview1 is merged to master and deleted. |
As I understand it from @JoeRobich, once I delete the branch the PRs will be closed. Before I delete it I will go back and retarget. |
|
@dibarbet That used to be correct. GitHub changed the behavior because we kept complaining about it. |
|
CHanged the base manually, github wouldn't let us delete the branch with open PRs targeting it |
ghost
added this to the 
No description provided.