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#2521jainaashish merged 4 commits intodevfrom
Conversation
7597d58 to
d3dc97e
Compare
d3dc97e to
f5a9391
Compare
|
@NuGet/nuget-client 🔔 |
| { | ||
| public class DependencyGraphSpec | ||
| { | ||
| public static readonly string DGSpecFileName = "{0}.nuget.dgspec.json"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I like it, will change it accordingly
| new Configuration.PackageSource(packageSource.Path) | ||
| }); | ||
|
|
||
| using (var testSolutionManager = new TestSolutionManager(true)) |
There was a problem hiding this comment.
Add a named parameter to explain the pesky true literal.
There was a problem hiding this comment.
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
| testLogger, | ||
| CancellationToken.None); | ||
|
|
||
| // Assert |
There was a problem hiding this comment.
You should first assert:
Assert.NotEmpty(restoreSummaries);
| sourceRepositoryProvider.GetRepositories(), | ||
| Guid.Empty, | ||
| false, | ||
| true, |
There was a problem hiding this comment.
Add named parameters for false and true.
| true, | ||
| testLogger, | ||
| CancellationToken.None); | ||
|
|
There was a problem hiding this comment.
Assert.NotEmpty(restoreSummaries);
| foreach (var project in projects) | ||
| var projects = ((await solutionManager.GetNuGetProjectsAsync()).OfType<IDependencyGraphProject>()).ToList(); | ||
|
|
||
| for (var i=0; i< projects.Count; i++) |
There was a problem hiding this comment.
You can simply foreach here.
There was a problem hiding this comment.
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
|
|
||
| foreach (var dependentPackageSpec in persistedDGSpec.GetClosure(packageSpec.RestoreMetadata.ProjectUniqueName)) | ||
| { | ||
| if (!projects.Any(p => PathUtility.GetStringComparerBasedOnOS().Equals(p.MSBuildProjectPath, dependentPackageSpec.RestoreMetadata.ProjectPath)) && |
There was a problem hiding this comment.
-
If you have
nprojects and all projects reference the same unloaded project, we'll perform this check up ton^2times. 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.
32754ac to
8c08217
Compare
|
Yay! |
| foreach (var project in projects) | ||
| var uniqueProjectDependencies = new HashSet<string>(stringComparer); | ||
|
|
||
| var projects = ((await solutionManager.GetNuGetProjectsAsync()).OfType<IDependencyGraphProject>()).ToList(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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: