search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.36k stars 4.74k forks source link

System.Runtime.WindowsRuntime adds incorrect Binding Redirects #28603

Closed rido-min closed 4 years ago

rido-min commented 5 years ago

Repro steps:

You will notice that the .exe.config gets generated with this binding redirect:

      <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>

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.

System.IO.FileNotFoundException
  HResult=0x80070002
  Message=Could not load file or assembly 'System.Runtime.WindowsRuntime, Version=4.0.14.0, Culture=neutral, PublicKeyToken=b77a5c561934e089' or one of its dependencies. The system cannot find the file specified.

/cc @jeffschwMSFT

jeffschwMSFT commented 5 years ago

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:

Unified Dependency "System.Runtime.WindowsRuntime, Version=4.0.14.0, Culture=neutral, PublicKeyToken=b77a5c561934e089".
    Using this version instead of original version "4.0.0.0" in "C:\Program Files (x86)\Windows Kits\10\UnionMetadata\Windows.winmd" because AutoUnify is 'true'.

cc @ericstj

rido-min commented 5 years ago

This issue is still happening with Preview4 https://www.nuget.org/packages/System.Runtime.WindowsRuntime/4.6.0-preview4.19212.13 I

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: 1 - Create WPF (net472) 2 - Add Microsoft.Toolkit.Wpf.UI.XamlHost 3 - Add a packaging project to the solution 4 - F5 on wapproj

The app fails with:

System.Windows.Markup.XamlParseException
  HResult=0x80131501
  Message='Initialization of 'Microsoft.Toolkit.Wpf.UI.XamlHost.WindowsXamlHost' threw an exception.' Line number '12' and line position '35'.
  Source=PresentationFramework
  StackTrace:
   at System.Windows.Markup.WpfXamlLoader.Load(XamlReader xamlReader, IXamlObjectWriterFactory writerFactory, Boolean skipJournaledProperties, Object rootObject, XamlObjectWriterSettings settings, Uri baseUri)
   at System.Windows.Markup.WpfXamlLoader.LoadBaml(XamlReader xamlReader, Boolean skipJournaledProperties, Object rootObject, XamlAccessLevel accessLevel, Uri baseUri)
   at System.Windows.Markup.XamlReader.LoadBaml(Stream stream, ParserContext parserContext, Object parent, Boolean closeStream)
   at System.Windows.Application.LoadComponent(Object component, Uri resourceLocator)
   at WpfApp6.MainWindow.InitializeComponent() in C:\Users\rmpablos\source\repos\WpfApp6\WpfApp6\MainWindow.xaml:line 1

Inner Exception 1:
FileNotFoundException: Could not load file or assembly 'System.Runtime.WindowsRuntime, Version=4.0.14.0, Culture=neutral, PublicKeyToken=b77a5c561934e089' or one of its dependencies. The system cannot find the file specified.

Inner Exception 2:
FileNotFoundException: Could not load file or assembly 'System.Runtime.WindowsRuntime, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089' or one of its dependencies. The system cannot find the file specified.
jeffschwMSFT commented 5 years ago

@rido-min can you try referencing the win10 meta contracts from your main project?

rido-min commented 5 years ago

@jeffschwMSFT I tried referencing the contracts and the interop packages from the main project but I still got the same FileNotFound exception

ericstj commented 5 years ago

Is NuGet writing the bindingRedirect to the source app.config? That could be getting in your way.

jeffschwMSFT commented 5 years ago

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?

ericstj commented 5 years ago

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

ericstj commented 5 years ago

Also can folks clarify if this is happening using Packages.config, PackageReference, or both.

rido-min commented 5 years ago

PackageReference. The contracts package uses refs that are not supported with packages.config

ericstj commented 5 years ago

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.

    <dependencies>
      <group targetFramework=".NETFramework4.6.2">
        <dependency id="Microsoft.Windows.SDK.Contracts" version="10.0.18362.2002-preview" exclude="Build,Analyzers" />
      </group>
      <group targetFramework=".NETCoreApp3.0">
        <dependency id="Microsoft.Windows.SDK.Contracts" version="10.0.18362.2002-preview" exclude="Build,Analyzers" />
      </group>

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.

azchohfi commented 5 years ago

The specific line of code we use to reference the SDK Contracts is this: https://github.com/windows-toolkit/Microsoft.Toolkit.Win32/blob/dea259fa43e19382740f341b1b2b46b79af5b10e/Directory.Build.targets#L10 It feels weird to me that this is being excluded by default. I could happily add the PrivateAssets="analyzers", but it feels to me that this is something else.

clairernovotny commented 5 years ago

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.

michael-hawker commented 5 years ago

Is 2017 supported for this scenario? The .NET Core Previews have this warning calling out to use VS 2019:

image

It's also called out in the announcement blog post for preview4.

clairernovotny commented 5 years ago

Core isn’t but wasn’t this issue about targets that affect the . NET Framework?

ericstj commented 5 years ago

The contracts package uses refs that are not supported with packages.config

Yeah, but it also has targets which can do the same thing as ref on packages.config... Granted that too will depend on targets, ironically packages.config always applies targets from transitive dependencies so it won't be impacted by this issue.

wasn’t this issue about targets that affect the . NET Framework?

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.

clairernovotny commented 5 years ago

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.

ericstj commented 5 years ago

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.

azchohfi commented 5 years ago

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. @rido-min what do you think?

ericstj commented 5 years ago

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. \ First step should be for Microsoft.Toolkit.UI.XamlHost to remove exclude="build".

Second step would be for both Microsoft.Windows.SDK.Contracts and System.*.WindowsRuntime.* packages to duplicate all their build assets in buildTransitive folder.

azchohfi commented 5 years ago

https://www.nuget.org/packages/Microsoft.Toolkit.Wpf.UI.XamlHost/6.0.0-preview4.1

ericstj commented 5 years ago

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?

jeffschwMSFT commented 5 years ago

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?

ericstj commented 5 years ago

Yep, that will do it. I double checked that using <PackageReference Include="Microsoft.Toolkit.Wpf.UI.XamlHost" Version="6.0.0-preview4.1" /> fixed the original bug.

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.