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.

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

PrivateAssets for central transitive dependencies should flow regardless whether the parent node is a project or a package #4952

Merged
63 changes: 33 additions & 30 deletions src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -515,50 +515,53 @@ private void AddCentralTransitiveDependencyGroupsForPackageReference(PackageSpec
/// <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)
{
foreach (GraphNode<RemoteResolveResult> node in targetGraph.Graphs.SelectMany(i => i.InnerNodes))
foreach (GraphNode<RemoteResolveResult> rootNode in targetGraph.Graphs)
jeffkl marked this conversation as resolved.
Show resolved Hide resolved
{
// Only consider nodes that are Accepted, IsCentralTransitive, and have a centrally defined package version
if (node?.Item == null || node.Disposition != Disposition.Accepted || !node.Item.IsCentralTransitive || !targetFrameworkInformation.CentralPackageVersions?.ContainsKey(node.Item.Key.Name) == true)
foreach (GraphNode<RemoteResolveResult> node in rootNode.InnerNodes)
{
continue;
}
// Only consider nodes that are Accepted, IsCentralTransitive, and have a centrally defined package version
if (node?.Item == null || node.Disposition != Disposition.Accepted || !node.Item.IsCentralTransitive || !targetFrameworkInformation.CentralPackageVersions?.ContainsKey(node.Item.Key.Name) == true)
{
continue;
}

CentralPackageVersion centralPackageVersion = targetFrameworkInformation.CentralPackageVersions[node.Item.Key.Name];
Dictionary<string, LibraryIncludeFlags> dependenciesIncludeFlags = _includeFlagGraphs[targetGraph];
CentralPackageVersion centralPackageVersion = targetFrameworkInformation.CentralPackageVersions[node.Item.Key.Name];
Dictionary<string, LibraryIncludeFlags> dependenciesIncludeFlags = _includeFlagGraphs[targetGraph];

LibraryIncludeFlags suppressParent = LibraryIncludeFlags.None;
LibraryIncludeFlags suppressParent = LibraryIncludeFlags.None;

if (centralPackageTransitivePinningEnabled)
{
// 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))
if (centralPackageTransitivePinningEnabled)
{
LibraryDependency parentDependency = targetFrameworkInformation.Dependencies.FirstOrDefault(i => i.Name.Equals(parentNode.Item.Key.Name, StringComparison.OrdinalIgnoreCase));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetFrameworkInformation.Dependencies doesn't contain projects.

// 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)
{
continue;
}

// A transitive dependency that is a few levels deep won't be a top-level dependency so skip it
if (parentDependency == null)
suppressParent |= parentDependency.SuppressParent;
}

// If all assets are suppressed then the dependency should not be added
if (suppressParent == LibraryIncludeFlags.All)
{
continue;
}

suppressParent |= parentDependency.SuppressParent;
}

// If all assets are suppressed then the dependency should not be added
if (suppressParent == LibraryIncludeFlags.All)
yield return new LibraryDependency()
{
continue;
}
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
Original file line number Diff line number Diff line change
Expand Up @@ -2486,6 +2486,92 @@ public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToP
}
}

[Theory]
[InlineData(LibraryIncludeFlags.All, 0)]
[InlineData(LibraryIncludeFlags.None, 1)]
public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToPinnedDependenciesWithSingleParentProject(LibraryIncludeFlags privateAssets, int expectedCount)
{
// Arrange
using (var tmpPath = new SimpleTestPathContext())
{
var logger = new TestLogger();

var project1Directory = new DirectoryInfo(Path.Combine(tmpPath.SolutionRoot, "Project1"));
var project2Directory = new DirectoryInfo(Path.Combine(tmpPath.SolutionRoot, "Project2"));

var globalPackages = new DirectoryInfo(Path.Combine(tmpPath.WorkingDirectory, "globalPackages"));
var packageSource = new DirectoryInfo(Path.Combine(tmpPath.WorkingDirectory, "packageSource"));

globalPackages.Create();
packageSource.Create();

// Project1 -> Project2 -> PackageA 1.0.0
var packageA = new SimpleTestPackageContext { Id = "PackageA", Version = "1.0.0", };
await SimpleTestPackageUtility.CreateFullPackageAsync(tmpPath.PackageSource, packageA);

var project2Spec = PackageReferenceSpecBuilder.Create("Project2", project2Directory.FullName)
.WithTargetFrameworks(new[]
{
new TargetFrameworkInformation
{
FrameworkName = NuGetFramework.Parse("net46"),
Dependencies = new List<LibraryDependency>(new[]
{
new LibraryDependency
{
LibraryRange = new LibraryRange("PackageA", VersionRange.Parse("1.0.0"),
LibraryDependencyTarget.All),
VersionCentrallyManaged = true,
},
}),
CentralPackageVersions = { new KeyValuePair<string, CentralPackageVersion>("PackageA", new CentralPackageVersion("PackageA", VersionRange.Parse("1.0.0"))) },
}
})
.WithCentralPackageVersionsEnabled()
.WithCentralPackageTransitivePinningEnabled()
.Build()
.WithTestRestoreMetadata();

var project1Spec = PackageReferenceSpecBuilder.Create("Project1", project1Directory.FullName)
.WithTargetFrameworks(new[]
{
new TargetFrameworkInformation
{
FrameworkName = NuGetFramework.Parse("net46"),
Dependencies = new List<LibraryDependency>(),
CentralPackageVersions = { new KeyValuePair<string, CentralPackageVersion>("PackageA", new CentralPackageVersion("PackageA", VersionRange.Parse("1.0.0"))) },
}
})
.WithCentralPackageVersionsEnabled()
.WithCentralPackageTransitivePinningEnabled()
.Build()
.WithTestRestoreMetadata()
.WithTestProjectReference(project2Spec, privateAssets:privateAssets);

var restoreContext = new RestoreArgs()
{
Sources = new List<string>() { packageSource.FullName },
GlobalPackagesFolder = globalPackages.FullName,
Log = logger,
CacheContext = new TestSourceCacheContext(),
};

await SimpleTestPackageUtility.CreatePackagesAsync(
packageSource.FullName,
packageA);

var request = await ProjectTestHelpers.GetRequestAsync(restoreContext, project1Spec, project2Spec);

var restoreCommand = new RestoreCommand(request);
var result = await restoreCommand.ExecuteAsync();
var lockFile = result.LockFile;

// Assert
Assert.True(result.Success);
Assert.Equal(expectedCount, lockFile.CentralTransitiveDependencyGroups.Count);
}
}

/// <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>
Expand Down