-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove the EditorFeatures.Wpf dependency from Remote.ServiceHub #45115
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.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@genlu Can you submit that as a follow-up pull request?
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.
Yea, sure. Will do if we think it'd help.
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.
@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 |
No description provided.