dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.72k stars 1.07k forks source link

MSBuild missing automatic binding redirects when using NuGet transitive pinning #40284

Open AArnott opened 1 year ago

AArnott commented 1 year ago

Issue Description

MSBuild doesn't produce binding redirects automatically for indirect nuget dependencies, leading to runtime failure of my code.

Steps to Reproduce

git clone https://github.com/microsoft/vs-servicehub.git
cd vs-servicehub
git checkout 7659b150cd580d35f3751b33341bcb50f63c32dd
Microsoft.ServiceBroker.sln

Within VS, run the Microsoft.ServiceHub.Analyzers.Tests (net472) tests.

Expected Behavior

I expect them to pass (as they do at the command line).

Actual Behavior

Most of the tests fail with this error:

System.TypeInitializationException : The type initializer for 'Microsoft.CodeAnalysis.Testing.ReferenceAssemblies' threw an exception. ---- System.IO.FileLoadException : Could not load file or assembly 'NuGet.Packaging, Version=5.6.0.5, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)

Analysis

The Directory.Packages.props file forces the NuGet.Protocol transitive dependency to 6.4.0, where it would naturally be in the 5.x range. Because this is not a direct dependency, and the indirect dependency holds PrivateAssets="compile" on it such that it isn't passed to my compiler (or RAR?), msbuild doesn't produce the necessary binding redirect.

When I add my own top-level dependency on NuGet.Protocol, such that it goes to RAR, the binding redirect is created and the tests pass.

I believe MSBuild should be enhanced to be aware of transitive dependencies so that it can produce the necessary binding redirects.

Versions & Configurations

.NET SDK 7.0.101 Dev17.5

AArnott commented 1 year ago

This might be related: https://github.com/NuGet/Home/issues/10505

rainersigwald commented 1 year ago

@jeffkl is this what you would expect from transitive-pinned dependency? It is very surprising to me.

KalleOlaviNiemitalo commented 1 year ago

https://github.com/dotnet/sdk/issues/2547 is another case of missing binding redirects.

jeffkl commented 1 year ago

When I add my own top-level dependency on NuGet.Protocol, such that it goes to RAR, the binding redirect is created and the tests pass.

@AArnott what if you add a top-level dependency with PrivateAssets=Compile? Wouldn't the assembly also not get passed to MSBuild for binding redirect information?

AArnott commented 1 year ago

@jeffkl No. This top-level reference works just as well at producing the required binding redirect:

<PackageReference Include="NuGet.Protocol" PrivateAssets="compile" />

I couldn't tell you why, as I don't know the RAR system that deeply. But I know it's more complicated than just directly referenced assemblies.

sharwell commented 1 year ago

I just fixed another one this morning: https://github.com/dotnet/roslyn-analyzers/pull/6469

jeffkl commented 1 year ago

Just an update, I am actively investigating a fix but am on call this week. I'll let you know when I've made progress on this.

jeffkl commented 1 year ago

I think I have fix ready for all of the transitive pinning dependency flow problems: https://github.com/NuGet/NuGet.Client/pull/4953

AArnott commented 1 year ago

That's great, @jeffkl. Do you think it will resolve my missing binding redirect issue?

AR-May commented 1 year ago

Team triage: closing as a duplicate

sharwell commented 1 year ago

@AR-May It's difficult to understand if the linked item will truly resolve this. In order for binding redirects to be generated, the change would need to impact the inputs to ResolveAssemblyReference such that its SuggestedRedirects output includes consideration for assemblies that are transitive dependencies of the current project but are not included as references of the current project.

I updated the resolution state to reflect that this was marked as a duplicate of external issue NuGet/NuGet.Client#4953.

AArnott commented 1 year ago

Several bugs have been linked from this one. Which one is the alleged duplicate, @ar-may? I share @sharwell's concerns.

rainersigwald commented 1 year ago

https://github.com/NuGet/Home/issues/12270 is about transitive assets flowing incorrectly through transitive-pinned packages and looks like the canonical issue to me.

AArnott commented 1 year ago

@rainersigwald Thanks for looking. But 12270 appears to be a distinct issue. That one talks about how PrivateAssets should be aggregated when it appears along multiple paths. This issue is about how even a single path with PrivateAssets="compile" creates a problem because RAR doesn't ever see the reference assembly, even when CPVM changes the assembly version, such that no binding redirect is created for it, although CPVM creates a need for it.

AndreasClassen commented 6 months ago

Hi @AArnott. Did you find a solution to your problem? We are experiencing the same issue.

We use exclude="compile" inside our own NuGet packages to limit the publicly visible API surface to that of the packaged library itself. This is also what Microsoft recommends in the documentation.

This leads to PrivateAssets="compile" being added to the PackageReference inside the project file.

That in turn leads to the problematic behavior of msbuild that you describe in your initial post: msbuild does not add binding redirects for transitive dependencies.

In our case, we have a project that references two NuGet Packages: foo and bar. The package foo has a dependency on a NuGet Package CoreWcf.Primitives with exclude="compile" set. The package bar also has a dependency on CoreWcf.Primitives, but with another version (also with exclude="compile" set). In the project that references both foo and bar, no binding redirects for CoreWcf.Primitives will be generated. This leads to a runtime exception at some point.

A workaround, quite ugly, is to reference CoreWcf.Primitives directly from the project. Another workaround is to remove the exclude="compile" from the foo and bar packages.

AArnott commented 6 months ago

No, I haven't spent any more time on this issue.

rainersigwald commented 6 months ago

Reactivating (I appear to have missed the ping last year). Moving to SDK since I think this is about what happens to the asset between restore (create project.assets.json) and RAR, which is handled in SDK tasks (for .NET SDK projects).