-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add System.Reflection.PathAssemblyResolver support for 'retargetable' assemblies #30663
Comments
We are trying to address dotnet/wpf#1451 (potentially for 3.0). @ryalanms thinks that we'd need our own implementation of a This seems more suitable for /cc @rladuca |
Can you wrap PathAssemblyResolver? |
Yes, we could but PathAssemblyResolver has private members, and the wrapper would need to keep track of the assemblyPaths and have similar matching logic and error handling. |
Maybe a better way to phrase this is that PathAssemblyResolver ought to use a resolution policy that is similarly permissive as the normal load path. Presumably that normal load path handles retargetable assemblies correctly. @vitek-karas |
It also looks like RoAssemblyName doesn't support AssemblyName.Flags, which includes AssemblyNameFlags.Retargetable. |
I have a prototype at https://github.com/steveharter/dotnet_corefx/tree/RetargetableAsm It adds support for Due to the Thus we need to determine if this meets the bar for 3.0 -- how common is this scenario? |
Thanks, @steveharter. This looks great. Any applications ported from .NET Framework to .NET Core that rely on older versions of NuGet packages may hit this, so I believe it is a common scenario. |
@ericstj thoughts on how common this is going to be, and whether this will become a common enough issue during migration? |
Short-term, I'll get the PR out for master today. |
Can you clarify what is special about these? Desktop assemblies don't rely on retargetable, so this must only be for v1 PCLs that used the Silverlight key? I suspect there are still some of these out there, but I don't have a read on how many: I know all the ones we produced are superseded with newer contract-based versions which don't rely on retargetable. Seems like something we could measure with the right data. I know we've scanned packages for similar sorts of things in the past. /cc @terrajobst |
That is my understanding. I can create such an assembly (Retargetable mscorlib reference with version=2.0.5.0 PublicKey=7cec85d7bea7798e) with VS 2017 and a Universal Windows class library. At compile-time, a reference assembly like this is used: I don't think this would be a common scenario, so I'm also interested to know how older NuGet packages could influence this. |
Adding a few links:
|
Note a potential temporary work-around would be to create a custom resolver that checks for the presence of that reference and treat it as "relocatable":
|
Yes, the older versions of problem NuGet packages all contained assemblies were building against 2.0.5.0; it wasn't anything specific to NuGet. I wasn't aware 2.0.5.0 was Silverlight and that no Desktop versions of mscorlib use 'retargetable'. If that is the case, adding a special-case for 2.0.5.0 to a PathAssemblyResolver wrapper in WPF seems reasonable. |
I believe this will be the case for more than just mscorlib. The following assemblies are part of v1 portable and rely on retargetable.
Also, PKT is @steveharter I was actually thinking that the run-time ignores PKT completely in .NETCore. I vaguely remember actually tooling bugs that resulted because of this (runtime was working, but tooling was failing). If that's true should PathAssemblyResolver do the same? |
Yes perhaps we should just ignore PKT in PathAssemblyResolver. However usage under .NETFx may not work as expected for some edge cases, so this may be considered a breaking change (?). Perhaps it can be an option in a new constructor to ignore the PKT. Ignoring PKT means that we will just pick the highest version for a given simple name and don't support loading the same assembly (by name) more than once. However someone could author their own custom resolver and "add back" that PKT logic if necessary. Also the original design was to emulate what we do at build-time, using CSC as a reference implementation -- thus favor assemblies with PKT over those without and support references to different assemblies with the same simple name (if signed \ contains a PKT). The potential workaround now is to create a custom resolver and just ignore PKT in all cases: public override Assembly Resolve(MetadataLoadContext context, AssemblyName assemblyName)
{
Assembly result = null;
if (_fileToPaths.TryGetValue(assemblyName.Name, out List<string> paths))
{
foreach (string path in paths)
{
Assembly assemblyFromPath = context.LoadFromAssemblyPath(path);
AssemblyName assemblyNameFromPath = assemblyFromPath.GetName();
if (assemblyName.Name.Equals(assemblyNameFromPath.Name, StringComparison.OrdinalIgnoreCase))
{
// Pick the highest version.
if (result == null || assemblyNameFromPath.Version > result.GetName().Version)
{
result = assemblyFromPath;
}
}
}
}
return result;
} |
Good point on desktop, I had forgotten that this component works on desktop as well. I'm ok with the change to honor retargetable. As far as workaround vs fix, I'd actually feel better about fixing the right way, given WPF is one of the primary consumers of MetadataLoadContext. Either way we need to put in a code change, so I'd prefer that change not be a hack. My inclination would be to fix in MetadataLoadContext or no fix at all. Let me double check with tactics. |
👍 |
I brought this up in tactics and their indication was won't fix for now and consider for 3.1 if we get more feedback on this blocking multiple packages. |
Note that master currently has the fix explained earlier for retargetable assemblies. |
It seems to block a lot of legacy WPF apps when porting them to core. And as it stands now there are no workaround to fix it, since the nuget packages causing this usually are quite old. This also worked in preview3 and earlier. So my Netling app is already ported and working. As long as I compile it with the old preview SDK. |
I took a look at Netling and it looks like it's failing due to a dependency on "OxyPlot.Wpf". I took a look at this package on NuGet.org and the version you depend on is actually unlisted. A newer version (by date) 1.0.0 is available. If I change your project to use this newer version it works: - <PackageReference Include="OxyPlot" Version="2014.1.546.0" />
- <PackageReference Include="OxyPlot.Wpf" Version="2014.1.546.0" />
+ <PackageReference Include="OxyPlot.Wpf" Version="1.0.0" /> To workaround it you can avoid passing the reference to the XAML compiler, though this might not always be possible. If more folks hit it let us know about the old package you are using. If it turns out to be a bigger issue than folks are assuming we can re-evaluate pulling the fix into 3.0. |
Given the (lack of) adoption of PCLs on NuGet, it seems @ericstj's suggestion should work for most components that are likely used by WPF apps. Postponing this until we see more evidence for this seems reasonable. Bringing all the complexity of PCLs v1 doesn't seem worth it at this point. |
Closing as this is fixed in master and currently "won't fix" for 3.0. |
the issue is still there in .netcore 3.1 ... as @ericstj said it occures on please advise |
The fix was taken in servicing for 3.1; see dotnet/corefx#42768. To get it you'll need the latest servicing build of 3.1.2. @mmitche / @Anipik is this available yet? |
no, 3.1.2 is not available yet |
.assembly extern retargetable mscorlib
{
.publickeytoken = (7C EC 85 D7 BE A7 79 8E ) // |.....y.
.ver 2:0:5:0
}
If any assemblies loaded by
MetadataLoadContext
were built against some older versions of mscorlib,PathAssemblyResolver
will fail to match any newer versions of mscorlib present inassemblyPaths
, due to a public key token mismatch.PathAssemblyResolver
already supports taking the most recent version for target assemblies with an empty public key token.https://github.com/dotnet/corefx/blob/master/src/System.Reflection.MetadataLoadContext/src/System/Reflection/PathAssemblyResolver.cs#L80
Add support for most-recent assembly match when assembly being resolved is 'retargetable'.
The text was updated successfully, but these errors were encountered: