-
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
System.Runtime.WindowsRuntime adds incorrect Binding Redirects #28603
Comments
Thanks for the repro. The design of the System.Runtime.WindowsRuntime package is that it 'avoids' RAR and the resulting unification entry. In this case, a library has the reference to System.Runtime.WindowsRuntime which when referenced from the main app is not bypassing RAR and the unification is occurring. Here is the relevant bit from the log:
cc @ericstj |
This issue is still happening with Preview4 We still have the wrong binding redirect <runtime>
<assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
<dependentAssembly>
<assemblyIdentity name="System.Runtime.WindowsRuntime" publicKeyToken="b77a5c561934e089" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-4.0.14.0" newVersion="4.0.14.0" />
</dependentAssembly>
</assemblyBinding>
</runtime> Repro steps: The app fails with:
|
@rido-min can you try referencing the win10 meta contracts from your main project? |
@jeffschwMSFT I tried referencing the contracts and the interop packages from the main project but I still got the same FileNotFound exception |
Is NuGet writing the bindingRedirect to the source app.config? That could be getting in your way. |
RAR is writing the bindingRedirect. From what I can see the System.Runtime.WindowsRuntime nuget packageReferences are not being used, and instead the version from the GAC. As a result, the props to ensure the bindingRedirects are not inserted are not run. This package has 3 layers of packageReferences, is there anything that we can do to ensure it references the preferred S.R.WR? |
Using the one from the GAC would only happen if something better wasn't found. I would expect these packages to provide something better. Even if the RAR suggests the redirect, I thought you remove it via targets. 3 layers shouldn't matter. Your package should still be installed to the project and its targets should be used. Is that not happening? /cc @joperezr |
Also can folks clarify if this is happening using Packages.config, PackageReference, or both. |
PackageReference. The contracts package uses |
Ok, then this really doesn't jive. Took a look at the package heirarchy and found out why. Microsoft.Toolkit.UI.XamlHost is specifying exclude=build on the the SDK.Contracts package, and therefore transitively on its dependencies. This causes the targets not to apply.
This needs to be fixed in the Microsoft.Toolkit.UI.XamlHost package. @rido-min the reason you didn't see a direct reference fix the problem is due to an incremental build issue with MSBuild: https://github.com/Microsoft/msbuild/blob/adb180d394176f36aca1cc2eac4455fef564739f/src/Tasks/Microsoft.Common.CurrentVersion.targets#L2215-L2231 If you happen to delete the config file and rebuild after directly referencing the packages that workaround it. Regardless, the bug here is in Microsoft.Toolkit.UI.XamlHost. @rido-min can you hook up the owners of that package with this analysis? Action item for them is do not specify ExcludeAssets on their packageReference. It could be the SDK that's doing that for them, they should turn that off. I believe you can do this by specifying PrivateAssets="none" (or PrivateAssets="analyzers") on the pacakgeReference. |
The specific line of code we use to reference the SDK Contracts is this: |
Build is excluded by default by NuGet and it doesn’t work well anyway. The better answer is to use buildTransitive, but that’s currently 2019 only. There’s an open issue tracking a back port. |
Is 2017 supported for this scenario? The .NET Core Previews have this warning calling out to use VS 2019: It's also called out in the announcement blog post for preview4. |
Core isn’t but wasn’t this issue about targets that affect the . NET Framework? |
Yeah, but it also has targets which can do the same thing as
Correct, we want this stuff to work downlevel & in desktop projects. I'm not opposed to duplicating the folders, though it seems a little unusual. I'd still like to get the package heirarchy fixed so that the packages that distribute these don't suppress meaningful functionality in their dependencies. |
IIRC, the problem with Build is that it requires all consuming packages to declare their dependency correctly, with non-default options. It’s not something the source package alone can fix. |
Also, looks like we aren't the only broken dependency here, Microsoft.Windows.SDK.Contracts itself also uses targets that are being suppressed. @onovotny you're correct in general for packages which need to persist as dependencies. At least for the CoreFx packags we're talking about here, they are more like an SDK. I'd expect any project that wants to use this functionality would bring in a reference to some "Entry point" package that represents the SDK used to be able to target WinRT. I don't think its neccesarrily intuitive for some library to transitively bring in WinRT support into an otherwise vanilla project. Maybe @rido-min has a spec for this experience. |
If they also have this transient dependency issue, we need to wait on them to push a new version to push an update on our side, since we depend on them. I don't think it would make sense for us to depend on the glue assemblies just to fix their issue. |
They don't have an issue, they do the right thing WRT these packages. I was pointing out that the bug in Microsoft.Toolkit.UI.XamlHost was resulting in both the System.*.WindowsRuntime.* packages and Microsoft.Windows.SDK.Contracts to lose their targets. Second step would be for both Microsoft.Windows.SDK.Contracts and System.*.WindowsRuntime.* packages to duplicate all their build assets in buildTransitive folder. |
Thank you for this fix @azchohfi. @jeffschwMSFT remaining work is for us to duplicate build folder into buildTransitive. @rido-min can you let the owners of Microsoft.Windows.SDK.Contracts know they need to do the same? |
Thanks for tracking this down @ericstj et. al. Just so I am clear on what is needed for this issue.. I need to duplicate the contents of 'build' into a folder called 'buildTransitive'? Correct? |
Yep, that will do it. I double checked that using To validate the fix, we can create some library which depends on S.R.WinRt, pack it, then restore from desktop project using packagereference without directly referencing the S.R.WinRT packages and make sure it doesn't get the bindingRedirects. |
Repro steps:
You will notice that the .exe.config gets generated with this binding redirect:
And since in your .NuGet package you are not copying the System.Runtime.WindowsRuntime.dll locally this will fail as it cannot find the assembly from the NuGet package.
/cc @jeffschwMSFT
The text was updated successfully, but these errors were encountered: