-
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
Ship Devkit dependencies as a nuget package to support directly including in the C# extension #70875
Conversation
…ding in the C# extension
@@ -33,7 +31,7 @@ private AssemblyLoadContextWrapper(AssemblyLoadContext assemblyLoadContext, Immu | |||
_logger = logger; | |||
} | |||
|
|||
public static bool TryLoadExtension(string assemblyFilePath, string? sharedDependenciesPath, ILogger? logger, [NotNullWhen(true)] out Assembly? assembly) | |||
public static bool TryLoadExtension(string assemblyFilePath, ILogger? logger, [NotNullWhen(true)] out Assembly? assembly) |
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.
@tmat would like you're review on this PR (but especially here). Seems unnecessary to load shared deps from devkit now. Smoke test on telemetry and hot reload works.
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.
Yeah, I think it'd even be incorrect to do so. We don't want to depend on versions of assemblies shipping in the DevKit.
src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/ServerConfigurationFactory.cs
Outdated
Show resolved
Hide resolved
// Use the SharedDependenciesPath option as a proxy for whether or not devkit is running. | ||
var isDevkitEnabled = !string.IsNullOrEmpty(serverConfiguration.SharedDependenciesPath); | ||
// Check if the devkit extension is included to see if devkit is enabled. | ||
var isDevkitEnabled = serverConfiguration.ExtensionAssemblyPaths.Any(path => path.Contains("Microsoft.VisualStudio.LanguageServices.DevKit.dll")); |
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 think this would be better: path => Path.GetFileName(path) == "..."
See client side PR for details - dotnet/vscode-csharp#6681