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. |
rainersigwald
left a comment
rainersigwald
left a comment
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?
agocke
left a comment
agocke
left a comment
There was a problem hiding this comment.
I need more info on what this code is trying to do and what isolation guarantees it’s supposed to have.
I also haven’t reviewed any of the desktop fx load logic
|
|
||
| _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.
There was a problem hiding this comment.
What do you mean? There's no string overload.
There was a problem hiding this comment.
I meant that you can just use Assembly.Load, but I didn’t realize you were trying to be more explicit.
| _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. |
Improve the --nativeaot description Fixes #35265
AndriySvyryd
force-pushed
the
Issue35265
branch
from
d26173f to
aa5da26
Compare
What does “more robust manner” mean? Just being more explicit about which ALC you expect to load into? |
I mean that using the |
Fixes #35265
Part of #18840