PrivateAssets flow incorrectly to transitively pinned centrally managed dependencies

[marcin-krystianc]

NuGet Product Used

dotnet.exe

Product Version

7.0.100

Worked before?

no

Impact

It bothers me. A fix would be nice

Repro Steps & Context

For project:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
    <CentralPackageTransitivePinningEnabled>true</CentralPackageTransitivePinningEnabled>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json.Bson" PrivateAssets="Compile;Runtime" />
    <PackageReference Include="Microsoft.Rest.ClientRuntime" PrivateAssets="Compile" />
  </ItemGroup>
</Project>

and Directory.Packages.props:

<Project>
    <ItemGroup>
        <PackageVersion Include="Newtonsoft.Json" Version="13.0.1" />
        <PackageVersion Include="Newtonsoft.Json.Bson" Version="1.0.2" />
        <PackageVersion Include="Microsoft.Rest.ClientRuntime" Version="2.3.24" />
    </ItemGroup>
</Project>

The expected content of centralTransitiveDependencyGroups is:

"centralTransitiveDependencyGroups": {
    ".NETStandard,Version=v2.0": {
      "Newtonsoft.Json": {
        "include": "Runtime, Compile, Native, BuildTransitive",
        "version": "[13.0.1, )"
    }
  }
}

, but the SDK 7.0.100 produces empty centralTransitiveDependencyGroups.

PS:
I'm going to submit a PR with a fix.

Verbose Logs

No response

[jeffkl]

@marcin-krystianc I don't think I understand what you're saying in the description. PrivateAssets should end up in a suppressParent section of the centralTransitiveDependencyGroups section of project.assets.json. I did the following:

Test.csproj

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json.Bson" PrivateAssets="Compile;Runtime" />
    <PackageReference Include="Microsoft.Rest.ClientRuntime" PrivateAssets="Compile" />
  </ItemGroup>
</Project>

Directory.Packages.props

<Project>
  <PropertyGroup>
    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
    <CentralPackageTransitivePinningEnabled>true</CentralPackageTransitivePinningEnabled>
  </PropertyGroup>
  <ItemGroup>
    <PackageVersion Include="Newtonsoft.Json" Version="13.0.1" />
    <PackageVersion Include="Newtonsoft.Json.Bson" Version="1.0.2" />
    <PackageVersion Include="Microsoft.Rest.ClientRuntime" Version="2.3.24" />
  </ItemGroup>
</Project>

And the assets file has this:

"centralTransitiveDependencyGroups": {
    ".NETStandard,Version=v2.0": {
      "Newtonsoft.Json": {
        "include": "Runtime, Compile, Native, BuildTransitive",
        "suppressParent": "Runtime, Compile",
        "version": "[13.0.1, )"
      }
    }
  }

NuGet takes the union of PrivateAssets, in this case Compile;Runtime and Compile so the "suppressParent": "Runtime, Compile" is correct.

But your example looks totally different and says that you get an empty centralTransitiveDependencyGroups section? Can you please clarify so I can understand better?

[marcin-krystianc]

But your example looks totally different and says that you get an empty centralTransitiveDependencyGroups section? Can you please clarify so I can understand better?

Ah, I think I've been playing with different examples which led to messing up my description. You are right, that my original example doesn't make sense 😞
It should be:

---

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
    <CentralPackageTransitivePinningEnabled>true</CentralPackageTransitivePinningEnabled>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json.Bson" />
    <PackageReference Include="Microsoft.Rest.ClientRuntime" PrivateAssets="All" />
  </ItemGroup>
</Project>

and Directory.Packages.props:

<Project>
    <ItemGroup>
        <PackageVersion Include="Newtonsoft.Json" Version="13.0.1" />
        <PackageVersion Include="Newtonsoft.Json.Bson" Version="1.0.2" />
        <PackageVersion Include="Microsoft.Rest.ClientRuntime" Version="2.3.24" />
    </ItemGroup>
</Project>

The expected content of centralTransitiveDependencyGroups is:

"centralTransitiveDependencyGroups": {
    ".NETStandard,Version=v2.0": {
      "Newtonsoft.Json": {
        "include": "Runtime, Compile, Native, BuildTransitive",
        "version": "[13.0.1, )"
      }
    }
  }

, but the SDK 7.0.100 produces empty centralTransitiveDependencyGroups.

---

The rationale is that adding PrivateAssets="All" only to one PackageReference shouldn't cause the Newtonsoft.Json to be excluded from centralTransitiveDependencyGroups. Only when PrivateAssets="All" is added to all of the PackageReference(s) , it should cause the Newtonsoft.Json to be excluded from centralTransitiveDependencyGroups.

[jeffkl]

@marcin-krystianc I think I'm convinced we need this fixed but I want to get consensus on the output. Assuming PackageA and PackageB reference the same PackageC.

The default PrivateAssets if you don't specify anything is Build | ContentFiles | Analyzers, so Default below means that.

PackageA	PackageB	PackageC
(Default)	PrivateAssets="All"	PrivateAssets="Build;ContentFiles;Analyzers"
PrivateAssets="Compile;Runtime"	PrivateAssets="Runtime"	PrivateAssets="Runtime"
PrivateAssets="All"	PrivateAssets="Runtime"	PrivateAssets="Runtime"
(Default)	PrivateAssets="Runtime;Build;ContentFiles"	PrivateAssets="Build;ContentFiles"
PrivateAssets="None"	PrivateAssets="Runtime"	PrivateAssets="None"

So I'm going to push a commit to your branch to get it updated and have the test try a few different permutations.

[marcin-krystianc]

So I'm going to push a commit to your branch to get it updated and have the test try a few different permutations.

Sure, adding more tests is always a good idea :-) I can also do it if you want.

[jeffkl]

Sure, adding more tests is always a good idea :-) I can also do it if you want.

I'm still experimenting so it'll probably just be easier to push the branch then explain what I'm after. I'll do it soon...

[jeffkl]

I finally think I got it figured out. You're absolutely right that the assets should be the intersection. However, the missing piece was that transitive dependencies have different defaults than direct dependencies. This was causing some confusion for me when I was updating the unit test and getting unexpected results. But I think I have it figured out so I force pushed to your branch. I'll get this reviewed and merged next week. Thanks again for the contribution, I hope taking over the branch doesn't make you think we are any less grateful. I think in this case the scenario was just too complicated to understand without me getting myself involved. Your contribution is greatly appreciated!