Flow PrivateAssets to transitively pinned centrally managed dependencies

[jeffkl]

Bug

Fixes: NuGet/Home#10311

Regression? No Last working version:

Description

When a centrally managed dependency version is pinned, the top-level package that pulled it in should have its PrivateAssets flow down. This change looks for the top-level dependency that brought in a pinned dependency and then applies its PrivateAssets.

PR Checklist

• PR has a meaningful title

• PR has a linked issue.

• Described changes

• Tests

  • Automated tests added
  • OR
  • Test exception
  • OR
  • N/A

• Documentation

  • Documentation PR or issue filled
  • OR
  • N/A

[jeffkl]

FYI @AArnott

[dominoFire]

A comment on FlattenParentNodes().

[nkolev92]

This is a tough issue to figure out for sure!

[jeffkl]

I've updated the logic to walk to the parent that brought in a dependency and use its PrivateAssets. But if you add one of the transitive dependencies as a top-level dependency, the walk stop at the last parent in the graph. This is intentional because if you add a top-level dependency, its probably because you want to alter in the include/exclude and/or private assets.

[nkolev92]

Can we do a test for the following:

Project -> A -> C
Project -> B -> C
?
I think it should be a merge between A & B's assets.

I thought we wanted to do the combine on the pack side as well?

In particular see:

NuGet.Client/test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs

Line 830 in c48ecf8

public void PackCommand_SupportsIncludeExcludePrivateAssets_OnPackages(

.

Note how the include and private assets combination:

includeassets	excludeassets	privateassets	expectedinclude	expectedexclude
null (default)	null (default)	null (default)		Analyzers,Build
Compile	null(default)	null(default)		Analyzers,Build,BuildTransitive,Native,Runtime

Another consideration:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="NuGet.Common" IncludeAssets="compile" PrivateAssets="compile" />
  </ItemGroup>

</Project>

Generates:

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    <id>PackIncludesTest</id>
    <version>1.0.0</version>
    <authors>PackIncludesTest</authors>
    <description>Package Description</description>
    <dependencies>
      <group targetFramework="net6.0">
        <dependency id="NuGet.Common" version="1.0.0" exclude="Runtime,Compile,Build,Native,Analyzers,BuildTransitive" />
      </group>
    </dependencies>
  </metadata>
</package>

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

[jeffkl]

Can we do a test for the following:

Project -> A -> C
Project -> B -> C

Added RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToPinnedDependenciesWithMultipleParents which shows that all parents' PrivateAssets are aggregated.

[nkolev92]

Do we need a pack test? For whatever reason, I did not realize we didn't have one for a very long time :D
It can go in a different PR.

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

[jeffkl]

Do we need a pack test?

Technically this doesn't change the pack logic, it simply updates how the lock file is written. That said, I think you're right that there isn't any logic to test the pack side of how its read:

NuGet.Client/src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs

Line 922 in a3d3b5c

IEnumerable<LibraryDependency> centralTransitiveDependencies = assetsFile

I'm going to merge for now and look into it later.

[AArnott]

I'd love to try this out. Is there an SDK preview (even a CI build) that includes this change?