warning when dot is omitted from targetframework version in net5.0+ #3625

zkat · 2020-09-01T20:58:36Z

zivkan

Can we use a better PR title? Whoever is hotseat the week this gets inserted into VS will have an easier time figuring out the "area" with a less cryptic PR title.

zivkan · 2020-09-01T22:11:02Z

src/NuGet.Core/NuGet.Build.Tasks.Pack/Strings.resx

@@ -169,6 +169,10 @@
  <data name="IsPackableFalseError" xml:space="preserve">
    <value>This project cannot be packaged because packaging has been disabled. Add &lt;IsPackable&gt;true&lt;/IsPackable&gt; to the project file to enable producing a package from this project.</value>
  </data>
+  <data name="MissingRequiredDot" xml:space="preserve">
+    <value>Dots in TargetFramework versions are required. The missing dot in '{0}' might cause future breakage.</value>


@JonDouglas do you have comments about this user-facing error message?

@zkat & @zivkan

Dots in TargetFramework versions are required. The missing dot in '{0}' might cause future breakage.

->

The TargetFrameworkVersion value `{0}` was not recognized. It may be missing a dotted version number. The TargetFrameworkVersion value `{0}` was not recognized. It may be missing a version number separated by periods.

or

The TargetFrameworkVersion value `{0}` is not valid. Ensure your TargetFrameworkVersion includes a dotted version number. The TargetFrameworkVersion value `{0}` is not valid. Ensure your TargetFrameworkVersion includes a version number separated by periods.

The framework is technically valid and understood...just might not work the way that customer intended it to.

In particular the problem is that net50 is parses to version 5.0, and the eventual progression of NET will eventually lead to version 10.0, so if people still write frameworks with versions then net10, it'd get parsed to 1.0 while they might expect it to parse to 10.0

Yep, and we want to change the behavior of the customer to promote the best practice.

This is similar with mobile TFMs such as monoandroid10 vs. monoandroid10.0. One is 1.0 & the other is 10.0.

So TargetFramework = monoandroid, TargetFrameworkVersion = v1.0

If anything, we should be promoting the separator across the board, it's the best practice is it not? Everything in the NET 5 spec suggests to use a period separator for all TargetBlahVersion properties.

https://github.com/dotnet/designs/blob/master/accepted/2020/net5/net5.md#upgrading-the-os-bindings

https://github.com/dotnet/designs/blob/master/accepted/2020/net5/net5.md#lighting-up-on-later-os-versions

The bigger question is best practices for the <TargetFramework> element here. Is it period separator or not? Prior to .NET Core it looks like no, it doesn't matter. After .NET Core it seems that it should be included. Thoughts?

Separator across the board is the right approach.

Everything in the NET 5 spec suggests to use a period separator for all TargetBlahVersion properties.

Those are not user defined usually.
The idea is that the TargetFramework property can be anything.
NuGet doesn't parse it, which is why the check is only happening at pack time.

Shouldn't we create an issue on the SDK team, or contribute a pull request ourselves, to do it in the SDK instead? It doesn't make much sense to me to do at pack time. It should be done at restore, but only when the monikers were not provided and the TargetFramework property was parsed, hence why it can only be done in the SDK and not in NuGet.

@zivkan So what I'm hearing is that the suggestion is to drop this PR in favor of doing this SDK-side in a more general way?

As far as whether to warn always or warn only for certain frameworks -- I think the concern was that this could get very noisy, very quickly for people. I mean even for us, we'd need to change a bunch of net45/net472 items in our tests and projects?

yes, my opinion is close this PR, and someone should make a change in the SDK instead.

The key is, do not warn if my csproj has:

<Project Sdk="Microsoft.NET.Sdk"> <PropertyGroup> <TargetFrameworks>whatever</TargetFrameworks> </PropertyGroup> <PropertyGroup Condition=" '$(TargetFramework)' == 'whatever' "> <TargetFrameworkIdentifier>.NETCoreApp</TargetFrameworkIdentifier> <TargetFrameworkVersion>v5.0</TargetFrameworkIdentifier> <TargetFrameworkMoniker>$(TargetFrameworkIdentifier),Version=$(TargetFrameworkVersion)</TargetFrameworkMoniker> </PropertyGroup> </Project>

This csproj doesn't have a . in the TargetFramework value, but it doesn't need one because it correctly sets the 3 other properties needed to correctly define the canonical target framework. Only when the SDK parses the TargetFramework value to set the other properties itself, in that situation warn about missing dots.

And yes, you're right about the noisy warnings. Just like this PR, we should only warn when the TFM ends up being TFI='.NETCoreApp' and TFV >= '5.0'. Or any TFI='.NETStandard' apparently.

I think we should still do it on our side in pack because people can manually create packages. We should catch things like that in the nuspec and package folders.

src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs

kartheekp-ms · 2020-09-02T05:52:45Z

src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs

+                    {
+                        var dotIdx = targetFramework.IndexOf('.');
+                        var dashIdx = targetFramework.IndexOf('-');
+                        var isDottedFwVersion = (dashIdx > -1 && dotIdx > -1 && dotIdx < dashIdx) || dotIdx > -1;


Suggested change

var isDottedFwVersion = (dashIdx > -1 && dotIdx > -1 && dotIdx < dashIdx) || dotIdx > -1;

var isDottedFwVersion = (dotIdx > -1 && dashIdx > dotIdx) || dotIdx > -1;

if dotIdx > -1 and dashIdx > dotIdx then we can imply that dashIdx > -1. Hence removed that condition. Please help me understand why we need to check the presence of - char in the string.

I see that this code is executed only when packing via csproj, not nuspec. Don't we need this rule when packing using nuspec?

Please help me understand why we need to check the presence of - char in the string.

We need to check for the dash because the dot might be in the platform part of the tfm (net50-windows7.0)

I see that this code is executed only when packing via csproj, not nuspec. Don't we need this rule when packing using nuspec?

I don't know why we do this, but I'm still figuring out how Pack works overall. @nkolev92 @zivkan do y'all have any idea what this difference is about?

When we pack a nuspec, I don't believe that NuGet implicitly adds anything to the package. @zkat you should confirm, but I think it's 100% up to the customer writing he nuspec to explicitly define everything in the package.

However, this got me thinking, TargetFramework is now just an alias. NuGet should not be using this string in the lib/<tfm>/ folder structure, it should be parsing the TargetFrameworkMoniker and TargetPlatformMoniker into a NuGetFramework, then calling .GetShortFolderName(), which will correctly add the .innet5.0, even if it was missing in TargetFramework`.

Maybe we need a test for this, but it shouldn't require a product change to pack. I'd say maybe there should be a warning about this for restore, but it's not really feasible for NuGet to do the check, I think the .NET SDK would need to. By the time it gets to NuGet, we don't know if the TargetFramework was parsed, and therefore needs the warning, or if it was just an alias, and therefore we should not warn. Hence I think it's the .NET SDK's responsibility if this should be warned.

I mostly agree with @zivkan.

We should add a test/warning for the manual nuspec scenario. Beyond that, it think the SDK might make more sense.

Tbh, it's not a deal breaker if we can't add it I think. The templates are creating the correct things.

test/NuGet.Core.Tests/NuGet.Build.Tasks.Pack.Test/PackTaskLogicTests.cs

nkolev92

I don't know if the nuspec pack is handled here.

Worth adding a test for that

nkolev92 · 2020-09-02T19:30:46Z

src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs

+                    var isNet5EraTfm = fw.Version.Major >= 5 &&
+                        StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, fw.Framework);
+                    var isNetStandard = StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetStandard, fw.Framework);
+                    if (isNet5EraTfm || isNetStandard)


Why are we checking the netstandard versions?

NuGet/Home#9215 (comment)

There'll be no new versions of netstandard.
The true risk here is what happens when we hit net10.0. The risk is not there with netstandard.

Keep in mind that there are existing packages/projects with netstandard. I see no reason to raise an extra warning for something that won't really affect them.

Co-authored-by: Kartheek Penagamuri <52756182+kartheekp-ms@users.noreply.github.com>

aortiz-msft

terrajobst · 2020-09-17T19:46:09Z

Any reason this wasn't merged yet? I'd love to close dotnet/runtime#34173.

zkat · 2020-09-18T18:32:56Z

I'm closing this and making a single PR for all the .net5-related changes (or I might split them up later, but I'm working on a single branch right now)

zkat added 4 commits August 28, 2020 14:28

error when TPV is missing from dependencies

0954642

Fixes: NuGet/Home#9441

format fix

1815b11

add platform version check to pack, too

f648027

warn on net5+ TFMs when dot is missing

5ef3780

Fixes: NuGet/Home#9215

zkat requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, heng-liu, kartheekp-ms, nkolev92, rrelyea and zivkan as code owners September 1, 2020 20:58

zivkan reviewed Sep 1, 2020

View reviewed changes

kartheekp-ms reviewed Sep 1, 2020

View reviewed changes

src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs Show resolved Hide resolved

zkat changed the title ~~omitted dots bad~~ warning when dot is omitted from targetframework version in net5.0+ Sep 1, 2020

zkat requested a review from kartheekp-ms September 2, 2020 04:54

kartheekp-ms reviewed Sep 2, 2020

View reviewed changes

fix detection of dots on dashed frameworks

7869d57

nkolev92 reviewed Sep 2, 2020

View reviewed changes

Update src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs

af6ee20

Co-authored-by: Kartheek Penagamuri <52756182+kartheekp-ms@users.noreply.github.com>

aortiz-msft approved these changes Sep 4, 2020

View reviewed changes

zkat force-pushed the dev-zkat-missing-tpv branch from 3bddcfb to 27d89d0 Compare September 8, 2020 21:30

zkat requested a review from a team as a code owner September 8, 2020 21:30

Base automatically changed from dev-zkat-missing-tpv to dev September 9, 2020 02:39

zkat closed this Sep 18, 2020

nkolev92 deleted the dev-zkat-omitted-dots-bad branch May 19, 2021 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

warning when dot is omitted from targetframework version in net5.0+ #3625

warning when dot is omitted from targetframework version in net5.0+ #3625

zkat commented Sep 1, 2020 •

edited

Loading

zivkan left a comment

zivkan Sep 1, 2020

JonDouglas Sep 2, 2020 •

edited

Loading

nkolev92 Sep 2, 2020

JonDouglas Sep 2, 2020

nkolev92 Sep 2, 2020

zivkan Sep 2, 2020

zkat Sep 3, 2020

zivkan Sep 3, 2020

nkolev92 Sep 3, 2020

kartheekp-ms Sep 2, 2020

kartheekp-ms Sep 2, 2020

zkat Sep 2, 2020

zivkan Sep 2, 2020

nkolev92 Sep 8, 2020 •

edited

Loading

nkolev92 left a comment

nkolev92 Sep 2, 2020

zkat Sep 3, 2020

nkolev92 Sep 3, 2020

aortiz-msft left a comment

terrajobst commented Sep 17, 2020

zkat commented Sep 18, 2020

	var isDottedFwVersion = (dashIdx > -1 && dotIdx > -1 && dotIdx < dashIdx) \|\| dotIdx > -1;
	var isDottedFwVersion = (dotIdx > -1 && dashIdx > dotIdx) \|\| dotIdx > -1;

warning when dot is omitted from targetframework version in net5.0+ #3625

warning when dot is omitted from targetframework version in net5.0+ #3625

Conversation

zkat commented Sep 1, 2020 • edited Loading

zivkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonDouglas Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aortiz-msft left a comment

Choose a reason for hiding this comment

terrajobst commented Sep 17, 2020

zkat commented Sep 18, 2020

zkat commented Sep 1, 2020 •

edited

Loading

JonDouglas Sep 2, 2020 •

edited

Loading

nkolev92 Sep 8, 2020 •

edited

Loading