warn on missing dots for net5.0+ TFMs #3692

zkat · 2020-09-28T22:27:00Z

Bug

How are we preventing it in future: tests added

Fix

This makes it so we warn when a user uses something like net50 instead of net5.0. We want customers to start getting used to (and will eventually enforce) including dots in .NET version numbers.

Testing/Validation

Tests Added: Yes

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

zivkan · 2020-09-30T19:33:19Z

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

+                {
+                    try
+                    {
+                        var parsedAlias = NuGetFramework.Parse(alias);


Can you remind me why we're doing this check in PackTask?

My memory is that we agreed that the SDK should warn on the same scenario on restore (or evaluation). Restore creates the assets file, and this pack task reads the assets file. Therefore, to get to this code, the customer must already have got the warning from the SDK (once it's implemented).

I thought we were doing this in case they packed a long time after doing a restore?

I think the SDK should be warning here not NuGet.

cc @wli3 is there an issue on your end to track this? Should we create one for you?

@nkolev92 @zivkan When you say we shouldn't do this check in PackTask, do you mean I should also not add the rule? Like, to be clear, are you saying this entire PR is unnecessary?

I think we're referring to this particular check.
This is validating the TargetFramework in the csproj property value. NuGet shouldn't do that.
We should validate the missing dots for anything the customers might have created manually for one or another reason (such as validating the nuspec + files in the package).

Alright. So we'll have to make sure the SDK agrees with the assessment and takes the time to implement this on their end before we can say we're complete. Would it make sense to do it on our end now, since it's implemented, and our timeline is so short, and switch to an SDK-sourced error later? /cc @aortiz-msft

I really don't think NuGet should get in the business of raising a warning for the TargetFramework property even temporarily. We spent lots of time getting away from it :)

The way I see it, that specific warning is a nice to have.

The warning for the TargetFramework property only impacts the project author.

The warning on the package assets can affect all consumers of that package.

I'd argue the latter group is larger.

Do I still need to do anything on my side?

@nkolev92 wait, do the validation rules run when you install/restore, too?...

@wli3 most likely, yes. You'd have to warn when, for example, someone does <TargetFramework>net50</TargetFramework> (that is, when you're resolving an alias that looks like a concrete targetframework).

src/NuGet.Core/NuGet.Packaging/Core/FrameworkNameValidatorUtility.cs

nkolev92 · 2020-10-01T00:30:46Z

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

+                {
+                    try
+                    {
+                        var parsedAlias = NuGetFramework.Parse(alias);


I think the SDK should be warning here not NuGet.

src/NuGet.Core/NuGet.Common/Errors/NuGetLogCode.cs

src/NuGet.Core/NuGet.Packaging/PackagingConstants.cs

src/NuGet.Core/NuGet.Packaging/Rules/InvalidUndottedFrameworkRule.cs

Ref: NuGet/NuGet.Client#3692 (comment)

zkat · 2020-10-06T23:49:03Z

PR for docs page: NuGet/docs.microsoft.com-nuget#2178

nkolev92

I think this looks a lot better, more scoped and just neater overall!

I left a few comments, but don't wait on me obviously :)

src/NuGet.Core/NuGet.Packaging/Rules/InvalidUndottedFrameworkRule.cs

src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkUtility.cs

src/NuGet.Core/NuGet.Packaging/GlobalSuppressions.cs

zivkan · 2020-10-09T15:22:49Z

src/NuGet.Core/NuGet.Packaging/Rules/AnalysisResources.resx

@@ -169,6 +169,10 @@
  <data name="InvalidPrereleaseDependencyWarning" xml:space="preserve">
    <value>A stable release of a package should not have a prerelease dependency. Either modify the version spec of dependency "{0}" or update the version field in the nuspec.</value>
  </data>
+  <data name="InvalidUndottedFrameworkWarning" xml:space="preserve">
+    <value>Packaged folders must include dots in their framework version numbers. This is required as of .NET 5.0. Please rename the following folder(s) to include dots as needed (e.g. 'net50' to 'net5.0'): {0}</value>


The first sentence in this message sounds a bit like 'net47' should be changed to 'net4.7'. It won't be in the list of folder names failing the rule, but to me that's what it sounds like.

Just an idea: What about, instead of doing string parsing to find if the framework version part of the TFM has a dot, we compare the input's string to NuGetFramework.Parse(tfm).ToShortFolderName(), then we can have an error message something along the lines of:

Error NU5501: The following folders do not have the expected target framework folder name: net50 (under lib/) should be net5.0 net50-windows7.0 (under build/) should be net5.0-windows7.0

I don't know if it's better to only report a folder once, like above, or for each file?

net50 (in build/net50/packageId.props) should be net5.0 net50 (in build/net50/packageId.targets) should be net5.0 net50 (in build/net50/myTasks.dll) should be net5.0 net50 (in build/net50/Newtonsoft.Json.dll) should be net5.0

We can still consider only .netcoreapp version >= 5.0, but perhaps this may minimise the potential for misunderstanding which tfm folders need to be renamed.

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

zivkan · 2020-10-09T18:34:16Z

src/NuGet.Core/NuGet.Packaging/PublicAPI/net472/PublicAPI.Unshipped.txt

@@ -1 +1,5 @@
+static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInDependencyGroupsWarning.get -> string


scope creep: It's just a guess, but I guess that AnalysisResources are public only so tests can access the message text without using InternalsVisibleTo. These strings are not useful to customers using the SDK, because we could change the {x} placeholders at any time.

What do you think about making a new resx file that's internal, so we stop getting these public API changes just because we're adding messages?

I'm gonna treat this as scope creep. Maybe we can create an issue to find a better solution for AnalysisResources in general?

src/NuGet.Core/NuGet.Packaging/Rules/InvalidUndottedFrameworkRule.cs

zkat · 2020-10-12T15:47:40Z

Note: Tests are green, but GH apparently didn't update properly.

zivkan · 2020-10-12T21:17:14Z

Note: Tests are green, but GH apparently didn't update properly.

We don't use CI integration with github, because .... reasons. We call GitHub's REST API from our own build scripts. Our builds recently to work a little bit differently, and now the "update github status" script doesn't correctly handle windows unit tests failing :(

This means if unit tests fail, we have to spin a new build until windows unit tests are green. Any other failure can be retried, but windws unit test failures will "break" github check status. I've already created a CI initiative task.

zivkan

I know there are time pressures to get this in, so it's acceptable to me that it gets merged as-is. But particularly my test comments should be considered for normal developments. Maybe my other comments are a bit nit-picky

zivkan · 2020-10-12T21:21:13Z

src/NuGet.Core/NuGet.Packaging/Rules/InvalidUndottedFrameworkRule.cs

+            // IPackageRule expects to get messages passed in, but we output
+            // different kinds of messages, so each individual validator is
+            // responsible for its own messaging.
+            MessageFormat = messageFormat;


Why not remove the parameter from the constructor?

There are already 3 rules that use the default constructor

zivkan · 2020-10-12T21:26:25Z

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

@@ -9,6 +9,7 @@
 using NuGet.Commands;
 using NuGet.Frameworks;
 using NuGet.Packaging;
+using NuGet.Packaging.Core;


this is unnecessary, given there are no other changes in the file

zivkan · 2020-10-12T21:26:49Z

test/NuGet.Core.Tests/NuGet.Frameworks.Test/NuGetFrameworkUtilityTests.cs

@@ -117,5 +117,6 @@ public void NuGetFrameworkUtility_GetNearest_Duplicates()
            Assert.Null(nearestWithSelector);
            Assert.Null(nearest);
        }
+


there are no other changes in this file. Ideally it's reverted.

zivkan · 2020-10-12T21:31:32Z

test/NuGet.Core.Tests/NuGet.Packaging.Test/InvalidUndottedFrameworkRuleTests.cs

+
+namespace NuGet.Packaging.Test
+{
+    public class InvalidUndottedFrameworkRuleTests


Apart from the FrameworkVersionHasDesiredDots test, the others are missing tests to ensure they don't generate the warning when:

The package correctly dots net5.0

The package doesn't contain any TFM that should have dots (for example, contains only netfx, netstandard or netcoreapp tfms)

I can't think of any other missing test cases, but particularly when you have easy to write unit tests, please remember to have tests for: happy case, error case, edge cases.

Another missing thing from the tests are validating that they generate the correct number of warnings. If any of the XML elements has both net50 and net50-windows7.0, will it generate the correct number of warnings? Taking a look at the code, it appears that you'll be generating one warning per TFM, whereas I thought that an earlier comment requested that we generate one message per "message format", with a list of all the TFMs failing the validation.

Finally, and this one is much more subjective, but particularly when generating messages with multiple generated parts, I think it might be useful to have a test to ensure the generated message looks correct:

NuGet.Client/test/NuGet.Core.Tests/NuGet.Packaging.Test/UpholdBuildConventionRuleTests.cs

Lines 139 to 143 in 16be252

var expectedMessage = "- At least one .props file was found in 'build/net45/', but 'build/net45/packageId.props' was not." + Environment.NewLine +

"- At least one .props file was found in 'build/net462/', but 'build/net462/packageId.props' was not." + Environment.NewLine +

"- At least one .props file was found in 'build/net471/', but 'build/net471/packageId.props' was not." + Environment.NewLine +

"- At least one .props file was found in 'build/netstandard1.3/', but 'build/netstandard1.3/packageId.props' was not." + Environment.NewLine +

"- At least one .props file was found in 'build/netcoreapp1.1/', but 'build/netcoreapp1.1/packageId.props' was not." + Environment.NewLine;

Taking a look at the code, it appears that you'll be generating one warning per TFM, whereas I thought that an earlier comment requested that we generate one message per "message format", with a list of all the TFMs failing the validation.

The code actually generates one message per "check type", each with all relevant TFMed values concatenated together into a single message. Additionally, a general "oops you got a NU5501" message is appended if any of these generate a warning.

Re: tests, working on those now, should be ready soon.

I've omitted the tests that check specific messages, and that the right number of messages is displayed, but I've also done a small refactor so that even more stuff is unit-testable, so this can be done later, after the current rush.

Fixes: NuGet/Home#9215

This reverts commit d06781b.

Ref: NuGet/NuGet.Client#3692 (comment)

zkat force-pushed the dev-zkat-net5-missing-dot-warning branch 3 times, most recently from 6e70c2f to 08ed72d Compare September 30, 2020 00:02

zkat marked this pull request as ready for review September 30, 2020 18:49

zkat requested a review from a team as a code owner September 30, 2020 18:49

zkat force-pushed the dev-zkat-net5-missing-dot-warning branch from 50d8a97 to 1b1ae62 Compare September 30, 2020 18:50

zivkan reviewed Sep 30, 2020

View reviewed changes

nkolev92 reviewed Oct 1, 2020

View reviewed changes

zkat force-pushed the dev-zkat-net5-missing-dot-warning branch from 35f35c1 to b1822e3 Compare October 6, 2020 21:41

zkat requested review from nkolev92 and zivkan October 6, 2020 21:50

zkat force-pushed the dev-zkat-net5-missing-dot-warning branch from b1822e3 to e59ea92 Compare October 6, 2020 22:23

zkat mentioned this pull request Jun 1, 2024

Document NU5501 NuGet/Home#13489

Open

zkat added a commit to NuGet/docs.microsoft.com-nuget that referenced this pull request Oct 6, 2020

Create NU5501.md

f879b7d

Ref: NuGet/NuGet.Client#3692 (comment)

zkat mentioned this pull request Oct 6, 2020

Create NU5501.md NuGet/docs.microsoft.com-nuget#2178

Merged

nkolev92 reviewed Oct 7, 2020

View reviewed changes

zivkan reviewed Oct 9, 2020

View reviewed changes

zkat requested review from nkolev92 and zivkan October 9, 2020 23:47

zivkan approved these changes Oct 12, 2020

View reviewed changes

Warn when dot missing from target framework strings

ccd07af

Fixes: NuGet/Home#9215

zkat force-pushed the dev-zkat-net5-missing-dot-warning branch from 829ffe9 to ccd07af Compare October 12, 2020 23:40

aortiz-msft merged commit 35126a1 into dev Oct 13, 2020

aortiz-msft deleted the dev-zkat-net5-missing-dot-warning branch October 13, 2020 18:20

zkat added the servicing-consider label Oct 16, 2020

zkat added a commit that referenced this pull request Oct 16, 2020

Warn when dot missing from target framework strings (#3692)

d06781b

Fixes: NuGet/Home#9215

zkat added a commit that referenced this pull request Oct 20, 2020

Warn when dot missing from target framework strings (#3692)

3d59f19

Fixes: NuGet/Home#9215

zkat added a commit that referenced this pull request Oct 26, 2020

Revert "Warn when dot missing from target framework strings (#3692)"

602ed08

This reverts commit d06781b.

jagilber added a commit to jagilber/CollectServiceFabricData that referenced this pull request Apr 17, 2021

modify net5 -> net5.0 for nuspec change warning NuGet/NuGet.Client#3692

28bb7de

nkolev92 pushed a commit to NuGet/docs.microsoft.com-nuget that referenced this pull request Mar 31, 2022

Create NU5501.md

b395d73

Ref: NuGet/NuGet.Client#3692 (comment)

jagilber added a commit to jagilber/CollectServiceFabricData that referenced this pull request Oct 23, 2022

modify net5 -> net5.0 for nuspec change warning NuGet/NuGet.Client#3692

082a050

jagilber added a commit to jagilber/CollectServiceFabricData that referenced this pull request Oct 23, 2022

modify net5 -> net5.0 for nuspec change warning NuGet/NuGet.Client#3692

fdf709f

jagilber added a commit to jagilber/CollectServiceFabricData that referenced this pull request Oct 23, 2022

modify net5 -> net5.0 for nuspec change warning NuGet/NuGet.Client#3692

3048966

jagilber added a commit to jagilber/CollectServiceFabricData that referenced this pull request Oct 23, 2022

modify net5 -> net5.0 for nuspec change warning NuGet/NuGet.Client#3692

b640977

jagilber added a commit to jagilber/CollectServiceFabricData that referenced this pull request Oct 23, 2022

modify net5 -> net5.0 for nuspec change warning NuGet/NuGet.Client#3692

1956b8b

jagilber added a commit to microsoft/CollectServiceFabricData that referenced this pull request Oct 23, 2022

modify net5 -> net5.0 for nuspec change warning NuGet/NuGet.Client#3692

bb5647a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

warn on missing dots for net5.0+ TFMs #3692

warn on missing dots for net5.0+ TFMs #3692

zkat commented Sep 28, 2020 •

edited

Loading

zivkan Sep 30, 2020

zkat Sep 30, 2020

nkolev92 Oct 1, 2020

zkat Oct 5, 2020

nkolev92 Oct 5, 2020

zkat Oct 5, 2020

nkolev92 Oct 5, 2020

wli3 Oct 5, 2020

zkat Oct 5, 2020

zkat Oct 6, 2020

nkolev92 Oct 1, 2020

zkat commented Oct 6, 2020

nkolev92 left a comment

zivkan Oct 9, 2020

zivkan Oct 9, 2020

zkat Oct 9, 2020

zkat commented Oct 12, 2020

zivkan commented Oct 12, 2020

zivkan left a comment

zivkan Oct 12, 2020

zivkan Oct 12, 2020

zivkan Oct 12, 2020

zivkan Oct 12, 2020

zkat Oct 12, 2020

zkat Oct 13, 2020

		@@ -1 +1,5 @@
		static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInDependencyGroupsWarning.get -> string

	var expectedMessage = "- At least one .props file was found in 'build/net45/', but 'build/net45/packageId.props' was not." + Environment.NewLine +
	"- At least one .props file was found in 'build/net462/', but 'build/net462/packageId.props' was not." + Environment.NewLine +
	"- At least one .props file was found in 'build/net471/', but 'build/net471/packageId.props' was not." + Environment.NewLine +
	"- At least one .props file was found in 'build/netstandard1.3/', but 'build/netstandard1.3/packageId.props' was not." + Environment.NewLine +
	"- At least one .props file was found in 'build/netcoreapp1.1/', but 'build/netcoreapp1.1/packageId.props' was not." + Environment.NewLine;

warn on missing dots for net5.0+ TFMs #3692

warn on missing dots for net5.0+ TFMs #3692

Conversation

zkat commented Sep 28, 2020 • edited Loading

Bug

Fix

Testing/Validation

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

zkat commented Oct 6, 2020

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

zkat commented Oct 12, 2020

zivkan commented Oct 12, 2020

zivkan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkat commented Sep 28, 2020 •

edited

Loading