diff --git a/src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs b/src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs index 41580b88a71..b1fadae7f39 100644 --- a/src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs +++ b/src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs @@ -474,7 +474,7 @@ private static void AddProjectFileDependenciesForPackageReference(PackageSpec pr private void AddCentralTransitiveDependencyGroupsForPackageReference(PackageSpec project, LockFile lockFile, IEnumerable targetGraphs) { - if (project.RestoreMetadata == null || !project.RestoreMetadata.CentralPackageVersionsEnabled) + if (project.RestoreMetadata == null || !project.RestoreMetadata.CentralPackageVersionsEnabled || !project.RestoreMetadata.CentralPackageTransitivePinningEnabled) { return; } @@ -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 centralEnforcedTransitiveDependencies = GetLibraryDependenciesForCentralTransitiveDependencies(targetGraph, targetFrameworkInformation, project.RestoreMetadata.CentralPackageTransitivePinningEnabled).ToList(); + List centralEnforcedTransitiveDependencies = GetLibraryDependenciesForCentralTransitiveDependencies(targetGraph, targetFrameworkInformation).ToList(); if (centralEnforcedTransitiveDependencies.Any()) { @@ -513,7 +513,7 @@ private void AddCentralTransitiveDependencyGroupsForPackageReference(PackageSpec /// The for the target framework to get centrally defined transitive dependencies for. /// A value indicating whether or not central transitive dependency version pinning is enabled. /// An representing the centrally defined transitive dependencies for the specified . - private IEnumerable GetLibraryDependenciesForCentralTransitiveDependencies(RestoreTargetGraph targetGraph, TargetFrameworkInformation targetFrameworkInformation, bool centralPackageTransitivePinningEnabled) + private IEnumerable GetLibraryDependenciesForCentralTransitiveDependencies(RestoreTargetGraph targetGraph, TargetFrameworkInformation targetFrameworkInformation) { foreach (GraphNode rootNode in targetGraph.Graphs) { @@ -528,29 +528,26 @@ private IEnumerable GetLibraryDependenciesForCentralTransitiv CentralPackageVersion centralPackageVersion = targetFrameworkInformation.CentralPackageVersions[node.Item.Key.Name]; Dictionary 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 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 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() diff --git a/src/NuGet.Core/NuGet.LibraryModel/GlobalSuppressions.cs b/src/NuGet.Core/NuGet.LibraryModel/GlobalSuppressions.cs index 82be35eab78..e566c7c0c18 100644 --- a/src/NuGet.Core/NuGet.LibraryModel/GlobalSuppressions.cs +++ b/src/NuGet.Core/NuGet.LibraryModel/GlobalSuppressions.cs @@ -25,3 +25,4 @@ [assembly: SuppressMessage("Build", "CA2227:Change 'Items' to be read-only by removing the property setter.", Justification = "", Scope = "member", Target = "~P:NuGet.LibraryModel.Library.Items")] [assembly: SuppressMessage("Build", "CA2227:Change 'NoWarn' to be read-only by removing the property setter.", Justification = "", Scope = "member", Target = "~P:NuGet.LibraryModel.LibraryDependency.NoWarn")] [assembly: SuppressMessage("Build", "CA1067:Type NuGet.LibraryModel.FrameworkDependency should override Equals because it implements IEquatable", Justification = "", Scope = "type", Target = "~T:NuGet.LibraryModel.FrameworkDependency")] +[assembly: SuppressMessage("Globalization", "CA1308:Normalize strings to uppercase", Justification = "", Scope = "member", Target = "~M:NuGet.LibraryModel.LibraryIncludeFlagUtils.GetFlags(System.Collections.Generic.IEnumerable{System.String},System.Boolean)~NuGet.LibraryModel.LibraryIncludeFlags")] diff --git a/src/NuGet.Core/NuGet.LibraryModel/LibraryIncludeFlagUtils.cs b/src/NuGet.Core/NuGet.LibraryModel/LibraryIncludeFlagUtils.cs index 9ca077acca2..0e5d25ef429 100644 --- a/src/NuGet.Core/NuGet.LibraryModel/LibraryIncludeFlagUtils.cs +++ b/src/NuGet.Core/NuGet.LibraryModel/LibraryIncludeFlagUtils.cs @@ -24,6 +24,14 @@ public static class LibraryIncludeFlagUtils /// Convert set of flag strings into a LibraryIncludeFlags. /// public static LibraryIncludeFlags GetFlags(IEnumerable flags) + { + return GetFlags(flags, includeBuildWithBuildTransitive: true); + } + + /// + /// Convert set of flag strings into a LibraryIncludeFlags. + /// + public static LibraryIncludeFlags GetFlags(IEnumerable flags, bool includeBuildWithBuildTransitive) { if (flags == null) { @@ -58,7 +66,7 @@ public static LibraryIncludeFlags GetFlags(IEnumerable 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 diff --git a/src/NuGet.Core/NuGet.LibraryModel/PublicAPI.Unshipped.txt b/src/NuGet.Core/NuGet.LibraryModel/PublicAPI.Unshipped.txt index 7dc5c58110b..9a51fcaa266 100644 --- a/src/NuGet.Core/NuGet.LibraryModel/PublicAPI.Unshipped.txt +++ b/src/NuGet.Core/NuGet.LibraryModel/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +~static NuGet.LibraryModel.LibraryIncludeFlagUtils.GetFlags(System.Collections.Generic.IEnumerable flags, bool includeBuildWithBuildTransitive) -> NuGet.LibraryModel.LibraryIncludeFlags diff --git a/src/NuGet.Core/NuGet.ProjectModel/JsonPackageSpecReader.cs b/src/NuGet.Core/NuGet.ProjectModel/JsonPackageSpecReader.cs index b4e1ce41fb0..94904a51c79 100644 --- a/src/NuGet.Core/NuGet.ProjectModel/JsonPackageSpecReader.cs +++ b/src/NuGet.Core/NuGet.ProjectModel/JsonPackageSpecReader.cs @@ -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": diff --git a/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests.cs b/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests.cs index 633f0f49e33..e05aa36da8f 100644 --- a/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests.cs +++ b/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests.cs @@ -2563,9 +2563,19 @@ public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToP /// /// 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. /// - /// - [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"); @@ -2617,7 +2627,7 @@ public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToP TypeConstraint = LibraryDependencyTarget.Package, }, VersionCentrallyManaged = true, - SuppressParent = LibraryIncludeFlags.Runtime + SuppressParent = suppressParent1, }, new LibraryDependency() { @@ -2627,7 +2637,7 @@ public async Task RestoreCommand_CentralVersion_AssetsFile_PrivateAssetsFlowsToP TypeConstraint = LibraryDependencyTarget.Package, }, VersionCentrallyManaged = true, - SuppressParent = LibraryIncludeFlags.Analyzers + SuppressParent = suppressParent2, }, }, new List @@ -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 transitiveDependencies = lockFile.CentralTransitiveDependencyGroups.First().TransitiveDependencies.ToList(); + List 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); + } } }