Remove unnecessary NuGet package references.

[teo-tsirpanis]

Bug

• Fixes: Remove .NET Standard 1.x packages from NuGet SDK packages Home#9723
• Regression: No
• Last working version: None
• How are we preventing it in future: Review the necessity of the package dependencies more carefuly. Do not reference System.* packages that do not target .NET Standard 2.0 and/or their version number is stuck at 4.3.0.

Fix

Details: This PR removes packages that are not meant for .NET Standard 2.0, significantly trimming the dependency tree, in a similar way to dotnet/msbuild#5242.

Testing/Validation

Tests Added: No
Reason for not adding tests: No functionality changed
Validation: The CI builds still succeed

[nkolev92]

Hey @teo-tsirpanis

Thanks for this change.
At this point we're in a stabilization phase, so it'll take us a few days before we are ready to take this PR.

In the meantime would you please rebase as there some added conflicts, because we added support for .NET 5.

Finally, can you please create a tracking issue for this and follow the PR template?

Thanks!

[teo-tsirpanis]

Hello @nkolev92, I created an issue, rebased the branch and edited the PR's description.

[zivkan]

@teo-tsirpanis I ran a build, but it's failing restore with package downgrade wanrings that are elevated to errors:

##[error]test\TestExtensions\TestablePlugin\TestablePlugin.csproj(0,0): Error NU1605: Detected package downgrade: System.Collections from 4.3.0 to 4.0.11. Reference the package directly from the project to select a different version. 
##[error]test\TestExtensions\TestablePlugin\TestablePlugin.csproj(0,0): Error NU1605: Detected package downgrade: System.IO.FileSystem.Primitives from 4.3.0 to 4.0.1. Reference the package directly from the project to select a different version. 
##[error]test\TestExtensions\TestablePlugin\TestablePlugin.csproj(0,0): Error NU1605: Detected package downgrade: System.Runtime.Extensions from 4.3.0 to 4.1.0. Reference the package directly from the project to select a different version. 
##[error]test\TestExtensions\TestablePlugin\TestablePlugin.csproj(0,0): Error NU1605: Detected package downgrade: System.Text.Encoding.Extensions from 4.3.0 to 4.0.11. Reference the package directly from the project to select a different version. 

[teo-tsirpanis]

@zivkan, the versioning conflicts were caused by an old version of Newtonsoft.Json which does not directly target .NET Standard 2.0. I updated it and restoring works.

[nkolev92]

@teo-tsirpanis

Unfortunately we can't update the newtonsoft.json version. There are some upstream dependencies of NuGet that are tied to a specific version.

See #3347 and #3383.

If all the warnings are happening in a test utility, I think it's ok to bump the versions for those projects only.

[teo-tsirpanis]

OK @nkolev92, I reverted to Newtonsoft.Json 9.0.1.

[zivkan]

@teo-tsirpanis sorry about the delays. We've been in a mad rush try to meet a deadline.

The builds are now failing with this error:

##[error]test\TestExtensions\GenerateLicenseList\GenerateLicenseList.csproj(0,0): Error NU1604: Project dependency System.Runtime.Loader does not contain an inclusive lower bound. Include a lower bound in the dependency version to ensure consistent restore results.

[teo-tsirpanis]

OK @zivkan, I removed the dependency (it was unnecessary for netcoreapp).

[teo-tsirpanis]

All tests passed except of 16. At least one of them (ReadStringArrayAsList_WithSupportedTypes_ReturnsStringArray) failed because in the Greek culture the decimal separator is the comma instead of the dot. Other tests failed because the expected error messages are in Greek and not English. But I hope it will be fine on the CI.

[zivkan]

Sorry, I forgot that the .NET 5 SDK had a bug in preview 5 and 6: 070c016 dotnet/sdk#11862

If you make sure your machine has the .NET 5 preview 4 SDK, or one of the nightly preview 7 or 8 SDKs, I think it should be ok. Your branch didn't change any code, only packages, so as long as the code compiles, we should be good. Although I'm interested in which tests failed because of the different number formats, because it sounds like we have a bug, even if it's just in the tests.

If you can revert your change adding the AssemblyMetadataAttribute class, I can run another CI build. It might be worthwhile to rebase on the latest dev branch because I think we might have skipped some flaky tests in the last 25 days.

thanks.

[teo-tsirpanis]

Done. By the way, configure.ps1 had downloaded a nightly SDK (preview 7), but anyway.

[zivkan]

I'm not sure what happened but github is now showing 31 commits and 133 files changed.

[teo-tsirpanis]

I rebased the branch... 😳

[zivkan]

It looks like you merged NuGet's dev branch, rather than rebasing. Since your work was in your fork's dev branch, when you merged NuGet's dev branch, NuGet's dev commits got added after your commits in the commit history, which means all their commits hashes got changed, hence why GitHub says they're changed. I think you should be able to fix it with an interactive rebase. Otherwise feel free to close this PR and open a new one (and use a different branch name, just in case).

[teo-tsirpanis]

OK, I cherry-picked each commit into the dev branch. Now it says I want to merge only 5 commits.

[nkolev92]

This looks great, thanks @teo-tsirpanis!

There's one small change I'd like for you to make before I approve this PR.

[nkolev92]

I'll surprised this did not fail the build.

The versions should remain managed in packages.targets.

There's a section with packages used in test.

[zivkan]

I'm also surprised it didn't fail the build, but it didn't. This suggests that removing the versions might leave these PackageReferences without a version, in which cause the build might fail. So, my thought is we merge as-is, and we should fix our msbuild files as an engineering improvement, and not burden the contributor for this.

[nkolev92]

I buy that.
Can we create a follow-up issue first?

[teo-tsirpanis]

Too late, I used the SystemPackagesVersion property.

If I recall correctly, these references stayed because they resolve package version conflicts.

[zivkan]

NuGet/Home#9790

[teo-tsirpanis]

I tried it again to build without these references and got this (adjusted for brevity):

error NU1605: Detected package downgrade: System.Collections from 4.3.0 to 4.0.11. Reference the package directly fr
om the project to select a different version.
error NU1605:  Plugin.Testable -> NuGet.Protocol -> NuGet.Packaging -> Newtonsoft.Json 9.0.1 -> System.Xml.ReaderWri
ter 4.0.11 -> System.IO.FileSystem 4.0.1 -> runtime.win.System.IO.FileSystem 4.3.0 -> System.Collections (>= 4.3.0)
error NU1605:  Plugin.Testable -> NuGet.Protocol -> NuGet.Packaging -> Newtonsoft.Json 9.0.1 -> System.Collections (
>= 4.0.11)

Same with the other packages. The real fix was to upgrade Newtonsoft.Json but unfortunately it can't be done at the moment.

[teo-tsirpanis]

@zivkan, it didn't fail the build because the SystemPackagesVersion property is equal to 4.3.0.

[zivkan]

As per the issue I linked, our MSBuild targets are supposed to validate that no PackageReference has a Version attribute set before it then applies all the versions. This isn't working, but is out of scope for your PR. Assuming the current build in progress is green, I'll merge this PR and the NuGet team will be responsible for fixing up our MSBuild files (unless someone in the community wants to do it and send a PR 😊 ).

[nkolev92]

We have a follow up :)

[teo-tsirpanis]

Hmm, why is the CI failing? 🤨

[zivkan]

One unit test on Windows failed, it looks like a flaky test. A directory was not empty when it was trying to be deleted (I'm guessing cleanup after a test ran). About 5% of Linux build hang, and this build was one of those. I've started a new build.