Skip to content

Commit

Permalink
Fix
Browse files Browse the repository at this point in the history
  • Loading branch information
jeffkl committed Feb 2, 2023
1 parent 098dab6 commit ce8af21
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 34 deletions.
37 changes: 17 additions & 20 deletions src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs
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,29 +528,26 @@ 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;

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;
}

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

yield return new LibraryDependency()
Expand Down
1 change: 1 addition & 0 deletions src/NuGet.Core/NuGet.LibraryModel/GlobalSuppressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@
[assembly: SuppressMessage("Build", "CA2227:Change 'Items' to be read-only by removing the property setter.", Justification = "<Pending>", Scope = "member", Target = "~P:NuGet.LibraryModel.Library.Items")]
[assembly: SuppressMessage("Build", "CA2227:Change 'NoWarn' to be read-only by removing the property setter.", Justification = "<Pending>", Scope = "member", Target = "~P:NuGet.LibraryModel.LibraryDependency.NoWarn")]
[assembly: SuppressMessage("Build", "CA1067:Type NuGet.LibraryModel.FrameworkDependency should override Equals because it implements IEquatable<T>", Justification = "<Pending>", Scope = "type", Target = "~T:NuGet.LibraryModel.FrameworkDependency")]
[assembly: SuppressMessage("Globalization", "CA1308:Normalize strings to uppercase", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.LibraryModel.LibraryIncludeFlagUtils.GetFlags(System.Collections.Generic.IEnumerable{System.String},System.Boolean)~NuGet.LibraryModel.LibraryIncludeFlags")]
10 changes: 9 additions & 1 deletion src/NuGet.Core/NuGet.LibraryModel/LibraryIncludeFlagUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ public static class LibraryIncludeFlagUtils
/// Convert set of flag strings into a LibraryIncludeFlags.
/// </summary>
public static LibraryIncludeFlags GetFlags(IEnumerable<string> flags)
{
return GetFlags(flags, includeBuildWithBuildTransitive: true);
}

/// <summary>
/// Convert set of flag strings into a LibraryIncludeFlags.
/// </summary>
public static LibraryIncludeFlags GetFlags(IEnumerable<string> flags, bool includeBuildWithBuildTransitive)
{
if (flags == null)
{
Expand Down Expand Up @@ -58,7 +66,7 @@ public static LibraryIncludeFlags GetFlags(IEnumerable<string> flags)
result |= LibraryIncludeFlags.Analyzers;
break;
case "buildtransitive":
result |= LibraryIncludeFlags.BuildTransitive | LibraryIncludeFlags.Build;
result |= includeBuildWithBuildTransitive ? LibraryIncludeFlags.BuildTransitive | LibraryIncludeFlags.Build : LibraryIncludeFlags.BuildTransitive;
break;

// None is a noop here
Expand Down
1 change: 1 addition & 0 deletions src/NuGet.Core/NuGet.LibraryModel/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
#nullable enable
~static NuGet.LibraryModel.LibraryIncludeFlagUtils.GetFlags(System.Collections.Generic.IEnumerable<string> flags, bool includeBuildWithBuildTransitive) -> NuGet.LibraryModel.LibraryIncludeFlags
2 changes: 1 addition & 1 deletion src/NuGet.Core/NuGet.ProjectModel/JsonPackageSpecReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ internal static void ReadCentralTransitiveDependencyGroup(
case "include":
values = jsonReader.ReadDelimitedString();
dependencyIncludeFlagsValue = LibraryIncludeFlagUtils.GetFlags(values);
dependencyIncludeFlagsValue = LibraryIncludeFlagUtils.GetFlags(values, includeBuildWithBuildTransitive: false);
break;
case "suppressParent":
Expand Down
39 changes: 27 additions & 12 deletions test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests.cs
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

0 comments on commit ce8af21

Please sign in to comment.