project-system
project-system copied to clipboard
dotnet
NuGet restore may happen multiple times for multi-target projects
Repro: 1, Open Roslyn solution, after everything settled, close VS.
2, Restart VS, open Roslyn solution again, notice it reaches intellisense ready state quickly after the solution explorer shows the tree
3, close the solution (keep VS open)
4, reopen Roslyn solution in the same VS session Notice: it takes much longer times (minutes) to reach intellinsense ready state. Check asset files in the obj folder. In about 40 projects, they were modified. It doesn't happen when the solution is opened in a fresh booted VS. The content of those files are not changed.
The reason it happens is that for multiple target projects, NuGet restore happens for one of the target framework once. That removes all references for other target frameworks in those projects (and projects in their dependency closure). The rest target frameworks will be added later, and projects will eventually return to the correct state. But that leads a big perf hit, including extra number of design time builds. Also, if projects are not changed within the session, it will eliminate possibility of optimizations like skipping updating build cache, and break incremental build as well.
-
@drewnoakes
-
@panopticoncentral
Hi @drewnoakes,
I see that this still reproes when VS opens the Roslyn project.
To reduce the repro steps I wanted to create a more simple project. I created a C# ConsoleApp project targeting net472, and added a project reference to a class library targeting net472;net48 with the nuGet package Newtonsoft, but I couldn't repro it in here.
Any suggestion on what project configuration can repro this ?
I noticed that when using Roslyn project, the cache file are not modified the first time you restart and open the solution. I see that sometimes it reproes after many tries.
@ocallesp it would be great to narrow down to a single project. Does it repro with the smaller Compilers.sln for example? You could try unloading half the projects at a time, and seeing if it still reproduces.
Does not repro always.
Using this simple project https://github.com/ocallesp/Repro7288 I found that during solution load some cache files are modified (we can detect this with git status). When the solution load finishes, the cache files appear as nothing has changed because the final modification are the same as it was originally.
modified: ClassLibrary.csproj.nuget.dgspec.json
modified: project.assets.json
modified: project.nuget.cache
@drewnoakes,
When a project is targeting many target frameworks, NET Project System might receive ProjectRestoreInfo containing only one target framework. This will cause Project System to do the restore for only one targe framework, and at later point in time Project System will receive another ProjectRestoreInfo with all target frameworks and do the restore. This is doing an extra restore because Project System should do a restore only when the ProjectRestoreInfo contains all the target frameworks.
Given that ProjectRestoreInfo get the list of target frameworks from the property NuGetTargetMoniker in the NuGetRestore.SchemaName
It seems that we can avoid the extra nuget restore
if (ProjectRestoreInfo.TargetFrameworks < UnconfiguredProject.TargetFrameworks) {
// Don't do any anything. Avoid this extra step
}
I am wondering if adding this validation will affect other behaviors.
Currently we're using CPS's ConfiguredProjectDataSourceJoinBlock to merge restore data across configurations into a single unconfigured snapshot via the callback into MergeRestoreInputs:
https://github.com/dotnet/project-system/blob/6eb553ce01f4655fff326eb5980582dde1df8d8b/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/PackageRestore/PackageRestoreUnconfiguredInputDataSource.cs#L38-L44
We need a way to ensure that all configurations are loaded before we produce a joined output here.
ProjectRestoreInfo.TargetFrameworks < UnconfiguredProject.TargetFrameworks
This might work, but I feel it would open up a race condition between the dataflow computation and the project.
@lifengl would it be possible to build protection against this class of bug into ConfiguredProjectDataSourceJoinBlock directly?
@ocallesp this would be a good topic to bring to the CPS meeting this week. I'm not sure offhand what the best approach would be here.
The problem is that CPS does not own any logic to load additional configurations, so it doesn't have anything to assume what would be loaded eventually. We cannot make any logic inside ConfiguredProjectDataSourceJoinBlock to do that, because that block takes a collection of configured projects as its inputs, and then gather data from them. It does not assume there is any logic connection between those configurations, nor you would include more in the future, so I don't think it is a bug in the block. If you expect more configurations, you need either add a filtering before feeding a set of configuredProjects into this JoinBlock, or have additional filtering after this block. Maybe join another block to gather the expected configurations to be loaded by chaining into configuration group service.
or, maybe I misunderstood it, it would be a problem in ConfiguredProjectDataSourceJoinBlock, if the input always has 3 configurations, but it generates data when receive 2 pieces of data
I'm going to reopen this as I'd like to validate that this is no longer an issue. The linked PR was unmerged. I don't know when I'll get to this however.