[Bug]: props/targets files in packages can modify VS restore but not CLI restore

[zivkan]

NuGet Product Used

Other/NA

Product Version

all

Worked before?

No response

Impact

No response

Repro Steps & Context

Packages can ship MSBuild props and targets files, and if it meets NuGet's naming convention, NuGet will import these files in the obj/project.nuget.g.props and obj/project.nuget.g.targets files that the .NET SDK itself imports as part of project evaluation.

In order to make restore as deterministic as possible, on command line restores, ExcludeRestorePackageImports is set to true, and the .g.props and .g.targets files that NuGet writes to the obj/ folder are all conditioned on '$(ExcludeRestorePackageImports)' != 'true'. This means, that packages that try to add <PackageReference /> items, or set properties like <AssetTargetFallback> can not change NuGet's restore inputs. Running restore more than once, as long as no errors occur, the second restore should always be "no op" (all projects are up to date).

However, in Visual Studio, NuGet gets restore inputs from the project system, not from MSBuild directly (both for performance reasons, and also to enable project system to support in-memory, not yet written to disk changes). However, Visual Studio's project systems do not use ExcludeRestorePackageImports.

Note that when a solution contains at least 1 SDK style project, the way project system and NuGet are integrated will cause restores to happen automatically when restore input changes are detected. When there are no SDK style projects in the solution, NuGet will only restore when the customer right clicks the solution and clicks restore, when a build starts and the relevant VS option is enabled, or if some other VS component calls NuGet's API to trigger a restore. However, since .NET and .NET Core only work with SDK style projects, we'll assume this is the case.

Therefore, it's possible that in Visual Studio for a package to change NuGet's restore inputs, which will cause the second (and maybe subsequent) restores to have a different result than the first restore. In most cases this will cause Visual Studio to require multiple restores to get to a "stable" state. However, there exist scenarios where no stable state is ever reached, causing an infinite restore loop. When this happens in an SDK style project, this cause consume 100% CPU time since the project system will detect restore input changes, notify NuGet and another restore will happen automatically.

Note that prior to Visual Studio 2022 17.2, NuGet did not take AssetTargetFallback into account when evaluating the restore graph, meaning that package dependencies were not downloaded (#5957). Since this was fixed in 17.2, it increases the risk of this issue's problem if a package modifies AssetTargetFallback in its own props/targets file.

Verbose Logs

No response

[zivkan]

After an internal discussion, the decision not to fix this was made.

Project system gets NuGet's restore inputs though a process called design time build (DBT), but design time build is used for more than just NuGet. DBT batches all the targets together into a single MSBuild execution, which improves performance. Trying to set ExcludeRestorePackageImports for just the NuGet related targets, while disabling it for the intellisense related targets would require additional MSBuild evaluations, which would harm performance.

Given the very low frequency that this type of issue happens, we will instead focus on a pack warning for package authors, and some kind of warning (yet to be determined) for package consumers in Visual Studio, when one of the packages used in their solution has this issue.