Handle satellite assembly loading in .NET Core #70769
Conversation
This fixes a bug where-by the compiler did not load satellite assemblies properly on Unix and Linux. The compiler recently changed (dotnet#69406) our loading strategy there to use `AssemblyLoadContext.LoadFromStream`. The runtime fallback logic for loading satellite assemblies is effectively the following: 1. Call `AssemblyLoadContext.Load(satelliteAssemblyName)` 2. If that fails call `AssemblyLoadContext.Load(realAssemblyName)` for the real assembly 3. Look at `Assembly.Location` on the return and change it to have the appropriate satellite path. 4. Call `AssemblyLoadContext.LoadFromPath(...)` on the result of (3) That logic breaks when assemblies are loaded via `LoadFromStream` as there is no location information. This changes our `AssemblyLoadContext` implementation to just handle satellite loads in step (1). This is done irrespective of whether the loader is loading via path or via stream. The loader has the information already so simpler to just handle it the same way every time. closes dotnet#56708 Note: these are [the docs](https://learn.microsoft.com/en-us/dotnet/core/dependency-loading/loading-resources) describing satellite assembly loading.
dotnet-issue-labeler
bot
added
the
untriaged
|
@dotnet/roslyn-compiler, @jjonescz, @RikkiGibson PTAL |
| @@ -128,59 +129,41 @@ private void DeleteLeftoverDirectories() | |||
| } | |||
| } | |||
|
|
|||
| protected override string PreparePathToLoad(string originalFullPath) | |||
| protected override string PreparePathToLoad(string originalAnalyzerPath, ImmutableHashSet<string> cultureNames) | |||
There was a problem hiding this comment.
For this file I recommend just looking at the new code, not the diff. The code is pretty straight forward.
| var rm = new ResourceManager("rmc", typeof(Util).Assembly); | ||
| _ = rm.GetString("hello", ci); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
It's needed because the resource doesn't actually exist. This code is just getting the runtime to probe for the satellite assemblies to prove that we do the loading correct. So the satellite load will succeed then it will throw when it can't find the resource.
Getting a real resource was going to involve checking in a couple of binary resources that I couldn't think of an easy way to maintain. As such decided to just focus on testing the load portion here.
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs
Outdated
Show resolved
Hide resolved
| // loader has a mode where it loads from Stream though and the runtime will not handle | ||
| // that automatically. Rather than bifurate our loading behavior between Disk and | ||
| // Stream both modes just handle satellite loading directly | ||
| if (!string.IsNullOrEmpty(assemblyName.CultureName) && simpleName.EndsWith(".resources", StringComparison.Ordinal)) |
There was a problem hiding this comment.
Is ordinal comparison correct? Shouldn't it ignore case?
There was a problem hiding this comment.
There are unfortunately no right answers to this. Whichever you pick can be made wrong. Generally though in the compiler we try and use case insensitive when searching for files but case sensitive when actually looking at files in hand.
Co-authored-by: Jan Jones <[email protected]>
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs
Show resolved
Hide resolved
|
@RikkiGibson PTAL |
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Rikki Gibson <[email protected]>
ghost
added this to the 
This fixes a bug where-by the compiler did not load satellite assemblies properly on Unix and Linux. The compiler recently changed (#69406) our loading strategy there to use
AssemblyLoadContext.LoadFromStream. The runtime fallback logic for loading satellite assemblies is effectively the following:AssemblyLoadContext.Load(satelliteAssemblyName)AssemblyLoadContext.Load(realAssemblyName)for the real assemblyAssembly.Locationon the return and change it to have the appropriate satellite path.AssemblyLoadContext.LoadFromPath(...)on the result of (3)That logic breaks when assemblies are loaded via
LoadFromStreamas there is no location information.This changes our
AssemblyLoadContextimplementation to just handle satellite loads in step (1). This is done irrespective of whether the loader is loading via path or via stream. The loader has the information already so simpler to just handle it the same way every time.closes #56708
Note: these are the docs describing satellite assembly loading.