Persisting and reading dg spec for unloaded or out of current solution projects #2521

jainaashish · 2018-11-06T07:05:50Z

Bug

Fixes: NuGet/Home#5820
Regression: Yes/No
If Regression then when did it last work:
If Regression then how are we preventing it in future:

Fix

Details: Currently restore inside VS fails when there is a unloaded project in solution or some projects are not part of current solution, even though those projects are already restored and up to date. So with this PR, we'll persist the dependency graph with each restore for every project in the build intermediate output directory (/obj) and use that persisted graph to complete restore when there are some missing projects.

Spec - https://github.com/NuGet/Home/wiki/Allow-restore-to-succeed-for-unloaded-projects-in-Visual-Studio

Testing/Validation

Tests Added: Yes
Reason for not adding tests:
Validation done:

…olution projects

jainaashish · 2019-01-04T17:34:08Z

@NuGet/nuget-client 🔔

dtivel · 2019-01-04T18:30:58Z

src/NuGet.Core/NuGet.ProjectModel/DependencyGraphSpec.cs

@@ -14,6 +14,8 @@ namespace NuGet.ProjectModel
 {
    public class DependencyGraphSpec
    {
+        public static readonly string DGSpecFileName = "{0}.nuget.dgspec.json";


This is not the file name. It's a format string. Instead of exposing this as a static readonly field (or better yet const) and requiring callers to format it, how about moving this format string inside a static method:

public static string GetDgSpecFileName(string projectName) { ... }

Then callers won't have to know how to format it.

I like it, will change it accordingly

dtivel · 2019-01-04T18:34:08Z

...kageManagement.VisualStudio.Test/ProjectSystems/LegacyPackageReferenceRestoreUtilityTests.cs

+                        new Configuration.PackageSource(packageSource.Path)
+                    });
+
+                using (var testSolutionManager = new TestSolutionManager(true))


Add a named parameter to explain the pesky true literal.

Named parameter is foo which is not even used. I dont want to refactor it with this PR. But we should just delete this parameter. For now, this doesn't make any sense to even make it a named parameter

dtivel · 2019-01-04T18:35:41Z

...kageManagement.VisualStudio.Test/ProjectSystems/LegacyPackageReferenceRestoreUtilityTests.cs

+                        testLogger,
+                        CancellationToken.None);
+
+                    // Assert


You should first assert:

Assert.NotEmpty(restoreSummaries);

dtivel · 2019-01-04T18:36:04Z

...kageManagement.VisualStudio.Test/ProjectSystems/LegacyPackageReferenceRestoreUtilityTests.cs

+                        sourceRepositoryProvider.GetRepositories(),
+                        Guid.Empty,
+                        false,
+                        true,


Add named parameters for false and true.

dtivel · 2019-01-04T18:36:23Z

...kageManagement.VisualStudio.Test/ProjectSystems/LegacyPackageReferenceRestoreUtilityTests.cs

+                        true,
+                        testLogger,
+                        CancellationToken.None);
+


Assert.NotEmpty(restoreSummaries);

dtivel · 2019-01-04T18:38:21Z

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/DependencyGraphRestoreUtility.cs

-            foreach (var project in projects)
+            var projects = ((await solutionManager.GetNuGetProjectsAsync()).OfType<IDependencyGraphProject>()).ToList();
+
+            for (var i=0; i< projects.Count; i++)


You can simply foreach here.

Foreach on a list still creates a IEnumable instance and waste double memory. This might not make a huge difference but a good practice to use for for list instead of foreach

dtivel · 2019-01-04T18:43:16Z

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/DependencyGraphRestoreUtility.cs

+
+                        foreach (var dependentPackageSpec in persistedDGSpec.GetClosure(packageSpec.RestoreMetadata.ProjectUniqueName))
+                        {
+                            if (!projects.Any(p => PathUtility.GetStringComparerBasedOnOS().Equals(p.MSBuildProjectPath, dependentPackageSpec.RestoreMetadata.ProjectPath)) &&


If you have n projects and all projects reference the same unloaded project, we'll perform this check up to n^2 times. I would add an initial check:

!uniqueProjectDependencies.Contains(dependentPackageSpec.RestoreMetadata.ProjectUniqueName) &&

Outside these loops (like here you can memoize PathUtility.GetStringComparerBasedOnOS() with a local and use it here and here.

natemcmaster · 2019-01-08T17:36:47Z

Yay!

nkolev92 · 2019-01-08T18:17:48Z

src/NuGet.Core/NuGet.PackageManagement/BuildIntegration/DependencyGraphRestoreUtility.cs

-            foreach (var project in projects)
+            var uniqueProjectDependencies = new HashSet<string>(stringComparer);
+
+            var projects = ((await solutionManager.GetNuGetProjectsAsync()).OfType<IDependencyGraphProject>()).ToList();


How this behave in the case in which we are still waiting for a nomination?

Didn't we want to distinguish between the NU1105 (the project is not in the solution & you have never restored) and NU110, the project-system is slow.

If there is a persisted DGSpec then it will consume it and continue with the restore. And when actual nomination happens for that project, then restore will either be NoOp (if there was no change in the project) or restore will consume the new changes. So IMO this experience is better than showing bunch of errors and the final restore being successful.

jainaashish mentioned this pull request Nov 8, 2018

Support VS 2019 Solution Filter feature. Also supports project not in solution, or unloaded projects. Need to restore complete solution (via CLI or VS) first. NuGet/Home#5820

Closed

jainaashish changed the title ~~[wip] persisting and reading dg spec for unloaded or out of current solution projects~~ [poc] persisting and reading dg spec for unloaded or out of current solution projects Nov 21, 2018

jainaashish force-pushed the dev-asja-PersistDGSpec branch from 7597d58 to d3dc97e Compare December 6, 2018 06:28

[wip] persisting and reading dg spec for unloaded or out of current s…

f5a9391

…olution projects

jainaashish force-pushed the dev-asja-PersistDGSpec branch from d3dc97e to f5a9391 Compare December 19, 2018 18:48

Added test

a50b71d

jainaashish changed the title ~~[poc] persisting and reading dg spec for unloaded or out of current solution projects~~ Persisting and reading dg spec for unloaded or out of current solution projects Dec 20, 2018

jainaashish requested review from PatoBeltran, nkolev92, zivkan, dominoFire and heng-liu December 20, 2018 18:13

jainaashish requested a review from dtivel January 4, 2019 17:33

dtivel requested changes Jan 4, 2019

View reviewed changes

Addressed comments

25ad244

dtivel approved these changes Jan 5, 2019

View reviewed changes

Removed writing dgspec for ENV variable

8c08217

jainaashish force-pushed the dev-asja-PersistDGSpec branch from 32754ac to 8c08217 Compare January 7, 2019 19:58

jainaashish merged commit aee50ea into dev Jan 8, 2019

nkolev92 reviewed Jan 8, 2019

View reviewed changes

nkolev92 deleted the dev-asja-PersistDGSpec branch January 9, 2019 21:59

natemcmaster mentioned this pull request Feb 21, 2019

Remove unneeded dependencies from Components.sln dotnet/aspnetcore#7799

Closed

This was referenced Mar 5, 2019

Unable to overwrite dgspec.json file in Docker scenario NuGet/Home#7836

Closed

When doing a NoOp restore, avoid *.dgspec.json write in obj directory NuGet/Home#7854

Closed

dtivel mentioned this pull request May 7, 2019

Test: remove duplicate protocol tests #2847

Merged

nkolev92 mentioned this pull request Feb 4, 2020

Unloading a project in the middle of large graph can lead to hundreds of errors NuGet/Home#4724

Closed

nkolev92 mentioned this pull request Mar 20, 2020

Improve solution restore performance by avoiding unnecessary disk hits when the projects are in Visual Studio #3306

Merged

nkolev92 mentioned this pull request May 11, 2020

Build in visual studio fails due to incomplete package restore when projects are unloaded NuGet/Home#1830

Closed

nkolev92 mentioned this pull request Apr 14, 2021

Core projects fail to build when they reference other projects not included to solution dotnet/project-system#2759

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persisting and reading dg spec for unloaded or out of current solution projects #2521

Persisting and reading dg spec for unloaded or out of current solution projects #2521

jainaashish commented Nov 6, 2018 •

edited

Loading

jainaashish commented Jan 4, 2019

dtivel Jan 4, 2019

jainaashish Jan 4, 2019

dtivel Jan 4, 2019

jainaashish Jan 4, 2019

dtivel Jan 4, 2019

dtivel Jan 4, 2019

dtivel Jan 4, 2019

dtivel Jan 4, 2019

jainaashish Jan 4, 2019

dtivel Jan 4, 2019

natemcmaster commented Jan 8, 2019

nkolev92 Jan 8, 2019

jainaashish Jan 9, 2019

Persisting and reading dg spec for unloaded or out of current solution projects #2521

Persisting and reading dg spec for unloaded or out of current solution projects #2521

Conversation

jainaashish commented Nov 6, 2018 • edited Loading

Bug

Fix

Testing/Validation

jainaashish commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainaashish commented Nov 6, 2018 •

edited

Loading