Sdk: Transitive FrameworkReference interfere with VS restore causing project system to nominate again and cause extra restores

Created on 20 Nov 2020 · 6Comments · Source: dotnet/sdk

@nkolev92 commented on Thu Nov 19 2020

Visual Studio Version: 16.9 P1 (repros with 16.8, I'd expect it repros all the way down to 16.3)

Summary:

Take a project that has a package that has a framework reference not set in the project.
That project starts with Microsoft.NETCore.App reference.
After the first restore happens in VS, the project transitively inherits Microsoft.AspNetCore.App.
Project-system this. This shouldn't happen, only direct FrameworkReference should be nominated.

Steps to Reproduce:

Checkout https://github.com/nkolev92/ProjectWithTransitiveFrameworkReference.
Enable project system logging with $env:CPS_DiagnosticRuntime=1
Do not restore/build. Load the solution in Visual Studio.
Observe that restore happens twice. Observe that nomination has happened twice.
If you inspect the nomination data you will see that 1st nomination is something like:

BEGIN Nominate Restore for C:\Users\Roki2\Documents\Code\ProjectWithTransitiveFrameworkReference\ConsoleApp\ConsoleApp.csproj
    MSBuildProjectExtensionsPath:     C:\Users\Roki2\Documents\Code\ProjectWithTransitiveFrameworkReference\ConsoleApp\obj\
    OriginalTargetFrameworks:         
    Target Frameworks (1)
        .NETCoreApp,Version=v5.0
            Framework References
                Microsoft.NETCore.App -- (PrivateAssets:All)
            Package Downloads
            Project References
            Package References

but the 2nd one has:

BEGIN Nominate Restore for C:\Users\Roki2\Documents\Code\ProjectWithTransitiveFrameworkReference\ConsoleApp\ConsoleApp.csproj
    MSBuildProjectExtensionsPath:     C:\Users\Roki2\Documents\Code\ProjectWithTransitiveFrameworkReference\ConsoleApp\obj\
    OriginalTargetFrameworks:         
    Target Frameworks (1)
        .NETCoreApp,Version=v5.0
            Framework References
                Microsoft.AspNetCore.App -- (PrivateAssets:)
                Microsoft.NETCore.App -- (PrivateAssets:All)
            Package Downloads
            Project References
            Package References

This happens because the project has a package that transitively pulls in the AspNet FrameworkReference.

Expected Behavior:

Only one nomination, thus one restore has happened.
The restore should not impact the data being sent to NuGet.

Note that commandline restores similar to the first restore.

Actual Behavior:

After the initial restore, a 2nd nomination comes in with additional FrameworkReference data. This causes an extra restore.

User Impact:

Performance, delays solution load.
Note that this also makes commandline restore outputs dirty, so restore would be more expensive.

cc @panopticoncentral @davkean @lifengl

Ran into while trying to test bulk file tests.
I had also been observing some weird restore counts in NuGet's RPS tests, and I think this partially explains those.

@davkean commented on Thu Nov 19 2020

We call a target provided by NuGet; CollectFrameworkReferences. This is outside our control.

@nkolev92 commented on Fri Nov 20 2020

NuGet uses that target on the commandline too. It always returns only 1 value, not 2.
Regardless whether the assets file exists or not. So there's something different about VS that I don't understand.

CollectFrameworkReferences

cc @dsplaisted maybe he has some ideas.

@davkean commented on Fri Nov 20 2020

The thing that turns framework references in packages/assets file into real framework references will be running alongside the CollectFrameworkReferences in VS. they aren't isolated.

@nkolev92 commented on Fri Nov 20 2020

they aren't isolated

That's what I feared.
We need to differentiate between them somehow.

Given that packages are rarely shipped through a VS restore/build/pack, I'm not too worried about potential bad packages getting created, but the performance impact of double restores is there.

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

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="5.0.0" />
  </ItemGroup>

  <Target Name="CheckRefs" BeforeTargets="Build">
    <CallTarget Targets="CollectFrameworkReferences" />
  </Target>
</Project>

Given that this runs the target at build time, if you generate a binlog, 2 framework references are shown.

The framework ref is being added in https://github.com/dotnet/sdk/blob/44f381b62d466565639d51847c9127afbe7062a9/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets#L154-L161.

One idea is:

AddTransitiveFrameworkReferences should add some metadata to the FrameworkReference.
CollectFrameworkReferences will ensure it skips items with that metadata.

Any other ideas?

I'll move this to SDK side.

Bug

Source

nkolev92

All 6 comments

@nkolev92 Your suggestion to distinguish these with metadata sounds good. However, since the CollectFrameworkReferences target is in NuGet.targets, we'd need both an SDK and a NuGet change.

We could update the AddTransitiveFrameworkReferences target to do this:

   <ItemGroup> 
     <FrameworkReference Include="@(TransitiveFrameworkReference)" Exclude="@(FrameworkReference)"
                         IsTransitiveFrameworkReference="true" /> 
   </ItemGroup>

Then the NuGet target would look something like this:

<Target Name="CollectFrameworkReferences" Returns="@(_FrameworkReferenceForRestore)">
    <ItemGroup>
        <_FrameworkReferenceForRestore Include="@(FrameworkReference)"
                                       Condition="'%(FrameworkReference.IsTransitiveFrameworkReference)' != 'true'"/>
    </ItemGroup>
</Target>

How does that sound?