Visual Studio restore is broken as projects are missing from the dependency closure

[ViktorHofer]

Since I applied various fixes to the product over the last weeks that should make the overall IDE experience better I wanted to try them out and had to realize with dismay that the integrated Visual Studio restore is still broken if an upfront restore via the CLI (dotnet restore) isn't performed. The impact of that is that the solution is unusable.

The root cause for that lies in this code path:

runtime/eng/slngen.targets

Lines 8 to 11 in cc649ac

	<!-- Don't include reference projects which compose the microsoft.netcore.app targeting pack (except the current leaf's reference project) as those are referenced by the source project via named references
	and hence don't need to be part of the solution file (only P2Ps need to).
	Include the reference project in the solution file if it targets more than just NetCoreAppCurrent as other frameworks like .NETFramework, .NETStandard or older .NETCoreApp ones require it. -->
	<IncludeInSolutionFile Condition="'$(IsNETCoreAppRef)' == 'true' and '$(MSBuildProjectName)' != '$(SlnGenMainProject)' and '$(TargetFramework)' == '$(NetCoreAppCurrent)' and ('$(TargetFrameworks)' == '' or '$(TargetFrameworks)' == '$(NetCoreAppCurrent)')">false</IncludeInSolutionFile>

This causes slngen to not include reference projects which are part of the shared framework and aren't shipping out-of-band. Even though, shared framework reference assemblies are only referenced via "Reference" items by source projects, the reference projects themselves reference each other via ProjectReferences. Note that VS restore requires all dependencies expressed via ProjectReference items to be present in the solution file and ignores Reference items as those point to the compiled assembly and there's nothing for restore to do on those.

As an example, ref/System.Drawing.Common.csproj inside System.Runtime.sln is depending on ref/System.Collections.NonGeneric.csproj but as it is missing, the restore inside Visual Studio fails.

If I remove the condition in the slngen.targets file and re-generate all solution files (dotnet.cmd build src\libraries\slngen.proj), the solution files get larger by about 50 ref projects (as the solution now includes all dependencies) but the solution restore finally works and no up-front restore is necessary anymore. Note that because the reference projects are very small and fast to evaluate, the solution load time doesn't increase noticeably and the solution doesn't look overloaded as all the added projects are grouped inside the "ref" folder (tested with VS 2022 17.2 Preview 1).

Because of that I propose that we remove the condition in the slngen.targets file with the caveat that solutions get bigger but with the huge win that up-front restores aren't necessary anymore. That means instead of:

build.cmd libs.sfx /p:RefOnly=true
build.cmd -restore libs
build.cmd -vs System.Runtime

I can now directly open VS after the libs.ref subset is built:

build.cmd libs.sfx /p:RefOnly=true
start src\libraries\System.Runtime\System.Runtime.sln

or just continue to use the VS switch for convenience (which also uses the repo local SDK which is useful when a matching one isn't globally installed):

build.cmd libs.sfx /p:RefOnly=true
build.cmd -vs System.Runtime

Ultimately at some point in the future when source projects use ProjectReference items instead of Reference items to dynamically express their dependencies, the build.cmd libs.ref step won't be necessary anymore either and a dev will be able to directly open a VS solution file from a fresh clone.

@ericstj @safern @joperezr @carlossanlop any objections to what I'm proposing?

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Since I applied various fixes to the product over the last weeks that should make the overall IDE experience better I wanted to try them out and had to realize with dismay that the integrated Visual Studio restore is still broken if an upfront restore via the CLI (dotnet restore) isn't performed. The impact of that is that the solution is unusable.

The root cause for that lies in this code path:

runtime/eng/slngen.targets

Lines 8 to 11 in cc649ac

	<!-- Don't include reference projects which compose the microsoft.netcore.app targeting pack (except the current leaf's reference project) as those are referenced by the source project via named references
	and hence don't need to be part of the solution file (only P2Ps need to).
	Include the reference project in the solution file if it targets more than just NetCoreAppCurrent as other frameworks like .NETFramework, .NETStandard or older .NETCoreApp ones require it. -->
	<IncludeInSolutionFile Condition="'$(IsNETCoreAppRef)' == 'true' and '$(MSBuildProjectName)' != '$(SlnGenMainProject)' and '$(TargetFramework)' == '$(NetCoreAppCurrent)' and ('$(TargetFrameworks)' == '' or '$(TargetFrameworks)' == '$(NetCoreAppCurrent)')">false</IncludeInSolutionFile>

This causes slngen to not include reference projects which are part of the shared framework and aren't shipping out-of-band. Even though, shared framework reference assemblies are only referenced via "Reference" items by source projects, the reference projects themselves reference each other via ProjectReferences. Note that VS restore requires all dependencies expressed via ProjectReference items to be present in the solution file and ignores Reference items as those point to the compiled assembly and there's nothing for restore to do on those.

As an example, ref/System.Drawing.Common.csproj inside System.Runtime.sln is depending on ref/System.Collections.NonGeneric.csproj but as it is missing, the restore inside Visual Studio fails.

If I remove the condition in the slngen.targets file and re-generate all solution files (dotnet.cmd build src\libraries\slngen.proj), the solution files get larger by about 50 ref projects (as the solution now includes all dependencies) but the solution restore finally works and no up-front restore is necessary anymore. Note that because the reference projects are very small and fast to evaluate, the solution load time doesn't increase noticeably and the solution doesn't look overloaded as all the added projects are grouped inside the "ref" folder (tested with VS 2022 17.2 Preview 1).

Because of that I propose that we remove the condition in the slngen.targets file with the caveat that solutions get bigger but with the huge win that up-front restores aren't necessary anymore. That means instead of:

build.cmd libs.ref
build.cmd -restore libs
build.cmd -vs System.Runtime

I can now directly open VS after the libs.ref subset is built:

build.cmd libs.ref
start src\libraries\System.Runtime\System.Runtime.sln

or just continue to use the VS switch for convenience (which also uses the repo local SDK which is useful when a matching one isn't globally installed):

build.cmd libs.ref
build.cmd -vs System.Runtime

Ultimately at some point in the future when source projects use ProjectReference items instead of Reference items to dynamically express their dependencies, the build.cmd libs.ref step won't be necessary anymore either and a dev will be able to directly open a VS solution file from a fresh clone.

@ericstj @safern @joperezr @carlossanlop any objections to what I'm proposing?

Author:	ViktorHofer
Assignees:	-
Labels:	area-Infrastructure-libraries
Milestone:	-

[ViktorHofer]

Would love to hear people's opinion about the above described issue. What I heard from the recent community survey is that one of the top pain points is that solution files in our repositories can't be opened without ceremony involved. Fixing the VS restore by making sure that the dependency closure is satisfied by the solution files would get us closer to the community's wish.

[joperezr]

I'm not opposed to what you are proposing, but I think that is still not what the community wants. The main ask here would be to be able to open a solution in VS after a clean git clone and even with your proposal, this wouldn't be possible. In fact, I'm not even sure that is something we can ever achieve given the fact that a lot of our infrastructure for building lives in Arcade which needs to be restored at least once before opening a solution, so even though with your proposal the "ceremony" is being shorter, it is still there. Perhaps something we can consider instead of making the more expensive changes proposed, is to consolidate the current ceremony into a single command.

The current upfront restore in a traditional machine with a decent network, doesn't really take that much (around 2 minutes). I am sure that with your proposed changes this is reduced greatly, but have you measured that and if so can you share some of the perf impact?

I guess my uber point is, before we go about and make some changes that will have notable differences to all (people will start seeing 50+ projects inside the ref/ folder in the solution) we should measure and make sure that the cost is worth the benefit, especially since the end result is not meeting the desired community result.

[ericstj]

cc @stephentoub @eerhardt

I guess the tradeoff for some here is the sln size and build performance. You've also done more work recently to improve that.
Can you measure the difference for incremental build after a small change in the src library in some common SLNs before and after and share it?

For example System.Runtime, System.Text.Json, and Microsoft.Extensions.Logging? I think those are somewhat representative of the developer workflows for bottom, middle, and top of stack.

[eerhardt]

and Microsoft.Extensions.Logging

Can you change that to Microsoft.Extensions.Hosting? That one is the most "top of the stack" since it references almost everything (including Logging).

My opinion is roughly inline with @joperezr above - if I need to run something up front, I'd rather that "up front" cost be more and my incremental cost (building constantly in VS, running tests, etc) be lower. After pulling new code, I kick off a repo libs+clr -rc Release build. I need to do that in order to run unit tests anyway. So having VS load more projects and become slower to load/build/etc isn't worth the tradeoff IMO.

[ViktorHofer]

Can you change that to Microsoft.Extensions.Hosting? That one is the most "top of the stack" since it references almost everything (including Logging).

I spent some more time thinking about how to fix the current broken behavior without increasing the build graph. Out-of-band projects already build on-top of the shared framework since we split the libs build into "libs.sfx" and "libs.oob". As these projects already sit on the top, we can just use the default targeting pack references for OOBs and remove the ProjectReferences that point to inbox projects. Note that this is already the behavior for most OOB source projects. Overall that will actually make the build graph and by that, solution files smaller for OOBs.

If I don't hear any objections I would like to give that a try and prepare a PR that will better demonstrate what I'm proposing.