Use AssemblyLoadContext to load EFC.Design #35527
Conversation
|
@baronfel @ViktorHofer @rainersigwald Please take a look at the first commit to verify whether this is a sane approach to find and load a referenced PrivateAssets assembly when the |
AndriySvyryd
force-pushed
the
Issue35265
branch
from
ef3416c to
49d1065
Compare
There was a problem hiding this comment.
Copilot reviewed 5 out of 15 changed files in this pull request and generated no comments.
Files not reviewed (10)
- src/EFCore.Tasks/buildTransitive/Microsoft.EntityFrameworkCore.Tasks.targets: Language not supported
- src/EFCore.Tools/tools/EntityFrameworkCore.psm1: Language not supported
- src/ef/Properties/Resources.Designer.cs: Language not supported
- src/ef/Properties/Resources.resx: Language not supported
- src/EFCore.Tasks/Tasks/Internal/OperationTaskBase.cs: Evaluated as low risk
- src/dotnet-ef/Project.cs: Evaluated as low risk
- src/dotnet-ef/RootCommand.cs: Evaluated as low risk
- src/ef/AppDomainOperationExecutor.cs: Evaluated as low risk
- src/ef/Commands/DbContextOptimizeCommand.cs: Evaluated as low risk
- src/ef/Commands/MigrationsHasPendingModelChangesCommand.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/ef/Commands/MigrationsBundleCommand.cs:39
- The variable 'context' should be nullable or have a null check to ensure consistency with the nullable 'version' variable.
string context;
d55bc66 to
d26173f
Compare
|
@agocke - please add relevant folks to review if this is a sane approach to find and load a referenced PrivateAssets assembly when the .deps.json file is not available. |
There was a problem hiding this comment.
Sorry, I missed this earlier. MSBuild loads plugins (tasks, loggers) in an ALC that ensures unification for MSBuild assemblies and supports references from the plugin DLL to other DLLs placed next to it, both with and without a .deps.json: https://github.com/dotnet/msbuild/blob/353c0f3d37957cc98bfa6a76b568d70d12193fc3/src/Shared/MSBuildLoadContext.cs
Is that sort of transitive reference something that is relevant to y'all?
| @@ -17,10 +20,15 @@ internal class ReflectionOperationExecutor : OperationExecutorBase | |||
| private const string ReportHandlerTypeName = "Microsoft.EntityFrameworkCore.Design.OperationReportHandler"; | |||
| private const string ResultHandlerTypeName = "Microsoft.EntityFrameworkCore.Design.OperationResultHandler"; | |||
| private readonly Type _resultHandlerType; | |||
| private string? _efcoreVersion; | |||
| #if !NET472 | |||
There was a problem hiding this comment.
Use a less specific tfm, like NET. Otherwise this will break when we update the tfm. Same for the rest of the #if in this PR.
| @@ -47,7 +55,20 @@ public ReflectionOperationExecutor( | |||
|
|
|||
| AppDomain.CurrentDomain.AssemblyResolve += ResolveAssembly; | |||
|
|
|||
| _commandsAssembly = Assembly.Load(new AssemblyName { Name = DesignAssemblyName }); | |||
| #if !NET472 | |||
| _commandsAssembly = AssemblyLoadContext.LoadFromAssemblyName(new AssemblyName(DesignAssemblyName)); | |||
There was a problem hiding this comment.
You can just load with the string name.
| _assemblyLoadContext = AssemblyLoadContext.Default; | ||
| } | ||
|
|
||
| return AssemblyLoadContext.Default; |
There was a problem hiding this comment.
I don’t understand this logic. Why is using an ALC opportunistic? It seems like you either need one or you don’t.
There was a problem hiding this comment.
ALC is always used, this code just ensures that the Resolving handler is registered even if DesignAssemblyPath is set after the first time AssemblyLoadContext is requested.
This code runs when EF tools are executed and loads EFC.Design reference from the user's project (usually with PrivateAssets="all") and later usually loads the assembly built from the project. No isolation is necessary since the process terminates after performing the requested operation. ALC is leveraged to load the assemblies in a more robust manner. |
Fixes #35265
Part of #18840