-
Notifications
You must be signed in to change notification settings - Fork 2
Optimize PluginLoader to check DLLs before running dotnet restore #52
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]>
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.
LGTM
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 optimizes the PluginLoader to reduce build times by checking for plugin DLL existence and versions before running dotnet restore. The optimization reverses the execution order to validate plugins first, then conditionally restore only when needed.
Key changes:
- Introduces smart restoration logic that validates both NuGet and project reference plugins before deciding whether to run
dotnet restore - Adds version checking for NuGet packages using
FileVersionInfoto ensure correct DLL versions - Maintains backward compatibility while providing significant performance improvements for the common case where all plugins are already present and valid
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Blake.BuildTools/Utils/PluginLoader.cs | Core optimization implementation with new helper methods for plugin validation and conditional restore logic |
| tests/Blake.BuildTools.Tests/Utils/PluginLoaderTests.cs | Comprehensive test coverage for new helper methods and integration scenarios |
| if (fileVersion.StartsWith(plugin.Version)) | ||
| { | ||
| return true; | ||
| } |
Copilot
AI
Aug 24, 2025
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.
Using StartsWith for version comparison is unreliable and could lead to false positives. For example, version '1.0' would incorrectly match file version '1.0.1.2' or '1.00.0'. Consider using System.Version for proper semantic version comparison or exact string matching.
| if (fileVersion.StartsWith(plugin.Version)) | |
| { | |
| return true; | |
| } | |
| // Compare versions component-wise for leniency, but avoid false positives | |
| if (Version.TryParse(plugin.Version, out var expectedVersion) && Version.TryParse(fileVersion, out var actualVersion)) | |
| { | |
| // Compare only as many components as are present in plugin.Version | |
| bool matches = true; | |
| if (expectedVersion.Major != actualVersion.Major) matches = false; | |
| if (expectedVersion.Minor != -1 && expectedVersion.Minor != actualVersion.Minor) matches = false; | |
| if (expectedVersion.Build != -1 && expectedVersion.Build != actualVersion.Build) matches = false; | |
| if (expectedVersion.Revision != -1 && expectedVersion.Revision != actualVersion.Revision) matches = false; | |
| if (matches) | |
| { | |
| return true; | |
| } | |
| } | |
| else | |
| { | |
| // Fallback: exact string match | |
| if (fileVersion == plugin.Version) | |
| { | |
| return true; | |
| } | |
| } |
| catch (Exception ex) | ||
| { | ||
| logger?.LogDebug(ex, "Error checking version for NuGet plugin: {dllPath}", plugin.DllPath); | ||
| return false; // Assume invalid if we can't check version |
Copilot
AI
Aug 24, 2025
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.
The comment suggests assuming invalid when version checking fails, but this could cause unnecessary restores on systems where FileVersionInfo.GetVersionInfo consistently fails due to file format issues. Consider logging the specific exception type and potentially having different fallback behavior for different exception types.
| catch (Exception ex) | |
| { | |
| logger?.LogDebug(ex, "Error checking version for NuGet plugin: {dllPath}", plugin.DllPath); | |
| return false; // Assume invalid if we can't check version | |
| catch (NotSupportedException ex) | |
| { | |
| logger?.LogWarning(ex, "File format not supported when checking version for NuGet plugin: {dllPath}. Assuming valid.", plugin.DllPath); | |
| return true; // Assume valid if file format is not supported | |
| } | |
| catch (Exception ex) | |
| { | |
| logger?.LogError(ex, "Unexpected error checking version for NuGet plugin: {dllPath}. Assuming invalid.", plugin.DllPath); | |
| return false; // Assume invalid for other exceptions |
This PR optimizes the PluginLoader to significantly reduce build times by checking for plugin DLL existence and versions before running
dotnet restore.Problem
The current implementation always runs
dotnet restoreduring plugin loading, which can be slow depending on cache state, number of packages, and projects. This happens even when all required plugin DLLs are already present and valid.Solution
Reversed the execution order to check DLLs first, then conditionally restore only when needed:
Before:
After:
Key Features
Smart Restoration Logic
dotnet restorewhen plugin DLLs are missing or outdatedVersion Checking
For NuGet packages, the implementation uses
FileVersionInfoto compare the DLL's file version with the package version specified in the csproj. This ensures that after package updates, the correct DLL versions are restored.Backward Compatibility
Performance Benefits
"All plugin DLLs are present and valid, skipping dotnet restore""Some plugin DLLs are missing or outdated, running dotnet restore"Implementation Details
Added helper methods for cleaner code organization:
GetNuGetPluginInfo()- Extracts NuGet plugin information from csprojGetProjectRefPluginInfo()- Extracts project reference plugin informationIsNuGetPluginValid()- Validates NuGet plugin DLL existence and versionAreAllPluginsValid()- Validates all plugins before deciding whether to restoreTesting
Comprehensive test coverage with 8 passing tests:
Fixes #51.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.