Central transitive dependencies should be considered only for root nodes #4611

marcin-krystianc · 2022-05-04T11:41:39Z

Bug

Regression? Last working version: -

Description

After this change, the DependencyWalker will check whether transitive dependency eclipses or downgrades the current library only when that transitive dependency is at the top-level node.

With this change, the example from the linked issue fails with downgrade errors (GOOD):

dotnet.exe restore --force
  Determining projects to restore...
  The project D:\workspace\TestSolutions\MissingDowngradeCentralisedLib\Lib1\Lib1.csproj is using CentralPackageVersionManagement, a NuGet preview feature.
  The project D:\workspace\TestSolutions\MissingDowngradeCentralisedLib\App\App.csproj is using CentralPackageVersionManagement, a NuGet preview feature.
D:\workspace\TestSolutions\MissingDowngradeCentralisedLib\App\App.csproj : error NU1605: Detected package downgrade: Newtonsoft.Json from 12.0.2 to 12.0.1. Reference the package directly from the project to select a different version.  [D:\workspace\TestSolutions\MissingDowngradeCentralisedLib\Solution.sln]
D:\workspace\TestSolutions\MissingDowngradeCentralisedLib\App\App.csproj : error NU1605:  App -> Lib1 -> Lib2 -> Newtonsoft.Json (>= 12.0.2)  [D:\workspace\TestSolutions\MissingDowngradeCentralisedLib\Solution.sln]
D:\workspace\TestSolutions\MissingDowngradeCentralisedLib\App\App.csproj : error NU1605:  App -> Newtonsoft.Json (>= 12.0.1) [D:\workspace\TestSolutions\MissingDowngradeCentralisedLib\Solution.sln]

PR Checklist

marcin-krystianc · 2022-05-04T11:42:31Z

src/NuGet.Core/NuGet.ProjectModel/LockFile/LockFileFormat.cs

@@ -843,7 +843,7 @@ private static List<CentralTransitiveDependencyGroup> ReadProjectFileTransitiveD
                    NuGetFramework framework = NuGetFramework.Parse(frameworkPropertyName);
                    var dependencies = new List<LibraryDependency>();

-                    JsonPackageSpecReader.ReadCentralTransitveDependencyGroup(


Just fixing a typographic mistake in an internal method.

jeffkl

Thanks, looks good with one small comment. I had to rebase your branch and force push so be sure to pull it next time you work on it.

src/NuGet.Core/NuGet.DependencyResolver.Core/Remote/RemoteDependencyWalker.cs

…ndencyWalker.cs Co-authored-by: Jeff Kluge <jeffkl@microsoft.com>

marcin-krystianc · 2022-05-05T09:36:32Z

@jeffkl 1612530 has some test failures. Are these legitimate issues or just a flakiness?

jeffkl · 2022-05-05T17:01:51Z

@jeffkl 1612530 has some test failures. Are these legitimate issues or just a flakiness?

Yeah sorry we're having some troubles with CI today, I'll work on it.

src/NuGet.Core/NuGet.DependencyResolver.Core/Remote/RemoteDependencyWalker.cs

nkolev92

Sorry for the delay.

Your explanation seems good! It's been a while since we've looked at this.

A few qs about the test.

I wouldn't be the worst idea to define an end to end test as well, as the dependency walker tests rely on a lot of configuration.

Example test is ExecuteAsync_WithDowngradesInPrunedSubgraph_DoesNotRaiseNU1605, it's not CPM, but other in the same class are RestoreCommand_CentralVersion_ErrorWhenDependenciesHaveVersion.

nkolev92 · 2022-05-20T01:43:33Z

test/NuGet.Core.Tests/NuGet.DependencyResolver.Core.Tests/RemoteDependencyWalkerTests.cs

+            var context = new TestRemoteWalkContext();
+            var provider = new DependencyProvider();
+            provider.Package("A", "1.0")
+                .DependsOn("B", "1.0")


I think B needs to be centrally managed here, as B is a top level to A, which effectively means all it's direct deps need to be centrally managed.

B is a project here so that is why it is not "centrally managed" as any package would be.

nkolev92 · 2022-05-20T01:45:16Z

test/NuGet.Core.Tests/NuGet.DependencyResolver.Core.Tests/RemoteDependencyWalkerTests.cs

+
+            provider.Package("B", "1.0")
+                .DependsOn("C", "1.0")
+                .DependsOn("D", "3.0", LibraryDependencyTarget.Package, versionCentrallyManaged: true, libraryDependencyReferenceType: LibraryDependencyReferenceType.None);


Should this be centrally managed?
In the context of A, this isn't centrally managed, it's just a dependency of B.

B is a project that uses CPM, so the package D dependency should be centrally managed.

~~but not in the context of A.~~

A and B are restored separately.
When A is getting restored, A doesn't care about central management configured by B, it just cares about the declared dependencies (and their versions) from B.

This is tricky, as it seems like we don't really recognize the concept of transitively pinned for project 1 but not pinned for project 2.

nkolev92 · 2022-05-20T01:47:57Z

test/NuGet.Core.Tests/NuGet.DependencyResolver.Core.Tests/RemoteDependencyWalkerTests.cs

+            var provider = new DependencyProvider();
+            provider.Package("A", "1.0")
+                .DependsOn("B", "1.0")
+                .DependsOn("D", "1.0", LibraryDependencyTarget.Package, versionCentrallyManaged: true, libraryDependencyReferenceType: LibraryDependencyReferenceType.None);


Do we another another level of dependencies here?

If A is the root node, and it's centrally managed, then all it's dependencies need to be centrally managed and are direct.

…entraltransitive

…nsitive' into dev-marcink-20220504-centraltransitive # Conflicts: # src/NuGet.Core/NuGet.DependencyResolver.Core/Remote/RemoteDependencyWalker.cs # test/NuGet.Core.Tests/NuGet.DependencyResolver.Core.Tests/RemoteDependencyWalkerTests.cs

marcin-krystianc · 2022-06-06T11:08:47Z

A few qs about the test.

I think that all these questions are due to confusion about which dependency is a project and which is a package. I've updated the test so now it should be clearer. Please have another look.

It wouldn't be the worst idea to define an end to end test as well, as the dependency walker tests rely on a lot of configuration.

I implemented ExecuteAsync_TransitiveDependenciesFromNonRootLibraries_AreIgnored - it has smililrar configuration complexity to the walker tests though 😞

nkolev92

Thank you for the contribution @marcin-krystianc and apologies for the delays.

nkolev92 · 2022-06-15T21:32:36Z

test/NuGet.Core.Tests/NuGet.DependencyResolver.Core.Tests/RemoteDependencyWalkerTests.cs

+
+            provider.Package("B", "1.0")
+                .DependsOn("C", "1.0")
+                .DependsOn("D", "3.0", LibraryDependencyTarget.Package, versionCentrallyManaged: true, libraryDependencyReferenceType: LibraryDependencyReferenceType.None);


~~but not in the context of A.~~

A and B are restored separately.
When A is getting restored, A doesn't care about central management configured by B, it just cares about the declared dependencies (and their versions) from B.

This is tricky, as it seems like we don't really recognize the concept of transitively pinned for project 1 but not pinned for project 2.

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

Meant to comment.

…entraltransitive

nkolev92 · 2022-06-20T21:35:44Z

Seems like our CI hook is broken.

cc @zivkan

zivkan · 2022-06-20T22:20:42Z

github webhooks are (were?) having an outage: https://www.githubstatus.com/incidents/gvzcw6sd0xxb

nkolev92 · 2022-06-20T23:18:19Z

Can you rebase again @marcin-krystianc

e9637ed didn't make it in your rebase.

nkolev92 · 2022-06-20T23:18:39Z

Basically NuGet.CommandLine.Test.NuGetPushCommandTest.PushCommand_InvalidInput_V2HttpSource(invalidInput: "https://invalid-2a0358f1-88f2-48c0-b68a-bb150cac00"...)

is failing.

nkolev92

LGTM.

I have a hunch about some other potential bugs we may have with the transitive walking.

I'll ping everyone on the details if it indeed does turn out to be a bug.

nkolev92 · 2022-06-21T00:02:31Z

So the thing I was worried about is that if a package is transitively specified in a child project, the parent project gets a library dependency that's centrally managed.

Turns out it doesn't hurt transitive pinning, but I wonder if it could mess with it in a different way.

This is the test I used (I basically tweaked the test being added in this change).

/// <summary>
        /// A 1.0 -> D 3.0 (Central transitive) ->  E 1.0.0
        ///       -> B 1.0 -> D 2.0.0 (declared)
        ///       -> B 1.0 -> E 2.0.0
        /// </summary>
        [Fact]
        public async Task RestoreNetCore_DependenciesFromRootProjectThatArePinned_ShouldBeIgnored()
        {
            // Arrange
            using var pathContext = new SimpleTestPathContext();

            // Set up solution, project, and packages
            var solution = new SimpleTestSolutionContext(pathContext.SolutionRoot);

            var projectB = CreateProject(pathContext, "B");
            var projectA = CreateProject(pathContext, "A", projectB);

            var packageD200 = new SimpleTestPackageContext("D", "2.0.0");
            var packageD300 = new SimpleTestPackageContext("D", "3.0.0");
            var packageE100 = new SimpleTestPackageContext("E", "1.0.0");
            var packageE200 = new SimpleTestPackageContext("E", "2.0.0");
            packageD300.Dependencies.Add(packageE100);

            await SimpleTestPackageUtility.CreateFolderFeedV3Async(
                pathContext.PackageSource,
                PackageSaveMode.Defaultv3,
                packageD200,
                packageD300,
                packageE100,
                packageE200
            );

            solution.Projects.Add(projectA);
            solution.Projects.Add(projectB);
            solution.Create(pathContext.SolutionRoot);

            AddPackageReferenceToProject(projectB);

            CreateDirectoryPackagesPropsWithVersionForPackageD(pathContext, projectA, "3.0.0");
            CreateDirectoryPackagesPropsWithVersionForPackageDAndE(pathContext, projectB, "2.0.0");

            var args = new string[] {
                    "restore",
                    solution.SolutionPath,
                    "-Verbosity",
                    "detailed",
                };

            // Act
            var r = CommandRunner.Run(
                Util.GetNuGetExePath(),
                pathContext.WorkingDirectory.Path,
                string.Join(" ", args),
                waitForExit: true);

            // Assert
            r.Success.Should().Be(true, because: r.AllOutput);

            var assetsFile = projectA.AssetsFile;

            assetsFile.Libraries.Should().HaveCount(3); // B, D & E.
            var eLibrary = assetsFile.Libraries.Single(e => e.Name.Equals("E"));
            eLibrary.Version.Should().Be(NuGetVersion.Parse("1.0.0"));
            // Local methods
            void CreateDirectoryPackagesPropsWithVersionForPackageD(SimpleTestPathContext pathContext, SimpleTestProjectContext projectContext, string version)
            {
                var directoryPackagesPropsContent =
                    @$"<Project>
                            <ItemGroup>
                                <PackageVersion Include=""D"" Version=""{version}"" />
                            </ItemGroup>
                        </Project>";
                var directoryName = Path.GetDirectoryName(projectContext.ProjectPath);
                File.WriteAllText(Path.Combine(directoryName, $"Directory.Packages.Props"), directoryPackagesPropsContent);
            }

            void CreateDirectoryPackagesPropsWithVersionForPackageDAndE(SimpleTestPathContext pathContext, SimpleTestProjectContext projectContext, string version)
            {
                var directoryPackagesPropsContent =
                    @$"<Project>
                            <ItemGroup>
                                <PackageVersion Include=""D"" Version=""{version}"" />
                                <PackageVersion Include=""E"" Version=""2.0.0"" />
                            </ItemGroup>
                        </Project>";
                var directoryName = Path.GetDirectoryName(projectContext.ProjectPath);
                File.WriteAllText(Path.Combine(directoryName, $"Directory.Packages.Props"), directoryPackagesPropsContent);
            }

            SimpleTestProjectContext CreateProject(SimpleTestPathContext pathContext, string name, SimpleTestProjectContext referencedProject = null)
            {
                var projectContext = SimpleTestProjectContext.CreateNETCoreWithSDK(
                    name,
                    pathContext.SolutionRoot,
                    "net472");

                projectContext.Properties.Add("ManagePackageVersionsCentrally", "true");
                projectContext.Properties.Add("CentralPackageTransitivePinningEnabled", "true");

                if (referencedProject != null)
                    projectContext.AddProjectToAllFrameworks(referencedProject);

                return projectContext;
            }

            void AddPackageReferenceToProject(SimpleTestProjectContext project)
            {
                var xml = project.GetXML();

                ProjectFileUtils.AddItem(
                    xml,
                    "PackageReference",
                    "D",
                    NuGetFramework.AnyFramework,
                    new Dictionary<string, string>(),
                    new Dictionary<string, string>());

                xml.Save(project.ProjectPath);
            }
        }

…entraltransitive

marcin-krystianc · 2022-06-23T09:21:35Z

@nkolev92 I've merged dev branch into mine.

I have a hunch about some other potential bugs we may have with the transitive walking.

Yeah, it is quite messy if you start considering graphs in which different projects have a different sets of central package versions and both use transitive pinning. That is definitely not a common scenario anyway.
I hope that my change removes all the ambiguity. The transitive pining information is going to be respected only for the entry project and ignored for other projects, so it is much simpler to reason about it.
This change also enables some performance improvements in the graph walking algorithm when transitive pinning is enabled (we can do some dictionary lookups instead of walking the entire graph).

ghost · 2022-07-23T19:01:26Z

This PR has been automatically marked as stale because it has no activity for 30 days. It will be closed if no further activity occurs within another 60 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.

nkolev92 · 2022-07-25T18:24:24Z

@jeffkl's out and he'll handle it when back.
We expect this change to go into 7.0.100 of the SDK, 6.4 of NuGet, 17.4 of VS.

It was pretty late in the game for 6.3 so we didn't feel comfortable merging.

…entraltransitive

jeffkl

Thanks for the contribution @marcin-krystianc !

Central transitive dependencies should be considered only for root nodes

a28d352

marcin-krystianc requested a review from a team as a code owner May 4, 2022 11:41

ghost added the Community PRs created by someone not in the NuGet team label May 4, 2022

marcin-krystianc commented May 4, 2022

View reviewed changes

jeffkl self-assigned this May 4, 2022

jeffkl added the Approved for CI label May 4, 2022

Central transitive dependencies should be considered only for root nodes

1612530

jeffkl force-pushed the dev-marcink-20220504-centraltransitive branch from a28d352 to 1612530 Compare May 4, 2022 13:12

jeffkl added Approved for CI and removed Approved for CI labels May 4, 2022

jeffkl reviewed May 4, 2022

View reviewed changes

src/NuGet.Core/NuGet.DependencyResolver.Core/Remote/RemoteDependencyWalker.cs Show resolved Hide resolved

Update src/NuGet.Core/NuGet.DependencyResolver.Core/Remote/RemoteDepe…

5fd0fbd

…ndencyWalker.cs Co-authored-by: Jeff Kluge <jeffkl@microsoft.com>

jeffkl added Approved for CI and removed Approved for CI labels May 5, 2022

nkolev92 reviewed May 6, 2022

View reviewed changes

src/NuGet.Core/NuGet.DependencyResolver.Core/Remote/RemoteDependencyWalker.cs Show resolved Hide resolved

nkolev92 reviewed May 20, 2022

View reviewed changes

marcin-krystianc added 3 commits June 2, 2022 15:04

Merge remote-tracking branch 'origin/dev' into dev-marcink-20220504-c…

bacbd94

…entraltransitive

E2E ExecuteAsync_TransitiveDependenciesFromNonRootLibraries_AreIgnored

5672183

erdembayar added Approved for CI and removed Approved for CI labels Jun 8, 2022

nkolev92 previously approved these changes Jun 16, 2022

View reviewed changes

nkolev92 self-requested a review June 16, 2022 00:37

marcin-krystianc added 2 commits June 16, 2022 12:55

Merge remote-tracking branch 'origin/dev' into dev-marcink-20220504-c…

f981210

…entraltransitive

Add integration test

b7992b4

nkolev92 removed the Approved for CI label Jun 20, 2022

nkolev92 added the Approved for CI label Jun 20, 2022

nkolev92 added Approved for CI and removed Approved for CI labels Jun 20, 2022

nkolev92 approved these changes Jun 20, 2022

View reviewed changes

Merge remote-tracking branch 'origin/dev' into dev-marcink-20220504-c…

06c1cce

…entraltransitive

nkolev92 added Approved for CI and removed Approved for CI labels Jun 23, 2022

nkolev92 requested a review from jeffkl June 23, 2022 17:36

ghost added the Status:No recent activity No recent activity. label Jul 23, 2022

ghost removed the Status:No recent activity No recent activity. label Jul 25, 2022

Merge remote-tracking branch 'origin/dev' into dev-marcink-20220504-c…

95e08c7

…entraltransitive

nkolev92 added Approved for CI and removed Approved for CI labels Aug 10, 2022

jeffkl approved these changes Aug 11, 2022

View reviewed changes

jeffkl merged commit bae08de into NuGet:dev Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Central transitive dependencies should be considered only for root nodes #4611

Central transitive dependencies should be considered only for root nodes #4611

marcin-krystianc commented May 4, 2022

marcin-krystianc May 4, 2022

jeffkl left a comment

marcin-krystianc commented May 5, 2022

jeffkl commented May 5, 2022

nkolev92 left a comment

nkolev92 May 20, 2022

marcin-krystianc Jun 6, 2022

nkolev92 May 20, 2022

marcin-krystianc Jun 6, 2022

nkolev92 Jun 15, 2022 •

edited

nkolev92 May 20, 2022

marcin-krystianc commented Jun 6, 2022

nkolev92 left a comment

nkolev92 Jun 15, 2022 •

edited

nkolev92 commented Jun 20, 2022

zivkan commented Jun 20, 2022 •

edited

nkolev92 commented Jun 20, 2022

nkolev92 commented Jun 20, 2022

nkolev92 left a comment

nkolev92 commented Jun 21, 2022

marcin-krystianc commented Jun 23, 2022

ghost commented Jul 23, 2022

nkolev92 commented Jul 25, 2022

jeffkl left a comment

Central transitive dependencies should be considered only for root nodes #4611

Central transitive dependencies should be considered only for root nodes #4611

Conversation

marcin-krystianc commented May 4, 2022

Bug

Description

PR Checklist

marcin-krystianc May 4, 2022

Choose a reason for hiding this comment

jeffkl left a comment

Choose a reason for hiding this comment

marcin-krystianc commented May 5, 2022

jeffkl commented May 5, 2022

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 May 20, 2022

Choose a reason for hiding this comment

marcin-krystianc Jun 6, 2022

Choose a reason for hiding this comment

nkolev92 May 20, 2022

Choose a reason for hiding this comment

marcin-krystianc Jun 6, 2022

Choose a reason for hiding this comment

nkolev92 Jun 15, 2022 • edited

Choose a reason for hiding this comment

nkolev92 May 20, 2022

Choose a reason for hiding this comment

marcin-krystianc commented Jun 6, 2022

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 Jun 15, 2022 • edited

Choose a reason for hiding this comment

nkolev92 commented Jun 20, 2022

zivkan commented Jun 20, 2022 • edited

nkolev92 commented Jun 20, 2022

nkolev92 commented Jun 20, 2022

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 commented Jun 21, 2022

marcin-krystianc commented Jun 23, 2022

ghost commented Jul 23, 2022

nkolev92 commented Jul 25, 2022

jeffkl left a comment

Choose a reason for hiding this comment

nkolev92 Jun 15, 2022 •

edited

nkolev92 Jun 15, 2022 •

edited

zivkan commented Jun 20, 2022 •

edited