-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Use no-op global assembly cache helper when running under .net core #69936
Conversation
@tmat mind taking a look at this? Since it looks like you were the last one to actually modify these |
@@ -27,7 +25,9 @@ private static GlobalAssemblyCache CreateInstance() | |||
} | |||
else | |||
{ | |||
return new ClrGlobalAssemblyCache(); | |||
return System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription.Contains(".NET Framework") |
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.
if there is a preferred way to do this let me know.
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.
I think this is fine
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.
Believe this is the preferred way. Pattern occurs several places in our code base.
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.
@dotnet/roslyn-compiler could use a couple reviews on the shared file changes |
src/Scripting/Core/Hosting/Resolvers/DotNetCoreGlobalAssemblyCache.cs
Outdated
Show resolved
Hide resolved
src/Scripting/Core/Hosting/Resolvers/DotNetCoreGlobalAssemblyCache.cs
Outdated
Show resolved
Hide resolved
@@ -27,7 +25,9 @@ private static GlobalAssemblyCache CreateInstance() | |||
} | |||
else | |||
{ | |||
return new ClrGlobalAssemblyCache(); | |||
return System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription.Contains(".NET Framework") |
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.
Believe this is the preferred way. Pattern occurs several places in our code base.
/// However it isn't extremely straightforward to implement and we don't need it at this time so leaving it as a no-op. | ||
/// More info on how this might be possible under a .net core runtime can be found https://github.com/dotnet/core/issues/3048#issuecomment-725781811 | ||
/// </summary> | ||
internal sealed class DotNetCoreGlobalAssemblyCache : GlobalAssemblyCache |
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.
Should we just make this type a singleton vs. creating a new one every time?
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.
Going to leave this as-is - this is already created as a singleton instance, as instantiated by the GlobalAssemblyCache.CreateInstance
in the parent.
Should fix dotnet/vscode-csharp#6235
The
ClrGlobalAssemblyCache.cs
is used to find the implementation location of .net framework based assemblies that might be in the global assembly cache in order to decompile them.ClrGlobalAssemblyCache.cs
is not valid to use as-is when running in a .net core runtime - it is not valid to [DllImport("clr")] - in fact this generally throws a dll not found exception (it gets caught and reported as a NFW). I'm guessing in the above issue it managed to get a bit farther before throwing.The solution here is to avoid calling
ClrGlobalAssemblyCache
when in .net core. I replaced it with a no-op implementation. While at some point it may be valuable to support decompiling .net framework assemblies in vscode, its not a high priority scenario and its not entirely clear on how that would be implemnted.