NuGetFallbackFolder not used for multitargeting projects

[natemcmaster]

The SDK adds the NuGetFallbackFolder for restore operations on .NET Core and .NET Standard projects so that the BundledNETCoreAppPackageVersion version of Microsoft.NETCore.App is always available and aligns with the CLI installer. However, the SDK fails to add the fallback folder in multitargeting projects.

sdk/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.BeforeCommon.targets

Lines 91 to 95 in cd25d6b

	<PropertyGroup Condition=" '$(DisableImplicitNuGetFallbackFolder)' != 'true' and '$(_IsNETCoreOrNETStandard)' == 'true' and Exists($(_NugetFallbackFolder)) ">
	<RestoreAdditionalProjectSources Condition=" '$(_TargetFrameworkVersionWithoutV)' &lt; '2.0' ">$(RestoreAdditionalProjectSources);$(_NugetFallbackFolder)</RestoreAdditionalProjectSources>
	<RestoreAdditionalProjectFallbackFolders Condition=" '$(_TargetFrameworkVersionWithoutV)' >= '2.0' ">$(RestoreAdditionalProjectFallbackFolders);$(_NugetFallbackFolder)</RestoreAdditionalProjectFallbackFolders>
	</PropertyGroup>

Repro

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

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>netcoreapp2.0;net461</TargetFrameworks>
     <!-- a single TFM, just netcoreapp2.0, works fine, but adding two tfms breaks it -->
  </PropertyGroup>

</Project>

<!-- NuGet.config -->
<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <clear />
    <add key="NuGet" value="https://api.nuget.org/v3/index.json" />
  </packageSources>
</configuration>

dotnet nuget locals all --clear
dotnet restore --verbose

Expected
Fallback folder is used, just as it would be for single-tfm projects

Actual

C:\tmp\tmp.csproj : error NU1102: Unable to find package Microsoft.NETCore.App with version (>= 2.0.0-preview3-25502-01)\r
C:\tmp\tmp.csproj : error NU1102:   - Found 13 version(s) in nuget.org [ Nearest version: 2.0.0-preview2-25407-01 ]

Details
dotnet --version: 2.0.0-preview3-006670

For comparison, look at the restore dependency graph generated with multi tfms vs a single tfm.

dotnet msbuild /t:GenerateRestoreGraphFile /p:RestoreGraphOutputPath=./dg.multitfm.json
dotnet msbuild /t:GenerateRestoreGraphFile /p:RestoreGraphOutputPath=./dg.singletfm.json /p:TargetFramework=netcoreapp2.0

dg.singletfm.json will contain this, but dg.multitfm.json won't.

        "fallbackFolders": [
          "C:\\Users\\namc\\.dotnet\\x64\\sdk\\NuGetFallbackFolder"
        ],

cc @emgarten

[livarcocc]

@emgarten I don't think NuGet participates in the inner-loop right? I will have to think about how to enable this scenario if that's the case. Because if the app targets, say, netcoreapp1.1 and netcoreapp2.0, we need one part to be from the fallback folder and the other from the package source, which I don't think is possible today. We can enable the fallback folder if any 2.0 TFM is present, but that may break 1.x apps, if you are multi-tfming with it. Though, to be fair, I am not sure how common that is. I guess for tools that might make sense.

[emgarten]

Sources and Fallback Folders are evaluated in the outer build since these are applied to the entire project and cannot be on a per framework basis.

Is it possible for the SDK to set these in the outer build?

From the NuGet side this could be fixed by reading these properties during the inner build, and then taking the super set of all RestoreAdditionalProjectSources values and the intersection of all RestoreAdditionalProjectFallbackFolders values to ensure that if any framework exists that does not support fallback folders, that fallback folders are not used at all. This might be fitting these properties too closely to this exact scenario, but they were added for this scenario.

[livarcocc]

After thinking some more about it, I am inclined to go with your suggestion above. I mean, we would pretty much have to do a similar evaluation ourselves in the outer loop to find all the TFMs and then set a property if any 1.x TFM exists, or even do what you are suggestion ourselves in the outerloop. Since you are already doing the inner loop evaluations, I think this might be the easier fix.

[livarcocc]

We should be able to do this in the SDK as well, if you think that makes more sense, though.