Microsoft.WinGet.Client custom assembly load context #3150
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
msftrubengu
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
This doesn't match the PowerShell example exactly. They required that the engine also be in the Dependencies directory, and from my reading of the documentation that was a very intentional thing. I get the sense that once an ALC loads a binary, it gets the first shot at loading any dependencies. Only if it fails does the Default context try, then if it also fails the Resolving event is raised.
The point is that you only want the Resolving event to be used for a binary whose name is not going to collide with any other module. The engine is the entry point for this. Then afterward, the context will be used to load all dependencies of the engine (recursively) via the context's Load.
If Resolving is allowed to load shared dependencies directly, then it is possible that multiple modules can still collide as the call order will determine which one of them wins.
I don't know how hard it will be to get things to work with the engine as an ALC dependency, but I think it is necessary. I would further suggest two directories: DirectDependencies and SharedDependencies. The direct dependencies can be loaded via the Resolving event and all have a name that is very unique (maybe it is only the engine). The shared dependencies can only be loaded via Load and contain anything that might result in a conflict. This design is of course completely dependent on my understanding of ALCs, which admittedly was only gained today.
| /// <inheritdoc/> | ||
| public void OnImport() | ||
| { | ||
| AssemblyLoadContext.Default.Resolving += WinGetAssemblyLoadContext.ResolvingHandler; |
There was a problem hiding this comment.
Why aren't we able to create our own context and only use it when the assembly load is coming from this PS module?
My main worry is that if we change the default context, then we end up in a similar situation as without it.
There was a problem hiding this comment.
I think that my overall comment has more understanding than this one I made before reading the docs.
| "SharedDependencies"); | ||
|
|
||
| private static readonly string DirectDependencyPath = Path.Combine( | ||
| Path.GetDirectoryName(typeof(WinGetAssemblyLoadContext).Assembly.Location), |
There was a problem hiding this comment.
It looks like you inverted the name suggestions that I used (completely, so functionally it should work). To me, the "shared" dependencies are the ones that might conflict (and so need to load from the ALC Load) and the "direct" ones are the ones that are directly dependencies of the main DLL (and so need to use Resolving). Feel free to use whatever names you want, other than reversing my suggestion which is confusing to me. 🫠
This PR modifies the Microsoft.WinGet.Client module to be more dependency friendly and play nice with winget configure.
It uses a custom AssemblyLoadContext that handles loading all the assembly dependencies of this module. For this, the Microsoft.WinGet.Client.Engine project was created. This project wraps all the cmdlet functionality on it and leaves the Microsoft.WinGet.Client.Cmdlets.dll (formerly Microsoft.WinGet.Client.dll) to just define the visibility of the cmdlets. The custom ALC is registered at onImport module initializer and cleaned up when the module is removed. At the end the binary layout of the module must be like this:
To avoid loading any other dlls at import module time, none of the cswinrt objects from Microsoft.Management.Deployment are exposed as parameters or outputs of the cmdlets. The engine contains PS wrappers objects and enum that are converted internally by and only by the engine.
Other fixes:
Removed the dependency on the PowerShellSDK for the PowerShell 7 version of the binary. It was used to call an API to convert json into hashtable but this wouldn't work for Windows PowerShell. Instead, I just took what PowerShell 7 does and applied it to both. We still don't support Windows PowerShell, but is a step forward into doing it.
Fixed passing a catalog package object back to the cmdlets to avoid the search. It got broken when Find-WinGetPackage and Get-WinGetPackage started returning an object instead of the cswinrt CatalogPackage object. The wrapper contains the catalog package but is not accessible to the user, so there was no way to pass it as a parameter back to Install-WinGetPackage,
Modified the PowerShell output directory for binary to follow more what nuget suggests for platform specific binaries.
