Restore with static graph evaluation fails with an NRE when a TargetFramework is specified

[nkolev92]

Take the following project for example:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;net5.0</TargetFrameworks>
  </PropertyGroup>

</Project>

Run the following command:

msbuild /t:restore /p:TargetFramework="net5.0" /p:RestoreUseStaticGraphEvaluation="true"

Actual

See output below.

C:\Code\Temp\MultiTargetted [master]> msbuild /t:restore /p:TargetFramework="net5.0" /p:RestoreUseStaticGraphEvaluation="true"
Microsoft (R) Build Engine version 17.2.0-preview-22116-01+7d926d7ab for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 20-Apr-22 5:09:34 PM.
Project "C:\Code\Temp\MultiTargetted\MultiTargetted.csproj" on node 1 (Restore target(s)).
Restore:
  Determining projects to restore...
  Evaluated 1 project(s) in 1259ms (1 builds, 0 failures).
C:\Program Files\Microsoft Visual Studio\2022\Preview\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error : Object reference not set to an instance of an object. [C:\Code\Temp\MultiTargetted\MultiTargetted.
csproj]
Done Building Project "C:\Code\Temp\MultiTargetted\MultiTargetted.csproj" (Restore target(s)) -- FAILED.


Build FAILED.

"C:\Code\Temp\MultiTargetted\MultiTargetted.csproj" (Restore target) (1) ->
(Restore target) ->
  C:\Program Files\Microsoft Visual Studio\2022\Preview\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error : Object reference not set to an instance of an object. [C:\Code\Temp\MultiTargetted\MultiTargette
d.csproj]

    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:02.11

Run it without static graph, and note that there's no failure.

msbuild /t:restore /p:TargetFramework="net5.0"

Expected

C:\Code\Temp\MultiTargetted [master]> msbuild /t:restore /p:TargetFramework="net5.0"
Microsoft (R) Build Engine version 17.2.0-preview-22116-01+7d926d7ab for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 20-Apr-22 5:09:39 PM.
Project "C:\Code\Temp\MultiTargetted\MultiTargetted.csproj" on node 1 (Restore target(s)).
_GetAllRestoreProjectPathItems:
  Determining projects to restore...
Restore:
  Assets file has not changed. Skipping assets file writing. Path: C:\Code\Temp\MultiTargetted\obj\project.assets.json
  Restored C:\Code\Temp\MultiTargetted\MultiTargetted.csproj (in 81 ms).

  NuGet Config files used:
      C:\Code\NuGet.Config
      C:\Users\Roki2\AppData\Roaming\NuGet\NuGet.Config
      C:\Program Files (x86)\NuGet\Config\Microsoft.VisualStudio.FallbackLocation.config
      C:\Program Files (x86)\NuGet\Config\Microsoft.VisualStudio.Offline.config

  Feeds used:
      https://api.nuget.org/v3/index.json
      C:\Program Files\dotnet\library-packs
  All projects are up-to-date for restore.
Done Building Project "C:\Code\Temp\MultiTargetted\MultiTargetted.csproj" (Restore target(s)).


Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.79

Discovered this while working on #11653.

Note that when fixing this, we might need to do the same thing as the fix in #11653

[nkolev92]

cc @jeffkl

Did some quick debugging, and we basically end up with a project with nodes, but no outer node as TargetFramework is always set globally, so technically we can't discover an outer build project.

[jeffkl]

Possible duplicate of #11680. Basically, static graph doesn't give NuGet the full representation of the project graph when you specify a TargetFramework because it doesn't think it needs to enumerate the values of TargetFrameworks. I think we'll need to update MSBuild static graph to have a mode where it always loads the full graph even if it could get a scoped version.

[jeffkl]

@nkolev92 can you try this and tell me what happens?

msbuild /t:restore /Property:TargetFramework=net472 /RestoreProperty:Foo=bar

By specifying a /RestoreProperty, MSBuild does not pass the /Property global property to restore, only Foo=Bar.

[nkolev92]

That does workaround it.

[jeffkl]

Maybe for implicity restore /restore in MSBuild, we can block certain global properties from getting through which we know aren't really applicable. Or on NuGet's side we could ignore these properties that don't apply.

[nkolev92]

Not sure what you mean by ignoring certain properties.

I have a PR that might provide you some context about how I discovered this: NuGet/NuGet.Client#4587

[jeffkl]

@nkolev92 for example:

msbuild /t:Restore /p:TargetFramework=net472

The global property TargetFramework is then set for all project evaluations by static graph restore. But TargetFramework isn't really a global property we want to set since restore won't be correct. So we could just ignore that as a global property and not blindly add it.

Where we get global properties to pass to static graph restore:

https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Build.Tasks/RestoreTaskEx.cs#L256

Where static graph restore parses global properties and could ignore certain ones:

https://github.com/NuGet/NuGet.Client/blob/9d99eb281df777a98dc55858d67ab7653fe3bc5f/src/NuGet.Core/NuGet.Build.Tasks.Console/Program.cs#L145

[nkolev92]

The global property TargetFramework is then set for all project evaluations by static graph restore. But TargetFramework isn't really a global property we want to set since restore won't be correct. So we could just ignore that as a global property and not blindly add it.

I don't think we should do that.
That'd break dotnet command -f xyz

Would appreciate feedback to NuGet/NuGet.Client#4587

[jeffkl]

That'd break dotnet command -f xyz

I'm only talking about restore. Do we ever want to restore for a single target framework? I'm just saying whenever NuGet restores, it never needs to tell MSBuild the TargetFramework property during evaluation of the outer project.

dotnet build -f net472 runs msbuild /restore /p:TargetFramework=net472 and the restore portion would just ignore the TargetFramework global property to do a full restore of the whole project and all of its supported frameworks. Then later on build would only build a single framework.

Maybe we should chat about this so I get a better understanding 😄

[baronfel]

FWIW the SDK explicitly performs a restore without TargetFramework if it detects one provided by a user at the command line (either via -f or -p TargetFramework=...). This actually caused a bug with the -getItem flags that I fixed in 8.0.200 :D