Prevent restore loop in VS #31798
Prevent restore loop in VS #31798
Conversation
ghost
added
ViktorHofer
requested review from
dsplaisted
and removed request for
dsplaisted
|
@sfoslund @rainersigwald can you please take a look? In combination with dotnet/msbuild#8660 this fixes the long standing issues that devs need to manually install the .NET Framework reference assembly packs when building .NET Framework apps inside VS as the targeting packs weren't restored automatically. I'm not exactly sure that this is the right fix and I hope that you can help @drewnoakes find the right solution. |
|
|
||
| <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="$(MicrosoftNETFrameworkReferenceAssembliesLatestPackageVersion)" IsImplicitlyDefined="true" PrivateAssets="All" | ||
| Condition="'$(_FullFrameworkReferenceAssemblyPaths)' == '' and '@(_ExistingReferenceAssembliesPackageReference)' == ''"/> | ||
| Condition="('$(_FullFrameworkReferenceAssemblyPaths)' == '' or $(_FullFrameworkReferenceAssemblyPaths.Contains('microsoft.netframework.referenceassemblies'))) and '@(_ExistingReferenceAssembliesPackageReference)' == ''"/> |
There was a problem hiding this comment.
I don't like this, but I can't see a better way.
I was hoping that we could add the PackageReference with some special metadata AddedByIncludeTargetingPackReference, and then check on that in the filter, but it doesn't work, because the subsequent evaluation (with the package ref included) doesn't actually have the PackageReference item, just its consequences through project.assets.json.
There was a problem hiding this comment.
Instead of doing a string contains check on a full path, would it be possible to check whether there are any ResolvedCompileFileDefinitions items (which come from ResolvePackageAssets) with NuGetPackageId metadata that starts with "microsoft.netframework.referenceassemblies"?
There was a problem hiding this comment.
@dsplaisted I tried this idea today and couldn't get it to work. I think the reason is that adding AfterTargets="ResolvePackageAssets" pushes this target too late to add another package reference successfully, so we're back to having a restore loop.
I cannot see a better approach forward for this PR than the Contains check unfortunately. Thankfully the string we're looking for is quite unique.
As an aside, the .NET Project System is supposed to detect such restore loops and that's clearly not working as restores in VS run indefinitely, burning CPU.
There was a problem hiding this comment.
As an aside, the .NET Project System is supposed to detect such restore loops and that's clearly not working as restores in VS run indefinitely, burning CPU.
Tracking this in dotnet/project-system#9050. I'm working on a fix now.
There was a problem hiding this comment.
The VS restore cycle detection has been fixed in dotnet/project-system#9051.
The `GetReferenceAssemblyPaths` target adds a `PackageReference` to `Microsoft.NETFramework.ReferenceAssemblies` when needed. In VS, this new package causes VS to nominate a NuGet restore then perform another design-time build. That second design-time build was previously determining that the `PackageReference` was no longer needed, then removing it. The task's `FullFrameworkReferenceAssemblyPaths` output parameter contains the full path of the reference assembly folder in such a case, which fails the empty-string test. This change tests whether the output parameter is a reference to the package, and if so allows the `PackageReference` to remain.
drewnoakes
force-pushed
the
fix-19506-ref-assembly-restore-loop
branch
from
217b7e8 to
25a743b
Compare
|
Per Daniel offline, there wasn't a better alternative solution to the above comments. This has been approved and 4xx is open. |
5 participants
Fixes #19506
The
GetReferenceAssemblyPathstarget adds aPackageReferencetoMicrosoft.NETFramework.ReferenceAssemblieswhen needed.In VS, this new package causes VS to nominate a NuGet restore then perform another design-time build.
That second design-time build was previously determining that the
PackageReferencewas no longer needed, then removing it. The task'sFullFrameworkReferenceAssemblyPathsoutput parameter contains the full path of the reference assembly folder in such a case, which fails the empty-string test.This change tests whether the output parameter is a reference to the package, and if so allows the
PackageReferenceto remain.I'm not sure whether this is the best approach to fixing this, but I identified this as a possible fix and verified it works correctly (when combined with dotnet/msbuild#8660) so wanted to capture it here in a PR for others to build on.