allow plugins to be specified by a dll file path #1069
Conversation
|
@david-driscoll I could alternately PR this to master. Rebasing from there to the lsp branch will result in a small merge conflict. Let me know if you have a preference. |
| @@ -94,7 +94,7 @@ private void CreateCompositionHost(InitializeParams initializeParams) | |||
| var compositionHostBuilder = new CompositionHostBuilder(_serviceProvider, _environment, eventEmitter) | |||
| .WithOmniSharpAssemblies() | |||
| .WithAssemblies(typeof(LanguageServerHost).Assembly) | |||
| .WithAssemblies(plugins.AssemblyNames.Select(Assembly.Load).ToArray()); | |||
| .WithAssemblies(plugins.LoadAssemblies().ToArray()); | |||
There was a problem hiding this comment.
i think you could do var assemblyLoader = _serviceProvider.GetRequiredService<IAssemblyLoader>(); here and use the OmniSharp assembly loader instead, this way the assembly loading logic would be always the same
There was a problem hiding this comment.
Thanks, I didn't notice that.
IAssemblyLoader has different methods for Load (which in the one implementation calls Assembly.Load) and LoadFrom (which calls Assembly.LoadFrom) so the method for deciding which to call based on file existence still needs to be added somewhere. It could be added to IAssemblyLoaderExtensions but would need a clearer name (e.g. LoadByAssemblyNameOrPath would be clear but verbose.)
The question is, since this is rationalizing command-line arguments for the -pl option, maybe resolving this ambiguity belongs closer to the parsing operation anyway?
There was a problem hiding this comment.
@filipw Thoughts on the latest changes?
There was a problem hiding this comment.
sorry for the delay - what I actually meant is that this logic would ideally utilize the already existing methods of the loader (or its extension methods) - as they seem to cover everything needed.
For example there is already a method to load an assembly by string name (here https://github.com/OmniSharp/omnisharp-roslyn/blob/master/src/OmniSharp.Abstractions/Services/IAssemblyLoader.cs#L23), by multiple names (here https://github.com/OmniSharp/omnisharp-roslyn/blob/master/src/OmniSharp.Abstractions/Services/IAssemblyLoader.cs#L34) or to load it from a given path (here https://github.com/OmniSharp/omnisharp-roslyn/blob/master/src/OmniSharp.Host/Services/AssemblyLoader.cs#L64). So you don't need to add any new ones, you just need to make the decision if it's a file or not a file and call the relevant existing method.
The advantage of this is also that the existing methods handle exceptions so they would prevent the logic from crashing already at startup and they log so we could easily see in the server logs which assemblies have been successfully loaded as plugins.
jonsequitur
force-pushed
the
allow-plugin-specified-by-dll-file-path
branch
from
ed648a2
to
541be2e
Compare
😀
This allows plugins to be specified using the path to a dll. This adds to the existing behavior of allowing plugins to be specified using an assembly name.