[release/5.0.2xx] Update dependencies from nuget/nuget.client

[dotnet-maestro]

This pull request updates the following dependencies

From https://github.com/NuGet/NuGet.Client

• Subscription: 7de3a189-a51a-4c60-bc8a-08d87076189d
• Build: 5.9.0.6955
• Date Produced: 11/4/2020 7:56 PM
• Commit: fa9fbcb87b8ce717d7f78d494cd16cf174be48e9
• Branch: dev

• Updates:
  • NuGet.Build.Tasks: from 5.8.0-rc.6930 to 5.9.0-preview.1.6955

[sfoslund]

@mmitche I'm assuming we shouldn't be getting these duplicated comments here?

[mmitche]

Nope. @MattGal Any idea why the comments are getting duplicated?

[MattGal]

I definitely caused this, will address via https://github.com/dotnet/core-eng/issues/11362

[sfoslund]

It looks like tests are failing because a new nuget error is being thrown before we get to the existing SDK error, which I assume is expected.

error NU1012: Platform version is not present for one or more target frameworks, even though they have specified a platform: net5.0-unsupported

[zkat]

Hm. Looks like the SDK is running into a new NuGet error code before it actually gets to test the thing it's trying to test. I'm trying to figure out whether I think this is something that should just be updated in the tests themselves, or whether it's NuGet being too aggressive with this error. Maybe @zivkan or @nkolev92 have thoughts on this? I think if it's the former, the SDK will need to probably just change the -unsupported test data to just -unsupported1.0 or something?

[nkolev92]

When is the SDK error raised?
At build time?

[zkat]

@nkolev92 this error gets raised at both restore AND pack time, in various scenarios. I'm pretty sure these specific test failures are hitting the restore scenario, though. See https://github.com/dotnet/sdk/blob/master/src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs#L88-L106 for example.

[nkolev92]

@zkat That's the NuGet error, I was asking where the SDK was raising it's own error.

If it's at evaluation time, then something weird is happening.
if it's at build time, then I think we can safely recommend changing the expectation of when the error is raised.

[zkat]

@sfoslund do you have any thoughts on whether this is something that should just get fixed on the SDK side?

[sfoslund]

I think the existing SDK error is more clear for the user, so I would prefer to keep that one. @wli3 and @dsplaisted do you have an opinion?

[zivkan]

I find msbuild's execution order very difficult to remember, but from memory when a target "a" runs, the order is:

1. targets listed in a's DependsOn
2. targets listing "a" as "BeforeTargets"
3. a's "body"
4. targets listing "a" as "AfterTargets"

Unfortunately, NuGet doesn't provide DependsOn extensibility in our restore task. Compare to our Pack target, which allows the SDK and customers to add their own targets to our DependsOn list.

Therefore, would it work if the SDK changed where it generates its error to a target that uses BeforeTargets="Restore"? Would an error in the target prevent NuGet's RestoreTask task from running? Or maybe NuGet should add RestoreDependsOn extensibility, and the SDK should use it?

I think this has to be the way we fix it. If NuGet tries to restore without a platform version, we will assume version 0.0, let's say net5.0-windows0.0. If the project has a package reference to a package with net5.0-windows7.0 assets, restore will fail because the windows 7.0 assets will not be compatible with windows 0.0, since it's a lower version, and the customer will get an error message saying the package is not compatible with the project. Similarly, a project targeting net5.0-windows0.0 with a project reference to a project targeting net5.0-windows7.0 will also cause NuGet problems.

Therefore, NuGet removing this restore time error will give customers a more confusing error message when the scenario is hit. It's only when the project doesn't have any package or project references to something that correctly uses platforms, that restore will succeed, allowing the SDK to report its current error message.

[nkolev92]

Therefore, would it work if the SDK changed where it generates its error to a target that uses BeforeTargets="Restore"? Would an error in the target prevent NuGet's RestoreTask task from running? Or maybe NuGet should add RestoreDependsOn extensibility, and the SDK should use it?

None of this works in VS :)

If the SDK is the one raising an error then it'd need to happen at evaluation time.
Alternatively, we can improve the NuGet side error.

[dsplaisted]

What about the situation in NuGet/Home#10195, where you have TargetFrameworks set to net5.0-android;net5.0-ios, but you don't have either of those workloads installed?

What we want is for you to get an error message telling you that you need to install the iOS and Android workloads. We don't want the NuGet restore failure to that the version numbers are 0.0 to pre-empt the message that tells you to install the workloads.

Maybe it would be sufficient to run the _CheckForUnsupportedTargetPlatformIdentifier target before the Restore target. That wouldn't work for Visual Studio, but VS is supposed to use a different way to detect missing workloads anyway: https://github.com/dotnet/designs/blob/main/accepted/2020/workloads/workload-resolvers.md#vs-behavior