[Bug]: AssetTargetFallback now brings in new dependencies and is causing build failures - Add a means to revert to the 17.1 behavior

[dfederm]

NuGet Product Used

MSBuild.exe

Product Version

Microsoft (R) Build Engine version 17.2.0-preview-22081-04+288ea72fc for .NET Framework

Worked before?

Microsoft (R) Build Engine version 17.1.0+ae57d105c for .NET Framework

Impact

I'm unable to use this version

Repro Steps & Context

As of the latest version, it seems like a missing dependency group breaks restore.

When packing, I do see this warning:

WARNING: NU5128: Some target frameworks declared in the dependencies group of the nuspec and the lib/ref folder do not have exact matches in the other location. Consult the list of actions below:
- Add a dependency group for .NETStandard2.0 to the nuspec

However, previously the behavior of a missing dependency group matched that of an empty dependency group.

Repro materials: Repro.zip

Repro steps:

1. msbuild /restore TestLib
2. nuget pack -OutputDirectory Packages TestPackageBroken.nuspec
3. msbuild /t:restore TestProjectBroken

A warning is emitted:

C:\Code\tmp\RestoreRepro\TestProjectBroken\TestProjectBroken.csproj : warning NU1701: Package 'Microsoft.AspNet.WebApi.Core 5.2.7' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramewo
rk,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' instead of the project target framework '.NETStandard,Version=v2.0'. This package may not be fully compatible with your
 project.

It works if you do this however (the package has an empty dependency group):

1. msbuild /restore TestLib
2. nuget pack -OutputDirectory Packages TestPackageWorks.nuspec
3. msbuild /t:restore TestProjectWorks

Verbose Logs

No response

[nkolev92]

Hey @dfederm,

I think this is an intended consequence of the fix for #5957, detailed in https://github.com/NuGet/Home/blob/dev/proposed/2022/AssetTargetFallback-DependenciesResolution.md.

tldr; When NuGet doesn't find a dependency group it uses AssetTargetFallback, which is basically the design of AssetTargetFallback itself.

[nkolev92]

To avoid this behavior, you can disable asset target fallback for your project.

[dfederm]

I know this is by design, but this seems to be causing breakages in every single repo I've tested so far. Currently I'm just avoiding using 17.2 since I just can't use it because of this issue.

Although at this point the issues I'm seeing are a little unrelated to the original issue, but related to AssetTargetFallback in general. This includes fairly standard packages, not just the weirdly formed one I originally referred to.

Example:

error NU1701: Package 'Microsoft.Net.Http 2.2.22' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' instead of the project target framework 'net5.0'. This package may not be fully compatible with your project.

[dfederm]

Here's a minimal repro:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Microsoft.Azure.Common.Dependencies" Version="1.0.0" />
  </ItemGroup>
</Project>

When restoring:

C:\Code\tmp\NugetRepro\NugetRepro.csproj : warning NU1701: Package 'Microsoft.Bcl 1.1.9' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
C:\Code\tmp\NugetRepro\NugetRepro.csproj : warning NU1701: Package 'Microsoft.Bcl.Async 1.0.168' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
C:\Code\tmp\NugetRepro\NugetRepro.csproj : warning NU1701: Package 'Microsoft.Net.Http 2.2.22' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
C:\Code\tmp\NugetRepro\NugetRepro.csproj : warning NU1701: Package 'Newtonsoft.Json 6.0.4' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.

[zivkan]

@dfederm with that repro, if the console app actually used an API from the Microsoft.Azure.Common.Dependencies and you restore with VS 17.1 or earlier, won't the app fail at runtime because none of the package's dependencies are restored and therefore none of their assemblies are int he console app's bin folder? FileNotFound exceptions, trying to load the assembly.

I feel like your repro is actually evidence that the 17.2 fix helps projects be successful at runtime.

[dfederm]

@zivkan Nope, everything works fine. In this specific case the indirect dependencies (Microsoft.Bcl, etc) are all effectively polyfills for older frameworks which are not needed with newer frameworks.

[dfederm]

This is explained in the Microsoft.Bcl readme btw: https://www.nuget.org/packages/Microsoft.Bcl/

[zivkan]

In the case of Microsoft.Bcl, looking at the assets file after restore, the only asset it has selected is lib/net45/_._, which is NuGet convention for none. Asking for a design change request (DCR) to stop NU1701 warnings when the only assets selected are _._ seems reasonable.

However, in the case of Microsoft.Bcl.Async, the assets selected are lib/net40/Microsoft.Threading.Tasks.Extensions.Desktop.dll, lib/net40/Microsoft.Threading.Tasks.Extensions.dll, and lib/net40/Microsoft.Threading.Tasks.dll. In other words, this package is not correctly authored to act as a polyfill. In fact, the way the package is authored, NuGet will instruct the .NET SDK to copy the assemblies to the bin folder, and the .NET SDK itself has logic not to. But the way the package is authored is instructing NuGet and the build system to copy the files to the bin directory.

Also note that the latest version of the package was published in 2014, about 2 years before .NET Core 1.0 was released, and AssetTargetFallback was released later than that.

However, previously the behavior of a missing dependency group matched that of an empty dependency group.

The previous behaviour was that a missing dependency group matched the next best dependency group. It's only when none of the dependency groups matched that no dependencies were used.

Consider for example a package with lib/net40/my.dll and lib/net48/my.dll. The package only contains one dependency group for .NETFramework4.0 to add Microsoft.BCL.Async. A project targeting .NET Framework 4.8 that installs this package will get the lib/net48/ assets, and since there's a missing dependency group for .NETFramework4.8, it will select the next best, .NETFramework4.0. Therefore, the project will get a transitive dependency on Microsoft.Bcl.Async because the package is not authored in the highest quality way.

Another example is a package that has lib/netstandard2.0/my.dll and lib/net5.0/my.dll, but only a .NETStandard2.0 dependency group. Similarly, net5 and net6 projects will get the dependencies, even though the files used from the package are not the .NET Standard files.

NU5128 was added 2 years ago (not in VS 17.2) to try to encourage package authors to improve their package quality, and reduce the risk of these types of experiences for package consumers.

The problem that #5957 fixes is for scenarios along the lines of a net6.0 project referencing a package that only supports net48, and it works via AssetTargetFallback, but fails at runtime because the package's dependencies are missing.

As the design spec for respecting AssetTargetFallback for package dependencies states: https://github.com/NuGet/Home/blob/dev/proposed/2022/AssetTargetFallback-DependenciesResolution.md#data-analysis---are-packages-authored-consistently

3.1% (5) of all (really only 85% can call into this) the packages have inconsistencies in the target frameworks defined in the compile and dependency groups. These packages can be considered not well authored and are specifically affected by the direction we choose. Pack warning NU5128 informs package authors that their package has this inconsistency, and the percentage of packages with this inconsistency should decrease as authors adopt the warning's suggestions.

So, feedback that you're experiencing this issue is valuable, if we get a lot of feedback that customers are having build failures, we can take action. But currently I agree with Nikolche that it's by design and is a symptom of packages that are not well authored. If package authors fixed warnings, NU5128 specifically, this type of package consumer issue would not happen.

Perhaps it was a misunderstanding that missing dependency group != empty dependency group. As I previously explained, NuGet has always selected the "best" dependency group, even when there isn't an exact match based on file assets selected. When the packages used conform to NU5128, package consumers benefit from not having runtime errors now that dependency resolution uses AssetTargetFallback.

[zivkan]

We got another related feedback internally.

The project is using MvvmLight in a .NET Core/5+ project, and therefore gets assets via AssetTargetFallback. The package has a dependency on MvvmLightLibs, but since none of the dependency groups are "directly" compatible with .NETCoreApp, prior to #5957 the project did not get any dependencies and would fail at runtime.

The MvvmLight package author created MvvmLightLibsStd10 which appear to be .NET Standard compiled versions of the MvvmLightLibs package. Personally, I would argue that this is an authoring mistake, and that MvvmLightLibs should have been updated to contain both netfx and netstandard assemblies in the same package. Customers using MvvmLight would still have needed to add a PackageReference to MvvmLightLib before #5957 was fixed. If they had, then after #5957 was fixed, nobody would have noticed as it would look to nuget like a "nearest-wins rule" mitigation.

A second package authoring error is that MvvmLightLibsStd10 uses a different strong name key to MvvmLightLibs. I'm not 100% sure, but I think that if the same key had beed used, then MSBuild would have deduplicated the assemblies. However, since they're different keys, this means that under .NET Framework assembly identity rules, they are different assemblies, and using these .NET Standard 1.0 assemblies would have caused assembly loading errors. Of course, there would be no point to use MvvmLightLibsStd10 on .NET Framework, since the MvvmLightLibs package is brought in as a dependency. However, the .NET Core runtime no longer validates strong name keys and identities, hence why this strong name key mismatch was not previously discovered.

Now, MvmLight is archived on GitHub: https://github.com/lbugnion/mvvmlight

Therefore, it's impossible for us to report these package authoring mistakes to the author and get them fixed in a newer version.

Assuming that MvvmLightLibsStd10 is just a recompile of the same code that MvvmLightLibs is, then package consumers can either remove their reference to MvvmLightLibsStd10, or change it to MvvmLightLibs. Both mitigations should fix the build error, but removing the reference requires NuGet 6.2, which CI agents won't have yet.

[nkolev92]

Triage:
As @zivkan explained, the current behavior is by design.
We will keep monitoring the feedback, and work on some docs/blog to increase awareness of the behavior.

[dfederm]

So far this has broken basically every repo I've worked in since it went in. Maybe I'm just unlucky or working in "weird" repos, so I'll see if I can do a broad test pass with 17.2 against internal repos to see how impactful this breaking change is.

[dfederm]

I still haven't run a broad test, but I just want to note that anecdotally I'm seeing this cause failures in a whole bunch of different MSFT-internal repos I've been working on in various orgs. I'm finding myself basically unable to use my VS 17.2 instance for much of my work and having to drop down to my 17.1 instance to work around this change in behavior.

[nkolev92]

@dfederm

What happens if you suppress NU1701 in those projects?

Are you facing any runtime issues with the current version?

[dfederm]

I'm not sure about runtime issues. Personally, I'm not running anything as I'm mostly concerned about build. I assume though if I were to suppress these warnings, a bunch of additional outputs would now be included.

[nkolev92]

If there are packages that are imperfectly authored, and bring in dependencies that are actually not needed, those new dependencies can be made top level and set with excludeassets=all to prevent them from appearing in the output. If these new assemblies are not relevant, them being in the output should normally not cause any problems.

The packages in question should also have their authoring fixed long term though.

[dfederm]

I agree that these are package authoring issue and that there are mitigations a repo owner can employ.

I think the primary problem is that upgrading from 17.1 to 17.2 will suddenly produce a lot of errors for a lot of people.

People will either:
a) upgrade, hit errors, work around or downgrade, be mad.
b) be told by others who hit (a) to not upgrade and stick on 17.1 for the foreseeable future.

[dfederm]

@nkolev92 I finally got around to doing a board smoke test of 17.2.

I ran a total of 733 test builds on internal MSFT repos and 55 (~7.5%) failed with NU* errors and do not fail with 17.1:

• 2x NU1107
• 2x NU1202
• 2x NU1603
• 8x NU1605
• 11x NU1608
• 30x NU1701

I do believe most of these are ultimately due to the issue mentioned (I didn't dig into each individually).

Based on this I do believe this change will make adoption of 17.2 quite rocky and is likely to cause a non-trivial amount of pain, at least internally.

[nkolev92]

Thanks for those details @dfederm!

The good part is that 4 of those failures are errors (harder to resolve) and the rest are warnings elevated to errors.

Unfortunately I don't think we have any other approaches beyond delaying (and making it more complicated for the people that need this change).

[breidikl]

Note that this causes failures when using lock files, since the transitive dependencies are not listed ("dotnet restore --use-lock-file" fails before attempting to update). You need to use --force-evaluate in order for the lock file to be correctly updated.

#5740 also becomes more problematic after this change.

[Dan-Albrecht]

Did this just get merged into the last dotnet SDK release? I have a solution that restores perfectly fine with SDK 6.0.203. After bumping my global.json to 6.0.300 I now see a bunch of NU1701 warnings about transitive packages (that are probably misauthored).

[dfederm]

@Dan-Albrecht yes 6.0.300 corresponds with VS 17.2

[fowl2]

Could this please at least be added to the release notes / known issues?

(Or even better, a specific error message added)

[nkolev92]

https://docs.microsoft.com/en-us/nuget/release-notes/nuget-6.2#summary-whats-new-in-62

Project A referencing package B via AssetTargetFallback, doesn't use that same AssetTargetFallback to pull B's dependency package C - #5957 - More information

We'll add some more documentation.

[nkolev92]

Hey all,

In 6.0.400/17.3/6.3 (SDK, VS, NuGet.exe) we'll temporarily add an env var that you can use to go back to the legacy behavior: NUGET_USE_LEGACY_ASSET_TARGET_FALLBACK_DEPENDENCY_RESOLUTION.

In 7.0.100, 17.4, 6.4 (SDK, VS, NuGet.exe), we'll remove this option and make only the fixed behavior available.

The default behavior in 6.3 will match 6.2