add support for net5.0-<platform> #3339

zkat · 2020-04-10T03:45:51Z

This PR adds support for net5.0-<platform> to NuGetFramework and related classes.

This isn't quite ready to merge, but I'd like to start getting reviews about the stuff that's already there. It turns out that I was missing a lot of stuff in NuGet/Home#9382, which I can go back and update the document with, although I'm tempted to just drop that PR altogether in favor of a straight-up implementation.

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

src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs

src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs

src/NuGet.Core/NuGet.Packaging/compiler/resources/nuspec.xsd

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

rrelyea

🕐

looks very good. found a few minor things so far.

src/NuGet.Core/NuGet.Frameworks/FrameworkConstants.cs

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

src/NuGet.Core/NuGet.Frameworks/CompatibilityProvider.cs

src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs

src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs

nkolev92

Looks good so far.

A few questions/suggestions.
No complaints on the public API part.

src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs

src/NuGet.Core/NuGet.Packaging/compiler/resources/nuspec.xsd

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

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

cristinamanum · 2020-06-02T18:15:12Z

src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs

@@ -134,6 +137,20 @@ public bool TryGetShortProfile(string frameworkIdentifier, string profile, out s
            return TryConvertOrNormalize(profile, _profilesToShortName, _profileShortToLong, out profileShortName);
        }

+        public bool TryGetPlatform(string frameworkIdentifier, Version frameworkVersion, string platformIdentifier, out string platformShortName)


Are there null checks needed?

The platformIdentifier is not used for an identification is this intended?

Do we need UnitTests for this method?

cristinamanum · 2020-06-02T18:31:41Z

src/NuGet.Core/NuGet.Frameworks/def/IFrameworkNameProvider.cs

@@ -33,6 +33,11 @@ interface IFrameworkNameProvider
        /// </summary>
        bool TryGetShortProfile(string frameworkIdentifier, string profile, out string profileShortName);

+        /// <summary>
+        /// Get the official platform name from the short name.


Isn't it the other way: "Get the official platform short name from the platform name" ? The out argument is platformShortName

erdembayar · 2020-06-02T22:24:25Z

src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs

@@ -73,6 +74,8 @@ public FrameworkNameProvider(IEnumerable<IFrameworkMappings> mappings, IEnumerab
            _profilesToShortName = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
            _identifierShortToLong = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
            _profileShortToLong = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+            _platformToShortName = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+            _platformShortToLong = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
            _portableFrameworks = new Dictionary<int, HashSet<NuGetFramework>>();


Added 2 dictionaries( _platformToShortName, _platformShortToLong ) are initialized values, but never used.
Why is that?

oops, it's a leftover from a previous iteration.

erdembayar · 2020-06-02T22:46:19Z

src/NuGet.Core/NuGet.Frameworks/CompatibilityProvider.cs

-            bool? result = _cache.GetOrAdd(cacheKey, (Func<CompatibilityCacheKey, bool>)((key) 
-                => { return IsCompatibleCore(target, candidate) == true; }));
+            bool? result = _cache.GetOrAdd(cacheKey, (Func<CompatibilityCacheKey, bool>)((key)
+                =>


Can we simplify this part like bool? result = _cache.GetOrAdd(cacheKey, IsCompatibleCore(target, candidate) == true);
Otherwise very hard to read and understand.

This looks to me like a re-formatting of existing code, but its functionality hasn't changed at all. I'm all for simplifying syntax, but I consider it out of scope for the PR (I wouldn't complain if it's improved, but I wouldn't hold up the PR because of it)

erdembayar · 2020-06-02T22:58:01Z

src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs

+        /// True if the platform is non-empty
+        /// </summary>
+        public bool HasPlatform
+        {


Can we use more modern syntax here? public bool HasPlatform => !string.IsNullOrEmpty(Platform);

I'd rather keep things consistent with the rest of the file, and I think it's out of scope to change all the other ones that work this way.

erdembayar · 2020-06-02T23:05:42Z

src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs

@@ -92,18 +92,19 @@ public static NuGetFramework ParseFrameworkName(string frameworkName, IFramework
            // if the first part is a special framework, ignore the rest
            if (!TryParseSpecialFramework(parts[0], out result))
            {


Just in case add parts.Any() if expecting at least 1 item in array which is parts[0] for not to get index out of range exception.
if (parts.Any() && !TryParseSpecialFramework(parts[0], out result))

When you know the data type, using .Length > 0 or .Count > 0 is a lot faster than using LINQ to do the same. Of course, this one thing is such a small part of the overall execution that in customer experience % terms it'll be just about zero, but I personally encourage people to avoid LINQ where possible.

erdembayar · 2020-06-02T23:22:53Z

src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs

-                                    bool validProfile = FrameworkConstants.FrameworkProfiles.Contains(profileShort);
-                                    if (validProfile)
+                                    // Find a platform version if it exists and yank it out
+                                    var platformChars = profileShort;


No need this extra platformChars variable here, keep using profileShort unless we do profileShort.ToCharArray(), but most likely no need;

This improves the readability because the 3rd part can be either profile or platform depending on the context.

I'd expect both of these to compile to the same thing anyways.

dsplaisted · 2020-06-03T20:04:47Z

fwiw, I distinctly remember @rrelyea asking whether NuGet should validate the platform names and the answer back then was yes.
It was also captured in:
https://github.com/dotnet/designs/blob/master/accepted/2020/net5/net5.md#tfms-are-a-closed-set

We should update the doc to reflect this change:

@nkolev92 I've sent a designs PR to cover this: dotnet/designs#130

zivkan · 2020-06-03T20:18:13Z

src/NuGet.Core/NuGet.Frameworks/CompatibilityProvider.cs

-            bool? result = _cache.GetOrAdd(cacheKey, (Func<CompatibilityCacheKey, bool>)((key) 
-                => { return IsCompatibleCore(target, candidate) == true; }));
+            bool? result = _cache.GetOrAdd(cacheKey, (Func<CompatibilityCacheKey, bool>)((key)
+                =>


This looks to me like a re-formatting of existing code, but its functionality hasn't changed at all. I'm all for simplifying syntax, but I consider it out of scope for the PR (I wouldn't complain if it's improved, but I wouldn't hold up the PR because of it)

zivkan · 2020-06-03T20:22:00Z

src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs

+                // Always use decimals and 2+ digits for netstandard and
+                // netcoreapp, or if any parts of the version are over 9
+                // we need to use decimals


I wouldn't have noticed if you didn't reformat this comment, but I don't believe this is correct any longer. net5.0 and above (net5Era) should also be using ".0" at the end.

Well, technically net5 will work just fine, but we want to make sure any time we (or the SDK, or anything else calling NuGet's APIs) write the short name, we always use the ".0" version, so customers are used to that syntax for when "10.0" comes along. But I'm not sure if this is a non-net5era codepath, so maybe it's ok.

net5.0 is netcoreapp everywhere internally, so the comment is accurate, and the behavior is correct.

zivkan · 2020-06-03T20:37:22Z

src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs

@@ -92,18 +92,19 @@ public static NuGetFramework ParseFrameworkName(string frameworkName, IFramework
            // if the first part is a special framework, ignore the rest
            if (!TryParseSpecialFramework(parts[0], out result))
            {


When you know the data type, using .Length > 0 or .Count > 0 is a lot faster than using LINQ to do the same. Of course, this one thing is such a small part of the overall execution that in customer experience % terms it'll be just about zero, but I personally encourage people to avoid LINQ where possible.

....Tests/NuGet.VisualStudio.Implementation.Test/Extensibility/VsFrameworkCompatibilityTests.cs

changes were made

nkolev92

Looks great overall.

It's been a long journey, where the bulk of work was agreeing to the right design :)
Great job!

2 smaller comments left. First one is more important and I feel more confident about it.
2nd one is up to you and based on your analysis.

I think GetShortFolderName should normalize by providing the same casing. See: https://nugettoolsdev.azurewebsites.net/5.5.0-preview.2.6382/parse-framework?framework=NEtcoreapp1.0 and https://nugettoolsdev.azurewebsites.net/5.5.0-preview.2.6382/parse-framework?framework=NEtcoreapp1.0. Maybe I missed where it's getting normalized, so let me know.
What's the guidance for customers that can't use the extensibility framework API. Will anyone of note be affected? Do we need an issue tracking that.

src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsFrameworkCompatibility.cs

nkolev92 · 2020-06-04T18:51:51Z

src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs

-                                    bool validProfile = FrameworkConstants.FrameworkProfiles.Contains(profileShort);
-                                    if (validProfile)
+                                    // Find a platform version if it exists and yank it out
+                                    var platformChars = profileShort;


This improves the readability because the 3rd part can be either profile or platform depending on the context.

I'd expect both of these to compile to the same thing anyways.

....Tests/NuGet.VisualStudio.Implementation.Test/Extensibility/VsFrameworkCompatibilityTests.cs

src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs

Fixes: NuGet/Home#9240 add TFI and TFV arguments + return null for GetNearest stop verifying platform names let bad frameworkname parses fail explosively fix tests more PR feedback fixes normalize platform shortnames and suppress resulting warning

zkat · 2020-06-05T19:45:50Z

@nkolev92 I've created an issue for it: NuGet/Home#9650

This reverts commit 071769b.

zkat requested a review from rrelyea April 10, 2020 03:45

zkat requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, heng-liu, kartheekp-ms, nkolev92 and zivkan as code owners April 10, 2020 03:45

rrelyea reviewed Apr 13, 2020

View reviewed changes

test/NuGet.Core.Tests/NuGet.Frameworks.Test/CompatibilityTests.cs Outdated Show resolved Hide resolved

rrelyea reviewed Apr 13, 2020

View reviewed changes

src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs Outdated Show resolved Hide resolved

rrelyea reviewed Apr 13, 2020

View reviewed changes

src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs Outdated Show resolved Hide resolved

rrelyea reviewed Apr 13, 2020

View reviewed changes

src/NuGet.Core/NuGet.Packaging/compiler/resources/nuspec.xsd Outdated Show resolved Hide resolved

rrelyea reviewed Apr 13, 2020

View reviewed changes

test/NuGet.Core.Tests/NuGet.Frameworks.Test/CompatibilityTests.cs Show resolved Hide resolved

rrelyea reviewed Apr 14, 2020

View reviewed changes

test/NuGet.Core.Tests/NuGet.Frameworks.Test/CompatibilityTests.cs Show resolved Hide resolved

rrelyea previously requested changes Apr 14, 2020

View reviewed changes

zkat force-pushed the dev-zkat-net5.0-platforms branch from 773c045 to 51a29a5 Compare April 14, 2020 00:14

rolfbjarne reviewed Apr 14, 2020

View reviewed changes

src/NuGet.Core/NuGet.Frameworks/FrameworkConstants.cs Outdated Show resolved Hide resolved

test/NuGet.Core.Tests/NuGet.Frameworks.Test/NuGetFrameworkParseTests.cs Outdated Show resolved Hide resolved