Skip to content

Infer the framework using the TargetPlatformMoniker instead of TPI and TPV, GetReferenceNearestTargetFrameworkTask now handles P2P frameworks with platforms, Add a LockFile.GetTarget utility for aliases#3578

Merged
nkolev92 merged 5 commits intodevfrom
dev-nkolev92-tfmImprovements
Aug 18, 2020
Merged

Conversation

@nkolev92
Copy link
Copy Markdown
Member

@nkolev92 nkolev92 commented Aug 13, 2020 •
edited
Loading

Bug

Fixes: NuGet/Home#9895
Fixes: NuGet/Home#9894
Regression: No

  • Last working version:
  • How are we preventing it in future:

Fix

Details:
This is a loaded PR with many small changes.

Things to look for:

  • NuGetFramework.ParseComponents uses TargetFrameworkMoniker and TargetPlatformMoniker instead of the 5 props.
  • Helper utility to detect the target framework now prefers TargetPlatformMoniker if passed, over TargetPlatformIdentifier/TargetPlatformMinVersion.
  • GetReferenceNearestTargetFrameworkTask can now handle P2Ps of TFMs with Platforms.
  • Added a LockFile.GetTarget utility method that can use an alias instead of a NuGetFramework.

Fixed the tests + added a hack covered in NuGet/Home#9913.

Testing/Validation

Tests Added: Yes
Reason for not adding tests:
Validation:

@nkolev92 nkolev92 marked this pull request as ready for review August 13, 2020 22:26
@nkolev92 nkolev92 changed the title Dev nkolev92 tfm improvements Infer the framework using the TargetPlatformMoniker instead of TPI and TPV, GetReferenceNearestTargetFrameworkTask now handles P2P frameworks with platforms, Add a LockFile.GetTarget utility for aliases Aug 13, 2020
@nkolev92
Copy link
Copy Markdown
Member Author

nkolev92 commented Aug 13, 2020

cc @dsplaisted @sfoslund

nkolev92
nkolev92 commented Aug 13, 2020
View reviewed changes
Comment thread test/NuGet.Core.Tests/NuGet.Build.Tasks.Test/GetReferenceNearestTargetFrameworkTaskTest.cs Outdated
dsplaisted
dsplaisted reviewed Aug 13, 2020
View reviewed changes
Comment thread test/NuGet.Core.Tests/NuGet.Build.Tasks.Test/GetReferenceNearestTargetFrameworkTaskTest.cs Outdated
Comment thread test/NuGet.Core.Tests/NuGet.Build.Tasks.Test/GetReferenceNearestTargetFrameworkTaskTest.cs Outdated
Comment thread test/NuGet.Core.Tests/NuGet.Build.Tasks.Test/GetReferenceNearestTargetFrameworkTaskTest.cs Outdated
zivkan
zivkan reviewed Aug 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to rebase on the latest dev, because I expect changes needed to VS nomination utilities + tests as well.

Comment thread test/NuGet.Core.Tests/NuGet.Build.Tasks.Console.Test/MSBuildStaticGraphRestoreTests.cs Outdated
Comment thread test/NuGet.Core.Tests/NuGet.Commands.Test/MSBuildRestoreUtilityTests.cs Outdated
Comment thread test/NuGet.Core.Tests/NuGet.Commands.Test/MSBuildRestoreUtilityTests.cs Outdated
Comment thread test/NuGet.Core.Tests/NuGet.Commands.Test/MSBuildRestoreUtilityTests.cs Outdated
@nkolev92 nkolev92 force-pushed the dev-nkolev92-tfmImprovements branch from 9b4a5e9 to b6a23cd Compare August 14, 2020 00:35
zkat
zkat reviewed Aug 14, 2020
View reviewed changes
Comment thread src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs Outdated
Comment thread test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreNETCoreTest.cs Outdated
@nkolev92 nkolev92 force-pushed the dev-nkolev92-tfmImprovements branch from b6a23cd to 09aa90a Compare August 14, 2020 01:12
@nkolev92 nkolev92 requested review from zivkan and zkat August 14, 2020 01:15
This was referenced Aug 14, 2020
Merged
Closed
@dsplaisted dsplaisted mentioned this pull request Aug 15, 2020
Merged
@nkolev92 nkolev92 mentioned this pull request Aug 15, 2020
Closed
@nkolev92
Copy link
Copy Markdown
Member Author

nkolev92 commented Aug 15, 2020

Fixed all tests + added a property to help tell MSBuild that the new GetNearest Task supports TPM.

cc @dsplaisted

@nkolev92
Copy link
Copy Markdown
Member Author

nkolev92 commented Aug 17, 2020

@zkat @zivkan

Can you please review?

NuGet/Home#9914 would depend on this.

zivkan
zivkan approved these changes Aug 17, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor nitpicks

Comment thread src/NuGet.Core/NuGet.Build.Tasks/GlobalSuppressions.cs
Comment thread src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets
@nkolev92 nkolev92 merged commit 935b1d2 into dev Aug 18, 2020
@nkolev92 nkolev92 deleted the dev-nkolev92-tfmImprovements branch August 18, 2020 18:30
@dsplaisted dsplaisted mentioned this pull request Aug 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zivkan zivkan zivkan approved these changes

@aortiz-msft aortiz-msft Awaiting requested review from aortiz-msft

@cristinamanum cristinamanum Awaiting requested review from cristinamanum

@dominoFire dominoFire Awaiting requested review from dominoFire

@donnie-msft donnie-msft Awaiting requested review from donnie-msft

@dtivel dtivel Awaiting requested review from dtivel

@heng-liu heng-liu Awaiting requested review from heng-liu

@kartheekp-ms kartheekp-ms Awaiting requested review from kartheekp-ms

@rrelyea rrelyea Awaiting requested review from rrelyea

@zkat zkat Awaiting requested review from zkat

+1 more reviewer

@dsplaisted dsplaisted dsplaisted left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

4 participants

@nkolev92 @zkat @dsplaisted @zivkan