Difference in transitivity behavior between graph and non-graph restore

dfederm commented 1 year ago

NuGet Product Used

MSBuild.exe

Product Version

17.6

Worked before?

Probably not

Impact

Other

Repro Steps & Context

When a project in a graph doesn't support restore, it seems that transitivity effectively stops for non-graph restores, but is allowed for graph restores. The referenced project does not get added to project.assets.json, and thus its dependencies (transitive dependencies for the main project) are missed.

Attached repro: graph-restore-repro.zip

Non-graph:

Graph:

I haven't been able to reason about this situation enough, but it seems like the non-graph restore is "wrong" here.

This is a simple graph of A -> B -> C. B here has restore disabled (in my real scenario, B and C here were vcxproj which do not support PackageReference). All 3 are restored (or not) properly, but in the non-graph case C is missing as a direct project reference to A due to the missing transitivity.

Note that the impact here is that enabling static graph restore changes build behavior, not just restore behavior.

Verbose Logs

No response

erdembayar commented 1 year ago

@dfederm I just want to make sure when graph mean msbuild /t:Restore /p:RestoreUseStaticGraphEvaluation=true and non-graph mean msbuild /t:Restore, right?

dfederm commented 1 year ago

That is correct

erdembayar commented 1 year ago

@nkolev92 I can repro this, at least project.asset.json looks very different, left one is from with static graph restore and right side is normal restore.

cc @jeffkl

nkolev92 commented 1 year ago

In a DBP, there's a:

<Project>
  <!-- Disable restore for this project -->
  <PropertyGroup>
    <SkipResolvePackageAssets>true</SkipResolvePackageAssets>
  </PropertyGroup>
  <Target Name="_IsProjectRestoreSupported" Returns="@(_ValidProjectsForRestore)" />
</Project>

_IsProjectRestoreSupported is not supposed to be used at all. This basically cuts out NuGet reading this project. If NuGet can't read the project, then it can't include it in the graph.

I'll find a recent internal thread and add you all to it where I pointed this out.

I think there's no bug here, and the action is to remove all the _IsProjectRestoreSupported customization in all repos.

dfederm commented 1 year ago

So I'm using _IsProjectRestoreSupported as an easy way to repro. In my actual repro, projects B and C are vcxprojs, which do not support restore.

nkolev92 commented 1 year ago

vcxprojs should still be importing the restore targets.

In the e-mail I sent, there's some mega repos that have some corext logic that excludes the vcxprojs, can you check if those have the same thing?

dfederm commented 1 year ago

Looks like the reason the customer repo is doing this is to "stop" transitivity. In general they need transitivity, but redefining _IsProjectRestoreSupported to (in a hacky way) disable restore also disables transitivity for projects "lower" in the graph. This project effectively ILMerges its dependencies, so if upstream projects are allowed to be transitive in this subgraph, there are weird ambiguity errors.

Seems like projects transitivity is very weirdly linked with NuGet rather than being part of the SDK.

So the question is, what's the supported way to "stop" transitivity beyond a specific project?

Note that I've tried <DisableTransitiveProjectReferences>true</DisableTransitiveProjectReferences>, but as mentioned, most of the repo depends heavily on transitivity. Also, it seems like DisableTransitiveProjectReferences doesn't apply as one would expect, as if some transitive dependency disables transitivity, the top-level project still seems to have full transitivity. eg if A -> B -> C -> ... -> Z, and C set DisableTransitiveProjectReferences=true, then A would still have transitive project refs on everything despite C not, which to me at least is very unexpected. I'd have expected it to do "roll up" transitivity instead.

NuGet / Home