Flow PrivateAssets to transitively pinned centrally managed dependencies by jeffkl · Pull Request #4669 · NuGet/NuGet.Client

jeffkl · 2022-06-13T19:47:48Z

Bug

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

jeffkl · 2022-06-13T19:48:09Z

FYI @AArnott

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

src/NuGet.Core/NuGet.DependencyResolver.Core/GraphModel/GraphNode.cs

dominoFire

A comment on FlattenParentNodes().

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

nkolev92

This is a tough issue to figure out for sure!

test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests.cs

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests.cs

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

jeffkl · 2022-06-24T22:27:06Z

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>

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

ghost · 2022-07-05T04:01:24Z

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.

NuGet/Home#10311

…adata

jeffkl · 2022-07-07T16:28:36Z

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 · 2022-07-20T04:01:18Z

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 · 2022-07-27T04:01:21Z

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 · 2022-07-29T16:10:14Z

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 · 2022-07-30T02:19:00Z

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

jeffkl marked this pull request as ready for review June 13, 2022 19:47

jeffkl requested a review from a team as a code owner June 13, 2022 19:47

jeffkl commented Jun 13, 2022

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.DependencyResolver.Core/GraphModel/GraphNode.cs Show resolved Hide resolved

jeffkl self-assigned this Jun 14, 2022

dominoFire reviewed Jun 14, 2022

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Outdated Show resolved Hide resolved

nkolev92 reviewed Jun 17, 2022

View reviewed changes

jeffkl mentioned this pull request Jun 21, 2022

[Epic]: Central Package Management Improvements for 17.3 NuGet/Home#11752

Closed

7 tasks

jeffkl force-pushed the dev-jeffkl-flow-privateassets-to-pinned-dependencies branch from 8c25fa3 to 6ceaad5 Compare June 27, 2022 17:25

nkolev92 reviewed Jun 28, 2022

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Show resolved Hide resolved

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Show resolved Hide resolved

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 5, 2022

jeffkl removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 6, 2022

jeffkl added 6 commits July 7, 2022 09:25

Flow PrivateAssets to transitively pinned centrally managed dependencies

e4b2af8

NuGet/Home#10311

Fix logic to narrow down items in the graph

f4f5fb3

Address comments

6ce5723

Refactor LINQ query into a standalone method for cleaner code

15c0977

Fix NullReferenceException when unit test does not provide ProjectMet…

796ad40

…adata

Adjust logic to work with multiple levels of transitivity

30e87d8

jeffkl force-pushed the dev-jeffkl-flow-privateassets-to-pinned-dependencies branch from 6ceaad5 to 30e87d8 Compare July 7, 2022 16:25

jeffkl mentioned this pull request Jul 7, 2022

[Epic]: Central Package Management improvements for 17.4 NuGet/Home#11952

Closed

21 tasks

nkolev92 approved these changes Jul 13, 2022

View reviewed changes

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 20, 2022

jeffkl removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 29, 2022

jeffkl merged commit 5957409 into dev Jul 29, 2022

jeffkl deleted the dev-jeffkl-flow-privateassets-to-pinned-dependencies branch July 29, 2022 16:10

marcin-krystianc mentioned this pull request Nov 23, 2022

Improved performance and fixed how the PrivateAssets flow to transitively pinned centrally managed dependencies #4947

Closed

8 tasks

marcin-krystianc mentioned this pull request Feb 28, 2023

Performance regression of NuGet restores in the sdk v7.0.100 due to calculation of "CentralTransitiveDependencyGroups" NuGet/Home#12269

Closed

Conversation

jeffkl commented Jun 13, 2022

Bug

Description

PR Checklist

Uh oh!

jeffkl commented Jun 13, 2022

Uh oh!

Uh oh!

Uh oh!

dominoFire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeffkl commented Jun 24, 2022

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Jul 5, 2022

Uh oh!

jeffkl commented Jul 7, 2022

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 20, 2022

Uh oh!

ghost commented Jul 27, 2022

Uh oh!

jeffkl commented Jul 29, 2022

Uh oh!

AArnott commented Jul 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants