net5.0 warnings, errors, and auto-translation of aliases #3678

zkat · 2020-09-21T03:41:24Z

nkolev92

Not a complete review, have a meeting will come back to it after.

I think there's some extra duplication, but I'm not clear about that 100% yet.
Most of the line specific comments should be actionable though.

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

src/NuGet.Core/NuGet.Commands/CommandArgs/PackArgs.cs

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

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

zkat · 2020-09-21T18:35:28Z

@nkolev92 some of the duplication you're seeing is because the code is... a bit of a maze, and I needed to put things in two places just to get them working for both SDK/msbuild scenarios AND nuspec scenarios.

zivkan

Most of my comments are coding guidelines issues, but I would like the public APIs to be improved before I approve.

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

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

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

src/NuGet.Core/NuGet.Packaging/PackageCreation/Resources/NuGetResources.resx

src/NuGet.Core/NuGet.Packaging/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt

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

dominoFire

Comments on NuGet Log codes.

dominoFire · 2020-09-21T20:07:49Z

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

+
+                        string targetFrameworkString = Path.GetDirectoryName(path).Split(Path.DirectorySeparatorChar).ElementAt(1);
+
+                        var isNet5EraTfm = fw.Version.Major >= 5 && StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, fw.Framework);


Can we create an utility method to test .NET 5 era condition? I have seen this in multiple parts of the codebase

public static bool isNet5Era(Version targetFrameworkVersion, string targetFrameworkIdentifier);

See also:

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

Line 94 in 0e54137

IsNet5Era = (_frameworkVersion.Major >= Version5 && StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, _frameworkIdentifier));

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

Lines 139 to 140 in 0e54137

var isNet5EraTfm = targetFrameworkVersion.Major >= 5 &&

StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, targetFrameworkIdentifier);

This came up in review (I originally had this property set in NuGetFramework), but the feedback that I got pretty consistently is that people didn't want to have this as part of that class. /cc @zivkan @nkolev92

I don't like the name IsNet5Era. It's acceptable for internal use, but public APIs should be 1. as obvious/easy to understand as possible, without reading docs 2. try to be "future proof".

Will a IsNet5Era API be easy to understand in 5 or 10 years from now when everyone is using .NET 10, .NET 15? Even now, will someone browsing our public APIs via IntelliSense be able to easily guess what the API means, and when they should use it?

But given we do the check in several assemblies, there's a good chance that some customers will want to as well. Maybe DoesTargetFrameworkUsePlatform(...)? IsPlatformUsed(...)? UsesPlatform(...)? UsesPlatformMoniker(..)?

Also, given the TFM format is {tfi},Version=v{tfv}, I'd suggest that the parameters should be ordered (string identifier, Version version), regardless of what order they're used in the method body.

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

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

zivkan · 2020-09-22T21:11:31Z

src/NuGet.Core/NuGet.Commands/CommandArgs/PackArgs.cs

@@ -42,6 +42,7 @@ public class PackArgs
        public bool Tool { get; set; }
        public string Version { get; set; }
        public bool Deterministic { get; set; }
+        public IDictionary<string, string> AliasMappings { get; set; }


can be done later: I know most of NuGet doesn't have good XML doc, but since this is part of our public API and therefore customers might use it, I think it would be great to have XML doc to explain public things. It might be obvious for other properties such as Version, Deterministic, and so on, but there's no way for customers to guess what this dictionary's key and value represent, so that should be documented. Consider, for example, the xmldocs I wrote for the GetInstalledPackagesAsync model.

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

nkolev92 · 2020-09-22T22:44:25Z

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

+                var isNet5EraTfm = framework.Version.Major >= 5 && StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, framework.Framework);
+                if (isNet5EraTfm)
+                {
+                    var dotIndex = targetFramework.IndexOf('.');


It'd be nice if this was extracted to a utility so that it can get unit tested properly (said utility can be internal).

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

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

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

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

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

src/NuGet.Core/NuGet.Packaging/PackageCreation/Resources/NuGetResources.resx

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

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

zivkan · 2020-09-23T23:38:11Z

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

@@ -44,6 +44,18 @@ public static class Folders
                Analyzers,
                Source
            };
+
+            public static string[] SupportFrameworks { get; } = new string[] {


Can we use ImmutableArray or IReadOnlyCollection or IReadOnlyList instead of string[]? Right now, someone can do `SupportFrameworks[0] = "whatever" and suddenly NuGet is going to behave differently.

zivkan · 2020-09-23T23:40:42Z

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

@@ -44,6 +44,18 @@ public static class Folders
                Analyzers,
                Source
            };
+
+            public static string[] SupportFrameworks { get; } = new string[] {


Can we make this internal? It doesn't seem to be used outside of NuGet.Packaging, and I don't think it's particularly useful for customers using this package.

zivkan · 2020-09-23T23:41:10Z

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

+                Tools,
+                ContentFiles,
+                Lib,
+                Native,


I've never heard of a native/ package folder before, although I see it's listed in the Known array. Still, I'm not sure we ever look for native/<tfm>/*.

zivkan · 2020-09-23T23:48:10Z

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

+                    }
+                    else
+                    {
+                        frameworkPart = path.Substring(folderPrefix.Length);


I just remembered that a class named ManagedCodeConventions exists, and it lists the path conventions for each. For example, look at the runtimes folder: https://github.com/NuGet/NuGet.Client/blob/9f521755495c05bc03efd5365762e32fde98761f/src/NuGet.Core/NuGet.Packaging/ContentModel/ManagedCodeConventions.cs#L415

also the multi-targetd build files has a deprecated path that is still used:
https://github.com/NuGet/NuGet.Client/blob/9f521755495c05bc03efd5365762e32fde98761f/src/NuGet.Core/NuGet.Packaging/ContentModel/ManagedCodeConventions.cs#L505-L508

I assume that the PatternSet class has methods to parse the path for you, so you don't need to do all this substring stuff.

I'm really sorry I didn't remember this earlier :( particularly as the current implementation doesn't correctly get the TFM for all package folders.

I even remember giving this feedback to Joshua a while ago :(

Here's a rule that makes use of that.

https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Packaging/Rules/NoRefOrLibFolderInPackageRule.cs

nkolev92

I think a good test would be to run E2E test for all the places where framework is inferred from the path.
In particular I think it's lib + contentFiles folders.

Basically I searched for TfmSpecific in the pack.targets, https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets.

nkolev92 · 2020-09-24T00:46:20Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

            }
            else
            {
                packagePath = Path.GetFileName(fullPath);
            }
-            return Path.Combine(targetPath ?? String.Empty, packagePath);
+
+            string path = Path.Combine(targetPath ?? string.Empty, packagePath);


Looking through PackTaskLogic, there are FrameworkAssemblyRefs that are also gonna have this problem.

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

Line 332 in 3431848

var targetFramework = tfmRef.GetProperty("TargetFramework");

.

AssemblyReferences are not a thing in netcore though, so it's only really, really custom scenarios that'll be broken.

It's safe to fix later

zivkan · 2020-09-24T21:30:20Z

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

+                    var frameworkVersionHasDot = (dashIndex > -1 && dotIndex > -1 && dotIndex < dashIndex) || (dashIndex == -1 && dotIndex > -1);
+                    if (!frameworkVersionHasDot)
+                    {
+                        logger.LogWarning(string.Format(CultureInfo.CurrentCulture, Strings.MissingRequiredDot, NuGetLogCode.NU5502, targetFramework));


This code block is checking the lib/<tfm>/project.dll tfm to make sure it has a dot. Since this check is also being done in PackageBuilder, this code is an unnecessary duplicate.

zivkan · 2020-09-24T21:36:36Z

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

+        /// <summary>
+        /// MissingRequiredDot
+        /// </summary>
+        NU5502 = 5502,


5501 and 5502 are the same scenario, just in different parts of the code. We should only have one code for the scenario.

zivkan · 2020-09-24T21:50:26Z

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

@@ -34,6 +38,60 @@ internal static bool IsValidFrameworkName(string path)
            return fx != null && fx.Framework != NuGetFramework.UnsupportedFramework.Framework;
        }

+        internal static bool IsDottedFrameworkVersion(string path)
+        {
+            foreach (string knownFolder in PackagingConstants.Folders.SupportFrameworks)


The way this works is

iterate this list of folder names

generate a path prefix for the folder name

use managedCodeConventions.Patterns.AnyTargettedFile to find a list of all <any>/<tfm>/<any?> files

filter the list of "any targetted file" with the prefix from 2.

However, if you instead have a list of PatternSet of interesting folders, then you can call ContenxtExtractor.GetContentForPattern on this pattern set, and avoid having to filter the results.

zivkan · 2020-09-24T21:52:08Z

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

+                    collection.Load(new string[] { path });
+
+                    var targetedItems = ContentExtractor.GetContentForPattern(collection, managedCodeConventions.Patterns.AnyTargettedFile);


you can add all the files in the package, and let ContentExtractor return the files that match, rather than this one-file-at-a-time approach.

zivkan · 2020-09-24T21:53:16Z

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

+                                return true;
+                            }
+
+                            string targetFrameworkString = Path.GetDirectoryName(path).Split(Path.DirectorySeparatorChar).ElementAt(1);


runtimes/ does not put tfm at index 1.

zivkan · 2020-09-24T21:57:39Z

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

+                            {
+                                var dotIndex = targetFrameworkString.IndexOf('.');
+                                var dashIndex = targetFrameworkString.IndexOf('-');
+                                var frameworkVersionHasDots = (dashIndex > -1 && dotIndex > -1 && dotIndex < dashIndex) || (dashIndex == -1 && dotIndex > -1);


follow up later: Looking at the net5 tfm spec, it looks like we're supposed to ensure that target platform version also has a dot.

zivkan · 2020-09-24T22:00:01Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

+            string path = Path.Combine(targetPath ?? string.Empty, packagePath);
+
+            // Translate the tfm alias to its actual framework if necessary.
+            foreach (string knownFolder in PackagingConstants.Folders.Known)


Folders.Known doesn't seem right, since there are folders that don't have a TFM folder.

zivkan · 2020-09-24T22:03:31Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

+                    if (knownFolder == PackagingConstants.Folders.ContentFiles && path.Count(sep => sep == Path.DirectorySeparatorChar) > 2)
+                    {
+                        // ContentFiles might have an extra level of nesting for the
+                        // language, so we have to skip over _that_ in order
+                        // to get to the framework itself.
+                        var languageEndIndex = path.IndexOf(System.IO.Path.DirectorySeparatorChar, folderPrefix.Length + 1);
+                        frameworkPart = path.Substring(folderPrefix.Length + (languageEndIndex - folderPrefix.Length));
+                    }
+                    else
+                    {
+                        frameworkPart = path.Substring(folderPrefix.Length);


This code doesn't correctly support runtimes/ as the tfm isn't the second folder name. Maybe we can use ManagedCodeConventions here to extract the TFM reliably?

zivkan · 2020-09-24T22:05:24Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

+                    if (AliasFolderNameMapping != null && AliasFolderNameMapping.TryGetValue(aliasString, out frameworkString))
+#pragma warning restore CS0618
+                    {
+                        path = folderPrefix + frameworkString + Path.DirectorySeparatorChar + path.Substring(folderPrefix.Length + aliasString.Length + 1);


I'm worried this doesn't correctly use the right folderPrefix for contentFiles or runtimes.

zivkan · 2020-09-24T22:08:24Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Resources/NuGetResources.resx

@@ -212,4 +212,8 @@
    <value>The element 'icon' cannot be empty.</value>
    <comment>Please, don't localize icon</comment>
  </data>
+  <data name="InvalidPlatformVersion" xml:space="preserve">
+    <value>Target Framework '{0}' has a platform component, but the platform could not be inferred. This is likely a problem with NuGet itself.</value>


Suggested change

<value>Target Framework '{0}' has a platform component, but the platform could not be inferred. This is likely a problem with NuGet itself.</value>

<value>Target Framework '{0}' has a platform component, but the platform could not be inferred.</value>

If a customer does <None Include="whatever.txt" PackagePath="lib/net5.0-windows/" />, then this isn't an internal issue at all.

Fixes: NuGet/Home#10020

nkolev92

We're obviously missing tests, but I am ok as long as the product changes stay the same :D

* translate aliases for lib and contentFiles Fixes: NuGet/Home#10020 * quick test for the lib part

zkat requested a review from a team as a code owner September 21, 2020 03:41

zkat force-pushed the dev-zkat-net5-pack-changes branch from fefb519 to fe93439 Compare September 21, 2020 03:51

nkolev92 reviewed Sep 21, 2020

View reviewed changes

zivkan reviewed Sep 21, 2020

View reviewed changes

dominoFire reviewed Sep 21, 2020

View reviewed changes

zkat requested review from nkolev92 and zivkan September 21, 2020 23:01

zivkan reviewed Sep 22, 2020

View reviewed changes

zkat requested a review from zivkan September 22, 2020 21:57

nkolev92 reviewed Sep 22, 2020

View reviewed changes

zkat requested review from nkolev92 and dominoFire September 23, 2020 23:39

zivkan reviewed Sep 23, 2020

View reviewed changes

nkolev92 reviewed Sep 24, 2020

View reviewed changes

zivkan reviewed Sep 24, 2020

View reviewed changes

translate aliases for lib and contentFiles

2d349b8

Fixes: NuGet/Home#10020

zkat force-pushed the dev-zkat-net5-pack-changes branch from d5a775b to 2d349b8 Compare September 24, 2020 22:51

nkolev92 approved these changes Sep 24, 2020

View reviewed changes

zivkan approved these changes Sep 24, 2020

View reviewed changes

quick test for the lib part

7efef9a

aortiz-msft merged commit 0f052fe into dev Sep 24, 2020

aortiz-msft deleted the dev-zkat-net5-pack-changes branch September 24, 2020 23:23

zkat added a commit that referenced this pull request Sep 24, 2020

net5.0 warnings, errors, and auto-translation of aliases (#3678)

2582e3e

* translate aliases for lib and contentFiles Fixes: NuGet/Home#10020 * quick test for the lib part

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net5.0 warnings, errors, and auto-translation of aliases #3678

net5.0 warnings, errors, and auto-translation of aliases #3678

zkat commented Sep 21, 2020 •

edited

Loading

nkolev92 left a comment

zkat commented Sep 21, 2020

zivkan left a comment

dominoFire left a comment

dominoFire Sep 21, 2020

zkat Sep 21, 2020

zivkan Sep 21, 2020

zivkan Sep 22, 2020

nkolev92 Sep 22, 2020

zivkan Sep 23, 2020

zivkan Sep 23, 2020

zivkan Sep 23, 2020

zivkan Sep 23, 2020

nkolev92 Sep 24, 2020

nkolev92 left a comment

nkolev92 Sep 24, 2020

zivkan Sep 24, 2020

zivkan Sep 24, 2020

zivkan Sep 24, 2020

zivkan Sep 24, 2020

zivkan Sep 24, 2020

zivkan Sep 24, 2020

zivkan Sep 24, 2020

zivkan Sep 24, 2020

zivkan Sep 24, 2020

zivkan Sep 24, 2020

nkolev92 left a comment


		string targetFrameworkString = Path.GetDirectoryName(path).Split(Path.DirectorySeparatorChar).ElementAt(1);

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

	var isNet5EraTfm = targetFrameworkVersion.Major >= 5 &&
	StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, targetFrameworkIdentifier);

		collection.Load(new string[] { path });

		var targetedItems = ContentExtractor.GetContentForPattern(collection, managedCodeConventions.Patterns.AnyTargettedFile);

	<value>Target Framework '{0}' has a platform component, but the platform could not be inferred. This is likely a problem with NuGet itself.</value>
	<value>Target Framework '{0}' has a platform component, but the platform could not be inferred.</value>

net5.0 warnings, errors, and auto-translation of aliases #3678

net5.0 warnings, errors, and auto-translation of aliases #3678

Conversation

zkat commented Sep 21, 2020 • edited Loading

nkolev92 left a comment

Choose a reason for hiding this comment

zkat commented Sep 21, 2020

zivkan left a comment

Choose a reason for hiding this comment

dominoFire 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

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 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

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 left a comment

Choose a reason for hiding this comment

zkat commented Sep 21, 2020 •

edited

Loading