Reference transitive not working when app and libs has different targetFramework

[yyjdelete]

Maybe two issues, and not sure where to report it(microsoft/msbuild, dotnet/sdk, dotnet/corefx, dotnet/standard; dotnet/project-system, visualstudio; etc?), so copy it from dotnet/sdk#3103.

Issue Title

Reference transitive not working when app and lib has different targetFramework, and the lib use an transitive reference from special targetFramework of an multi-targetFramework PackageReference/ProjectReference

General

Tested with dotnet-sdk : 3.0.100-preview3-010431, 2.1.504 and some other version

Run ConsoleApp5 in https://github.com/yyjdelete/test_dotnet_indirect_dependency
And see an FileNotFoundException for System.Data.SqlClient, Version=4.5.0.0(Ignore the MissingMethodException, as it's expected). The same if replace ClassLibrary1 and ClassLibrary2 with packed .nupkg

Project struct:

ConsoleApp5(net472)
|--ClassLibrary1(netstandard2.0)->use `System.Data.SqlClient` directly or exported as public api of `ClassLibrary2`
---|--ClassLibrary2(netstandard2.0;net472)
------|--System.Data.SqlClient(when netstandard2.0)

Expected:

ConsoleApp5(net472)
|--ClassLibrary1(netstandard2.0)
---|--System.Data.SqlClient(net472)
---|--ClassLibrary2(net472)

1. The System.Data.SqlClient should be include in the output bins of ConsoleApp5, if used by ClassLibrary1(netstandard2.0).
   Or at least give an warning, to make people know which Reference must be added explicitly. It's almost impossibe to find all of them by hand in an project with deep reference transitive(can also be something in nupkg).

2. VisualStudio should show the same result of project struct as the output does, and System.Data.SqlClient should be include again with ClassLibrary1(netstandard2.0) if it's removed with ClassLibrary2(net472)
   I already know it's reasonable to reference ClassLibrary2 with net472(runtime) instead of netstandard2.0, so ignore the MissingMethodException.

Actual:

ConsoleApp5(net472)
|--ClassLibrary1(netstandard2.0)
---|--ClassLibrary2(net472)

***System.Data.SqlClient is missing

1. The System.Data.SqlClient is missing from the output, even it's used by ClassLibrary1(netstandard2.0), and get an FileNotFoundException when execute.
2. VisualStudio show that ConsoleApp5(net472) use ClassLibrary2(net472), while the output use ClassLibrary2(netstandard2.0).
   

[carlossanlop]

@joperezr @ericstj is this something you could look into?
Also mentioning @divega since the problem is happening in the System.Data namespace.
Note: a duplicate issue was also opened in the dotnet/standard repo, so feel free to close one of them: dotnet/standard#1149

[joperezr]

@yyjdelete Does your ConsoleApp5.csproj have a PackageReference? If not try adding the following to that project which should fix this issue:

<PropertyGroup>
  <RestoreProjectStyle>PackageReference</RestoreProjectStyle>
</PropertyGroup>

After that, do a msbuild CnosoleApp5.csproj /t:Restore and msbuild ConsoleApp5.csproj /t:rebuild and you should see the right dependencies flowing and placed correctly. If that's not the case, ping me and I can help.

[joperezr]

This blog post contains more info on this problem and the resolution I provided above: https://www.hanselman.com/blog/ReferencingNETStandardAssembliesFromBothNETCoreAndNETFramework.aspx

[yyjdelete]

@joperezr It's not the case.
And still the same after added <RestoreProjectStyle>PackageReference</RestoreProjectStyle>.

net472 app
|- netstandard2.0 only library
|- an multi target library(like dapper or your self build library, which reference nothing for net472 and System.Data.SqlClient/System.Data.Common for ns2)

So you get System.Data.SqlClient(ns2.0) in your ns2.0 only library with reference transitive, and can use it in code or as an part of public apis. But the net472 app only see Dapper(net451), and don't think it need any version of System.Data.SqlClient, even the ns2.0 only library reference it. So finally, the build success without any warns and the reference to System.Data.SqlClient is stripped silently (which makes it almost impossible to check and fix manually), but the facades assembly(TypeForward) of System.Data.SqlClient won't be added to net472 app, and get an FileNotFoundException during running.

[joperezr]

Can you share an binlog of your build so that we can investigate a bit more? to produce a binlog run the following command on a developer command prompt: msbuild yourSolution.sln /t:rebuild /bl which will generate a msbuild.binlog file. Paste that here so that we can take a look and figure out what is wrong.

[yyjdelete]

@joperezr
msbuild_binlog.zip
And the projects can be see at https://github.com/yyjdelete/test_dotnet_indirect_dependency
msbuild.binlog is generated with msbuild.exe and msbuild_dotnet-sdk.binlog with dotnet msbuild

[joperezr]

I see, I'm able to repro your issue. It is very weird, if I remove the condition on the ClassLibrary2.csproj PackageReference to SqlCient so that it is also referenced when building the .NET Framework version, then everything work as expected. I also analyzed the project.assets.json files that were produced on the restore, and looks like the SqlClient dependency does flow correctly from ClassLibrary2 to ClassLibrary1 but doesn't make it to ConsoleApp5. @dsplaisted do you mind taking a look to the repro repository that @yyjdelete pointed out above? Is this a NuGet bug causing the dependencies to not flow?

[dsplaisted]

@nkolev92 I think this is a NuGet bug, can you take a look?

The issue is there is a .NET Framework app which references a .NET Standard Library, which references a library multi-targeted to .NET Standard and .NET Framework which conditionally references System.Data.SqlClient only for .NET Standard. The System.Data.SqlClient doesn't seem to be flowing through to the .NET Framework app, even though it ends up consuming the .NET Standard version of the multi-targeted library.

Repro is here: https://github.com/yyjdelete/test_dotnet_indirect_dependency

[nkolev92]

I think this is expected/by design as far as NuGet is concerned.

There could in theory be 2 different edges to ClassLibrary2, that come through different target framework pivots, so for that reason, NuGet always selects the best matching for that particular project/package in an isolated fashion.

[joperezr]

@nkolev92 correct me if I'm wrong but if the references to ClassLibrary1 and 2 were via NuGet packages and not ProjectReferences, I believe the right dependency would be detected and would flow correctly. If so, it doesn't seem to be the correct behavior in case that was by design.

[carlossanlop]

Hey @nkolev92 do you have a follow up on @joperezr 's last comment?

[nkolev92]

correct me if I'm wrong but if the references to ClassLibrary1 and 2 were via NuGet packages and not ProjectReferences, I believe the right dependency would be detected and would flow correctly. If so, it doesn't seem to be the correct behavior in case that was by design.

I verified that, and it does not seem to be the case.
I see the following in the targets section of my assets file

  "targets": {
    ".NETFramework,Version=v4.7.2": {
      "ClassLibrary1/1.0.0": {
        "type": "package",
        "dependencies": {
          "ClassLibrary2": "1.0.0"
        },
        "compile": {
          "lib/netstandard2.0/ClassLibrary1.dll": {}
        },
        "runtime": {
          "lib/netstandard2.0/ClassLibrary1.dll": {}
        }
      },
      "ClassLibrary2/1.0.0": {
        "type": "package",
        "compile": {
          "lib/net472/ClassLibrary2.dll": {}
        },
        "runtime": {
          "lib/net472/ClassLibrary2.dll": {}
        }
      }
    },
}

Let me know if you do see something different, but I think NuGet's behavior is as expected here.

[ericstj]

So the problem here is that the ClassLibrary1 is relying on the transitive dependency from ClassLibrary2 to deliver SQL client.

ClassLibrary2 is actually malformed to remove compile-time dependencies from a compatible target framework. A library need to consider both the API it exposes, and any packages it depends on as part of its public contract. This is the reason why we are very careful in CoreFx to choose which packages we expose to the compiler vs just reference for runtime consumption.

We had a similar problem in CoreFx years back and specifically made our packaging infra catch this and promote those dependencies. Here's that commit: dotnet/buildtools@98119a0. Now the code lives in arcade and has gone through a few iterations to handle other cases but it still addresses this type of issue.

NuGet could do a couple things here:

1. Add validation whenever you create a dependency group that is compatible with some other dependency group and isn't a super set of that compatible depedendency group (perhaps only considering dependencies that are not Exclude="Compile". This would fix ClassLibrary2 by making the author change their PakcageReference conditions.
2. Add validation for a package that directly references some library: it's resultant assembly has an assembly reference to library X that came from package X but it didn't directly reference package X. This would fix ClassLibrary1 by making the author change the project to directy reference all packages he directly uses in his assembly, effectively insulating him from cases like ClassLibrary2,

[nkolev92]

A library need to consider both the API it exposes, and any packages it depends on as part of its public contract

I've seen issues like this quite often, and there are some related asks from customers wanting to avoid taking compile time dependencies on transitively delivered libraries.

Related issue: https://github.com/NuGet/Home/issues/6720|

1. Add validation whenever you create a dependency group that is compatible with some other dependency group and isn't a super set of that compatible depedendency group (perhaps only considering dependencies that are not Exclude="Compile". This would fix ClassLibrary2 by making the author change their PakcageReference conditions.

I like this but it might raise too many warnings. Especially in the .NET Framework/.NET Standard2.0 multi targeting worlds.
See https://www.nuget.org/packages/NuGet.Protocol/5.3.0 for example.

2. Add validation for a package that directly references some library: it's resultant assembly has an assembly reference to library X that came from package X but it didn't directly reference package X. This would fix ClassLibrary1 by making the author change the project to directy reference all packages he directly uses in his assembly, effectively insulating him from cases like ClassLibrary2,

NuGet doesn't spy on the assemblies today, but I'd be excited to explore what we can do here.
It'll frustrate devs initially but it can do wonders for the eco-system.

I'll move some of this ideas to NuGet/Home and we can continue the discussion there.

[ericstj]

2 is effectively what we do in CoreFx to build our packages, though we use it as the source of truth for dependencies rather than a source for validation.

[carlossanlop]

@nkolev92 I'll close this issue since the questions seems to be answered. Feel free to reopen if you'd like to continue the conversation.