Skip to content

Refactor PowerShell Core to use the default CoreCLR loader instead #3903

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

Merged
merged 4 commits into from
Jun 6, 2017

Conversation

daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Jun 1, 2017

Partially fix #3649

Issue Summary

The API AppDomain.GetAssemblies is brought back in .NET Core 2.0 which returns the loaded assemblies from the default loader. Therefore it's possible now for powershell to just depend on the default CoreCLR loader without having our own assembly load context getting in the picture. This would greatly simplify the scenario of hosting powershell in applications.

Fix

Remove the code that spins up our own assembly load context. Keep the code that registers our Resolve method to the default loader's Resolving event, so that we can continue to do special assembly resolution as needed (such as the GAC probing logic needed for consuming FullCLR PS modules).

Essentially, the assembly Microsoft.PowerShell.CoreCLR.AssemblyLoadContext.dll is not needed anymore, the remaining code should be moved to S.M.A.dll. However, that will break DSC and other native hosts that are hosting powershell. So this assembly is kept for now.

CoreFX Fixes needed

https://github.com/dotnet/corefx/issues/18989
https://github.com/dotnet/corefx/issues/18877
https://github.com/dotnet/corefx/issues/18791

This PR is not blocked by those CoreFX issues, but it's incomplete until we have the fixes. Those issues have already been addressed, and #3887 will get those fixes for us.
Note that this PR is NOT blocked by #3887.

Follow-up work

This PR addresses the first task listed in #3649. Three tasks are remaining.

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with 1 comment. Probably we need not worry about it right now, as this code might be merged into SMA.dll in the future.

@@ -31,11 +31,7 @@ internal partial class PowerShellAssemblyLoadContext : AssemblyLoadContext
// When the first attempt fails, we again need to retrieve the resource string to construct another exception, which ends up with an infinite loop.
private const string BaseFolderDoesNotExist = "The base directory '{0}' does not exist.";
private const string ManifestDefinitionDoesNotMatch = "Could not load file or assembly '{0}' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference.";
private const string AssemblyPathDoesNotExist = "Could not load file or assembly '{0}' or one of its dependencies. The system cannot find the file specified.";
private const string InvalidAssemblyExtensionName = "Could not load file or assembly '{0}' or one of its dependencies. The file specified is not a DLL file.";
private const string AbsolutePathRequired = "Absolute path information is required.";
private const string SingletonAlreadyInitialized = "The singleton of PowerShellAssemblyLoadContext has already been initialized.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the localization story for these messages?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response. These strings are currently not localized because the loading of a satellite resource DLL when a loading exception happens causes a recursion. It's explained in the comment above the first resource string. Eventually, after moving the remaining code from PowerShellAssemblyLoadContext.dll to SMA.dll, we can reconsider the localization.

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Jun 6, 2017

Minor update to add back Microsoft.PowerShell.CoreCLR.AssemblyExtensions.LoadFrom(string assemblyPath) for the short term. This is because PackageManagement depends on this API and thus removing this API will instantly break PackageManagement.

Since the LoadFrom functionality has been removed from our assembly load context, we just call Assembly.LoadFrom(string) in the above API. I also added ObsoleteAttribute to this API to prevent new code from using it. @brywang-msft is working on migrating PackageManagement to .NET Core 2.0. Once that work is done, the above API will be permanently removed from PowerShell Core. This is added to the follow-up tasks in the issue.

I have verified that PackageManagement works with this update.
I also verified that host powershell in an application is the same as in FullCLR after this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid using powershell assembly load context with netcoreapp2.0
4 participants