Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Effective PrivateAssets of centrally managed transitive dependencies should be an intersection of parent dependencies #4953

Merged
merged 1 commit into from
Feb 22, 2023
Merged

Effective PrivateAssets of centrally managed transitive dependencies should be an intersection of parent dependencies #4953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 22 additions & 27 deletions src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ private static void AddProjectFileDependenciesForPackageReference(PackageSpec pr

private void AddCentralTransitiveDependencyGroupsForPackageReference(PackageSpec project, LockFile lockFile, IEnumerable<RestoreTargetGraph> targetGraphs)
{
if (project.RestoreMetadata == null || !project.RestoreMetadata.CentralPackageVersionsEnabled)
if (project.RestoreMetadata == null || !project.RestoreMetadata.CentralPackageVersionsEnabled || !project.RestoreMetadata.CentralPackageTransitivePinningEnabled)
{
return;
}
Expand All @@ -491,7 +491,7 @@ private void AddCentralTransitiveDependencyGroupsForPackageReference(PackageSpec
}

// The transitive dependencies enforced by the central package version management file are written to the assets to be used by the pack task.
List<LibraryDependency> centralEnforcedTransitiveDependencies = GetLibraryDependenciesForCentralTransitiveDependencies(targetGraph, targetFrameworkInformation, project.RestoreMetadata.CentralPackageTransitivePinningEnabled).ToList();
List<LibraryDependency> centralEnforcedTransitiveDependencies = GetLibraryDependenciesForCentralTransitiveDependencies(targetGraph, targetFrameworkInformation).ToList();

if (centralEnforcedTransitiveDependencies.Any())
{
Expand All @@ -513,7 +513,7 @@ private void AddCentralTransitiveDependencyGroupsForPackageReference(PackageSpec
/// <param name="targetFrameworkInformation">The <see cref="TargetFrameworkInformation" /> for the target framework to get centrally defined transitive dependencies for.</param>
/// <param name="centralPackageTransitivePinningEnabled">A value indicating whether or not central transitive dependency version pinning is enabled.</param>
/// <returns>An <see cref="IEnumerable{LibraryDependency}" /> representing the centrally defined transitive dependencies for the specified <see cref="RestoreTargetGraph" />.</returns>
private IEnumerable<LibraryDependency> GetLibraryDependenciesForCentralTransitiveDependencies(RestoreTargetGraph targetGraph, TargetFrameworkInformation targetFrameworkInformation, bool centralPackageTransitivePinningEnabled)
private IEnumerable<LibraryDependency> GetLibraryDependenciesForCentralTransitiveDependencies(RestoreTargetGraph targetGraph, TargetFrameworkInformation targetFrameworkInformation)
{
foreach (GraphNode<RemoteResolveResult> rootNode in targetGraph.Graphs)
{
Expand All @@ -528,39 +528,34 @@ private IEnumerable<LibraryDependency> GetLibraryDependenciesForCentralTransitiv
CentralPackageVersion centralPackageVersion = targetFrameworkInformation.CentralPackageVersions[node.Item.Key.Name];
Dictionary<string, LibraryIncludeFlags> dependenciesIncludeFlags = _includeFlagGraphs[targetGraph];

LibraryIncludeFlags suppressParent = LibraryIncludeFlags.None;
LibraryIncludeFlags suppressParent = LibraryIncludeFlags.All;
jeffkl marked this conversation as resolved.
Show resolved Hide resolved

if (centralPackageTransitivePinningEnabled)
// Centrally pinned dependencies are not directly declared but the intersection of all of the PrivateAssets of the parents that pulled it in should apply to it
foreach (GraphNode<RemoteResolveResult> parentNode in EnumerateParentNodes(node))
{
// Centrally pinned dependencies are not directly declared but the PrivateAssets from the top-level dependency that pulled it in should apply to it also
foreach (GraphNode<RemoteResolveResult> parentNode in EnumerateParentNodes(node))
{
LibraryDependency parentDependency = rootNode.Item.Data.Dependencies.FirstOrDefault(i => i.Name.Equals(parentNode.Item.Key.Name, StringComparison.OrdinalIgnoreCase));

// A transitive dependency that is a few levels deep won't be a top-level dependency so skip it
if (parentDependency == null || parentDependency.ReferenceType != LibraryDependencyReferenceType.Direct)
{
continue;
}

suppressParent |= parentDependency.SuppressParent;
}
LibraryDependency parentDependency = rootNode.Item.Data.Dependencies.FirstOrDefault(i => i.Name.Equals(parentNode.Item.Key.Name, StringComparison.OrdinalIgnoreCase));

// If all assets are suppressed then the dependency should not be added
if (suppressParent == LibraryIncludeFlags.All)
// A transitive dependency that is a few levels deep won't be a top-level dependency so skip it
if (parentDependency == null || parentDependency.ReferenceType != LibraryDependencyReferenceType.Direct)
{
continue;
}

suppressParent &= parentDependency.SuppressParent;
}

yield return new LibraryDependency()
// If all assets are suppressed then the dependency should not be added
if (suppressParent != LibraryIncludeFlags.All)
{
LibraryRange = new LibraryRange(centralPackageVersion.Name, centralPackageVersion.VersionRange, LibraryDependencyTarget.Package),
ReferenceType = LibraryDependencyReferenceType.Transitive,
VersionCentrallyManaged = true,
IncludeType = dependenciesIncludeFlags[centralPackageVersion.Name],
SuppressParent = suppressParent,
};
yield return new LibraryDependency()
{
LibraryRange = new LibraryRange(centralPackageVersion.Name, centralPackageVersion.VersionRange, LibraryDependencyTarget.Package),
ReferenceType = LibraryDependencyReferenceType.Transitive,
VersionCentrallyManaged = true,
IncludeType = dependenciesIncludeFlags[centralPackageVersion.Name],
SuppressParent = suppressParent,
};
}
}
}
}
Expand Down
39 changes: 27 additions & 12 deletions test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2563,9 +2563,19 @@ public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToP
/// <summary>
/// Verifies that when a transitive package version is pinned and is referenced by multiple parents, the PrivateAssets flow from the package that pulled it into the graph to the pinned dependency.
/// </summary>
/// <returns></returns>
[Fact]
public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToPinnedDependenciesWithMultipleParents()
[Theory]
[InlineData(
LibraryIncludeFlags.All, // PrivateAssets="All"
LibraryIncludeFlags.Build | LibraryIncludeFlags.ContentFiles | LibraryIncludeFlags.Analyzers, // Default PrivateAssets
LibraryIncludeFlags.Build | LibraryIncludeFlags.ContentFiles | LibraryIncludeFlags.Analyzers)] // Expect only the intersection, in this case the default
[InlineData(
LibraryIncludeFlags.Compile | LibraryIncludeFlags.Runtime, // PrivateAssets="Compile;Runtime"
LibraryIncludeFlags.Compile, // PrivateAssets="Compile"
LibraryIncludeFlags.Compile)] // The intersection is Compile
[InlineData(LibraryIncludeFlags.All, LibraryIncludeFlags.All, LibraryIncludeFlags.All)] // When both parents have PrivateAssets="All", expect that the dependency does not flow
[InlineData(LibraryIncludeFlags.None, LibraryIncludeFlags.None, LibraryIncludeFlags.None)] // When both parents have PrivateAssets="None", expect all assets of the dependency to flow
[InlineData(LibraryIncludeFlags.None, LibraryIncludeFlags.Runtime | LibraryIncludeFlags.Compile, LibraryIncludeFlags.None)] // When both parents have PrivateAssets="None", expect that the dependency is completely suppressed
public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToPinnedDependenciesWithMultipleParents(LibraryIncludeFlags suppressParent1, LibraryIncludeFlags suppressParent2, LibraryIncludeFlags expected)
{
// Arrange
var framework = new NuGetFramework("net46");
Expand Down Expand Up @@ -2617,7 +2627,7 @@ public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToP
TypeConstraint = LibraryDependencyTarget.Package,
},
VersionCentrallyManaged = true,
SuppressParent = LibraryIncludeFlags.Runtime
SuppressParent = suppressParent1,
},
new LibraryDependency()
{
Expand All @@ -2627,7 +2637,7 @@ public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToP
TypeConstraint = LibraryDependencyTarget.Package,
},
VersionCentrallyManaged = true,
SuppressParent = LibraryIncludeFlags.Analyzers
SuppressParent = suppressParent2,
},
},
new List<CentralPackageVersion>
Expand Down Expand Up @@ -2660,19 +2670,24 @@ public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToP
var result = await restoreCommand.ExecuteAsync();
var lockFile = result.LockFile;

var targetLib = lockFile.Targets.First().Libraries.Where(l => l.Name == packageA.Id).FirstOrDefault();

// Assert
Assert.True(result.Success);
Assert.Equal(1, lockFile.CentralTransitiveDependencyGroups.Count);
if (expected == LibraryIncludeFlags.All)
{
Assert.Equal(0, lockFile.CentralTransitiveDependencyGroups.Count);
}
else
{
Assert.Equal(1, lockFile.CentralTransitiveDependencyGroups.Count);

List<LibraryDependency> transitiveDependencies = lockFile.CentralTransitiveDependencyGroups.First().TransitiveDependencies.ToList();
List<LibraryDependency> transitiveDependencies = lockFile.CentralTransitiveDependencyGroups.First().TransitiveDependencies.ToList();

Assert.Equal(1, transitiveDependencies.Count);
Assert.Equal(1, transitiveDependencies.Count);

LibraryDependency transitiveDependencyC = transitiveDependencies.Single(i => i.Name.Equals(packageC2_0.Id));
LibraryDependency transitiveDependencyC = transitiveDependencies.Single(i => i.Name.Equals(packageC2_0.Id));

Assert.Equal(LibraryIncludeFlags.Runtime | LibraryIncludeFlags.Analyzers, transitiveDependencyC.SuppressParent);
Assert.Equal(expected, transitiveDependencyC.SuppressParent);
}
}
}

Expand Down