Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a project framework inference/parsing utility #3562

Merged
merged 3 commits into from
Aug 6, 2020

Conversation

nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Aug 4, 2020

Bug

Fixes: NuGet/Home#9871
Regression: No

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

Fix

Details: In 280abd3, NuGet started reading the platform information out of the internal properties. There is some inherent priority in those properties and they are not always perfect. For example, UWP projects will have a TargetFrameworkIdentifier of .NETCoreApp. NuGet has known how to interpret these for a while. Now we just need to incorporate, TargetPlatformIdentifer and TargetPlatformVersion into the mix.

To do that, we'll introduce a new utility, GetProjectFramework, that takes all these values and spits out a NuGetFramework.
Internally the existing utility has been refactored so that the same codepaths are hit. The public API hit is minimal ;)

Caveats

There's a larger impact of this PR.

In new .NET 5 SDKs, think NET5.0 P8, from the 5 properties NuGet reads, the ones that are not necessary, are cleared, however in .NET 5 P7 and earlier (that includes the complete 3.0, 3.1 SDK line), the TargetPlatformInformation & TargetPlatformVersion may default to something like Windows and v7.0 and we'd end up with a framework such as net5.0-windows7.0 in .NET 5 cases (think NuGet.Client), or an exception (in netcoreapp3.1 cases) saying that TargetPlatformIdentifier is only allowed. Note that the last case can easily happen in commandline scenarios where the customer ends up with an upgrade nuget, but not sdk.
While this scenario is not recommended, I imagine it'll be common enough where we have to handle it.

After a discussion with @zivkan and @zkat we agreed to change ParseComponents to fit the requirements that we have.
Specifically that it having a precedence order of the params.

Other options considered:

  • Add a utility in NuGet.Commands. This will require some code duplication. Essentially copying b0130c8 into a different assembly.

Note that given that ParseComponents hasn't shipped, we can easily revert the API.

Testing/Validation

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

@zivkan
Copy link
Member

zivkan commented Aug 4, 2020

UWP projects will have a TargetFrameworkIdentifier of .NETCoreApp

UWP projects actually use TargetFrameworkIdentifier == .NETCore, not .NETCoreApp (at least in my tests). So, it's already distinguishable from actual .NETCoreApp TFMs, it's just confusing.

@nkolev92 nkolev92 force-pushed the dev-nkolev92-frameworkUtility branch from 02e2e8d to c453e22 Compare August 5, 2020 18:07
nkolev92 and others added 3 commits August 6, 2020 11:32
Co-authored-by: Andy Zivkovic <zivkan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a utility to interpret the effective target framework from a list of projects and file path extensions.
3 participants