-
Notifications
You must be signed in to change notification settings - Fork 2
🐛 Fix plugin dependency loading using AssemblyLoadContext #35
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
Conversation
Co-authored-by: matt-goldman <[email protected]>
Co-authored-by: matt-goldman <[email protected]>
…ndency resolution Co-authored-by: matt-goldman <[email protected]>
matt-goldman
left a comment
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.
@copilot great work, please review comments. One of them is a requested change, for the other comment please either remove the file or respond to the comment clarifying why it needs to be kept.
…ax and remove temp file Co-authored-by: matt-goldman <[email protected]>
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.
Pull Request Overview
This PR fixes a critical bug in the Blake plugin loading system where plugins with NuGet dependencies would fail at runtime due to improper assembly loading. The fix implements a proper plugin loading mechanism using AssemblyLoadContext to create isolated loading contexts for each plugin.
Key Changes
- Added isolated plugin loading: New
PluginLoadContextclass creates separate loading contexts for each plugin with proper dependency resolution - Enhanced plugin loader: Modified the plugin loading logic to use
AssemblyLoadContextinstead of directAssembly.LoadFrom() - Comprehensive testing: Added test plugin with dependencies and corresponding unit/integration tests to validate the fix
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Blake.BuildTools/Utils/PluginLoadContext.cs | New custom AssemblyLoadContext for isolated plugin loading with dependency resolution |
| src/Blake.BuildTools/Utils/PluginLoader.cs | Updated to use PluginLoadContext instead of Assembly.LoadFrom() |
| src/Blake.BuildTools/Blake.BuildTools.csproj | Added InternalsVisibleTo for test access |
| tests/Blake.IntegrationTests/TestPluginWithDependencies/ | Test plugin project with Newtonsoft.Json dependency to validate fix |
| tests/Blake.BuildTools.Tests/Utils/PluginLoaderTests.cs | Unit tests for plugin loading functionality |
| tests/Blake.IntegrationTests/Commands/BlakePluginTests.cs | Integration test for end-to-end plugin dependency loading |
Co-authored-by: Copilot <[email protected]>
matt-goldman
left a comment
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.
Excellent
The plugin loader was only loading plugin assemblies directly using
Assembly.LoadFrom(), which doesn't resolve plugin dependencies properly. This caused plugins with NuGet dependencies (like SkiaSharp, Lucene.NET, etc.) to fail at runtime with assembly loading errors.Changes
Added PluginLoadContext.cs: A custom
AssemblyLoadContextthat usesAssemblyDependencyResolverto create isolated plugin loading contexts. Each plugin gets its own context that can resolve dependencies from the plugin's.deps.jsonfile.Modified LoadPluginDLLs method: Changed from using
Assembly.LoadFrom()to creating aPluginLoadContextfor each plugin and usingLoadFromAssemblyName()for proper dependency resolution.Technical Implementation
The solution follows Microsoft's recommended pattern for plugin loading with dependencies:
AssemblyLoadContextmarked as collectible for proper cleanupAssemblyDependencyResolverresolves both managed and unmanaged dependencies based on the plugin's.deps.jsonfileTesting
Added comprehensive tests including:
TestPluginWithDependenciesthat uses Newtonsoft.Json to verify dependency loadingPluginLoaderTeststo verify the fix works correctlyBlakePluginTestsfor end-to-end validationBefore/After
Before: Plugins with dependencies would fail at bake time:
After: Plugins can now include any NuGet dependencies and they will be properly resolved and loaded, enabling use cases like:
Fixes #34.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
esm.ubuntu.com/usr/lib/apt/methods/https(dns block)If you need me to access, download, or install something from one of these locations, you can either:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.