Switch to workspace service to ensure the analyzer loader uses streams on linux #73809
Conversation
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/VSCodeAnalyzerLoader.cs
Outdated
Show resolved
Hide resolved
| @@ -17,7 +17,7 @@ internal sealed class ProjectAnalyzerReference(string fullPath, IProjectSystemDi | |||
| // NOTE: It is important that we share the same shadow copy assembly loader for all VisualStudioAnalyzer instances. | |||
| // This is required to ensure that shadow copied analyzer dependencies are correctly loaded. | |||
| private static readonly IAnalyzerAssemblyLoader s_analyzerAssemblyLoader = | |||
| new ShadowCopyAnalyzerAssemblyLoader(Path.Combine(Path.GetTempPath(), "VS", "AnalyzerAssemblyLoader")); | |||
| DefaultAnalyzerAssemblyLoader.CreateNonLockingLoader(Path.Combine(Path.GetTempPath(), "VS", "AnalyzerAssemblyLoader")); | |||
There was a problem hiding this comment.
I considered using the workspace service here, but that uses a different path in temp to put them, so I didn't. cc @jasonmalinowski
There was a problem hiding this comment.
Does this still run in VS Code? Other than the initializer running, does anything use it?
There was a problem hiding this comment.
resolved by adding a version of the service exported for the host workspace (using the VS/AnalyzerAssemblyLoader path)
src/Workspaces/Core/Portable/Workspace/Host/Metadata/AnalyzerAssemblyLoaderOptions.cs
Show resolved
Hide resolved
| // Shadow copy analyzer files coming from packages to avoid locking the files in NuGet cache. | ||
| // NOTE: The provider will always return the same singleton analyzer loader instance, which is important to | ||
| // ensure that shadow copied analyzer dependencies are correctly loaded. | ||
| _analyzerAssemblyLoader = provider.GetLoader(shadowCopy: true); |
There was a problem hiding this comment.
So do we pass false for shadow copy anywhere? Because I'm not really sure why that flag exists. It seems the shadow copy or not flag is really a host question more or less...or is the flag a "you might want to shadow copy" hint and saying no definitely opts out?
There was a problem hiding this comment.
We potentially do here -
I couldn't track down though where that could be coming from. For now will leave as-is.
There was a problem hiding this comment.
Want to file a bug to track?
src/Workspaces/Core/Portable/Diagnostics/HostAnalyzerAssemblyLoaderProvider.cs
Outdated
Show resolved
Hide resolved
...Server/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerWorkspaceFactory.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/VSCodeAnalyzerLoader.cs
Outdated
Show resolved
Hide resolved
dibarbet
force-pushed
the
fix_analyzer_loading_linux
branch
from
c14fe84
to
402ab2d
Compare
dibarbet
force-pushed
the
fix_analyzer_loading_linux
branch
from
402ab2d
to
deccf46
Compare
src/LanguageServer/Protocol/Features/AbstractAnalyzerAssemblyLoaderProvider.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Provider to allow MEF access to <see cref="ExtensionAssemblyManager"/> | ||
| /// Must be done this way as the manager is required to create MEF as well. | ||
| /// </summary> |
There was a problem hiding this comment.
This is fine to go with for now, but I'm guessing there's some way we can directly inject the ExtensionAssemblyManager into the catalog. We do similar things though for logging as it is, so this isn't new.
There was a problem hiding this comment.
I did take a quick look. There's nothing in the discovery / catalog APIs that I could find where we could provide a specific instance of an export (we can manually add a type). This https://github.com/microsoft/vs-mef/blob/main/doc/dynamic_recomposition.md#adding-parts-directly-with-compositioncontainercompose also recommends basically what we're doing.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs
Outdated
Show resolved
Hide resolved
Resolves #73795