error when TPV is missing from target frameworks #3616

zkat · 2020-08-28T21:28:53Z

zivkan · 2020-08-28T21:59:35Z

The PR title says it's checking dependencies, but the code seems to be checking the project TFM. It seems reasonable to me to make sure the project has a platform version. If someone creates a nupkg without a platform version, do we want to make that nupkg unusable, or just assume it's platform 0.0 and make a best guess at compatibility (compatible with all projects using the same TFM and platform identifier)?
I don't see a task under the net5 epic to make sure that when a nupkg is being created (whether it's via msbuild pack or nuget.exe pack) that net5 TFMs with platforms must have a platform version. Should that work be done in this PR, or another issue/PR?

zkat · 2020-08-28T23:14:58Z

yeah, that was a brain fart. Fixed!
I was thinking of just doing this check during restore -- won't that block packs, too?

zivkan · 2020-08-28T23:51:28Z

I think if the obj folder is clean, then yes, pack will fail. But if they have a valid project, restore so an assets file is created, then change the project so they don't have a TPV, I don't know what will happen if they try to pack.

I was also wondering if we need to do any checks if the customer packs with a custom nuspec, but I guess we don't validate any other lib/folder is a valid TFM, so probably no need to do anything extra.

nkolev92 · 2020-08-29T00:10:36Z

I think adding it to pack might be a reasonable suggestion.

I think if the obj folder is clean, then yes, pack will fail. But if they have a valid project, restore so an assets file is created, then change the project so they don't have a TPV, I don't know what will happen if they try to pack.

Pack shouldn't happen if a restore fails. Build shouldn't happen if a restore fails.
Worth double checking though.

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs

aortiz-msft · 2020-09-02T17:46:17Z

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

+                if (!string.IsNullOrEmpty(targetFramework))
+                {
+                    var fw = NuGetFramework.Parse(targetFramework);
+                    if (!string.IsNullOrEmpty(fw.Platform) && fw.PlatformVersion == FrameworkConstants.EmptyVersion)


!string.IsNullOrEmpty(fw.Platform) && fw.PlatformVersion == FrameworkConstants.EmptyVersion [](start = 24, length = 91)

Just for my understanding, is there a case where having a PlatformVersion without a Platform is valid?

In that case, do we need the first check for fw.Platform?

Suggested change

if (!string.IsNullOrEmpty(fw.Platform) && fw.PlatformVersion == FrameworkConstants.EmptyVersion)

if (fw.PlatformVersion == FrameworkConstants.EmptyVersion)

Oh, yes we do, because "PlatformVersion" will be EmptyVersion when Platform is empty, and we only wanna do this check when we definitely have a platform.

nkolev92

Looks like a great start.

I think we should make sure we cover the manual edit scenarios in the test.

In particular, nuspec packing where both the targetframework is specified in the dependency group and when the file structure is created by the customer.

ie verify that someone can't just add a

lib/net50/mylib.dll

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

src/NuGet.Core/NuGet.Commands/CommandRunners/PackCommandRunner.cs

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs

src/NuGet.Core/NuGet.Commands/Strings.resx

Fixes: NuGet/Home#9441

aortiz-msft · 2020-09-04T22:30:31Z

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

+                if (!string.IsNullOrEmpty(targetFramework))
+                {
+                    var fw = NuGetFramework.Parse(targetFramework);
+                    if (!string.IsNullOrEmpty(fw.Platform) && fw.PlatformVersion == FrameworkConstants.EmptyVersion)


Suggested change

if (!string.IsNullOrEmpty(fw.Platform) && fw.PlatformVersion == FrameworkConstants.EmptyVersion)

if (fw.PlatformVersion == FrameworkConstants.EmptyVersion)

nkolev92

Can we please cover manual nuspec scenarios + advanced pack targeting scenarios where the user manually redirects to a folder using PackagePath.

In particular I think we need to handle the following scenarios:

A customer manually writes a nuspec, and in the DependencyGroups specifies a version such as net5.0-windows
A customer manually writes a nuspec, and uses the file element and puts a dll in a directory such as lib\net5.0-windows.
A customer says <IncludeBuildOutput>false</IncludeBuildOutput>. And then uses one of the following: https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#targetsfortfmspecificbuildoutput, and redirects manually using PackagePath.

I know these scenarios might be new, so grab some time with me if you need help crafting the tests.

nkolev92 · 2020-09-08T23:32:18Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs

+            {
+                foreach (NuGetFramework fw in badPlatforms)
+                {
+                    _logger.Log(RestoreLogMessage.CreateError(NuGetLogCode.NU1012, string.Format(CultureInfo.CurrentCulture, Strings.Error_PlatformVersionNotPresent, fw.Framework, fw.Platform)));


nit: In an ideal world, I'd recommend these get merged so that a user gets only 1 instead of N errors, but I simply don't see this scenario getting hit too often (actually I'd only expect it during development), so I would not bother too much :)

nkolev92 · 2020-09-09T02:41:13Z

@heng-liu I don't think this change was ready to be merged

* error when TPV is missing from dependencies Fixes: NuGet/Home#9441

nkolev92 · 2020-09-09T02:46:48Z

We should probably create a follow-up here to make sure all scenarios are covered.

We might be adding just tests, but I don't have high confidence that we don't need more product changes.

This reverts commit f172006.

This reverts commit d1e99c9.

) This reverts commit d1e99c9.

zkat requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, heng-liu, kartheekp-ms, nkolev92, rrelyea and zivkan as code owners August 28, 2020 21:28

zkat changed the title ~~error when TPV is missing from dependencies~~ error when TPV is missing from target frameworks Aug 28, 2020

erdembayar reviewed Aug 31, 2020

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Outdated Show resolved Hide resolved

zkat requested a review from a team as a code owner August 31, 2020 23:40

zkat requested a review from erdembayar September 1, 2020 16:09

aortiz-msft reviewed Sep 2, 2020

View reviewed changes

nkolev92 reviewed Sep 2, 2020

View reviewed changes

zkat added 7 commits September 8, 2020 14:08

error when TPV is missing from dependencies

3c5a259

Fixes: NuGet/Home#9441

add platform version check to pack, too

048ee0a

show all bad platform errors

35c3e0f

format fixes

01be6c8

review

8911618

missed one

d7b641a

forgot to update tests >:(

27d89d0

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

zkat requested review from nkolev92 and aortiz-msft September 8, 2020 21:34

aortiz-msft approved these changes Sep 8, 2020

View reviewed changes

nkolev92 reviewed Sep 8, 2020

View reviewed changes

heng-liu merged commit d1e99c9 into dev Sep 9, 2020

heng-liu deleted the dev-zkat-missing-tpv branch September 9, 2020 02:39

heng-liu pushed a commit that referenced this pull request Sep 9, 2020

error when TPV is missing from target frameworks (#3616)

f172006

* error when TPV is missing from dependencies Fixes: NuGet/Home#9441

heng-liu added a commit that referenced this pull request Sep 9, 2020

Revert "error when TPV is missing from target frameworks (#3616)"

5eaf256

This reverts commit f172006.

heng-liu added a commit that referenced this pull request Sep 9, 2020

Revert "error when TPV is missing from target frameworks (#3616)"

b0a4dd3

This reverts commit d1e99c9.

heng-liu mentioned this pull request Sep 9, 2020

Revert "error when TPV is missing from target frameworks (#3616)" #3650

Merged

heng-liu added a commit that referenced this pull request Sep 14, 2020

Revert "error when TPV is missing from target frameworks (#3616)" (#3650

55823aa

) This reverts commit d1e99c9.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

error when TPV is missing from target frameworks #3616

error when TPV is missing from target frameworks #3616

zkat commented Aug 28, 2020

zivkan commented Aug 28, 2020 •

edited

Loading

zkat commented Aug 28, 2020

zivkan commented Aug 28, 2020

nkolev92 commented Aug 29, 2020

aortiz-msft Sep 2, 2020

zkat Sep 2, 2020

aortiz-msft Sep 2, 2020

aortiz-msft Sep 4, 2020

zkat Sep 4, 2020 •

edited

Loading

nkolev92 left a comment

aortiz-msft Sep 4, 2020

nkolev92 left a comment

nkolev92 Sep 8, 2020

nkolev92 commented Sep 9, 2020

nkolev92 commented Sep 9, 2020 •

edited

Loading

	if (!string.IsNullOrEmpty(fw.Platform) && fw.PlatformVersion == FrameworkConstants.EmptyVersion)
	if (fw.PlatformVersion == FrameworkConstants.EmptyVersion)

error when TPV is missing from target frameworks #3616

error when TPV is missing from target frameworks #3616

Conversation

zkat commented Aug 28, 2020

zivkan commented Aug 28, 2020 • edited Loading

zkat commented Aug 28, 2020

zivkan commented Aug 28, 2020

nkolev92 commented Aug 29, 2020

aortiz-msft Sep 2, 2020

Choose a reason for hiding this comment

zkat Sep 2, 2020

Choose a reason for hiding this comment

aortiz-msft Sep 2, 2020

Choose a reason for hiding this comment

aortiz-msft Sep 4, 2020

Choose a reason for hiding this comment

zkat Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

aortiz-msft Sep 4, 2020

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 Sep 8, 2020

Choose a reason for hiding this comment

nkolev92 commented Sep 9, 2020

nkolev92 commented Sep 9, 2020 • edited Loading

zivkan commented Aug 28, 2020 •

edited

Loading

zkat Sep 4, 2020 •

edited

Loading

nkolev92 commented Sep 9, 2020 •

edited

Loading