Add support for AssetTargetFallback #1436

emgarten · 2017-06-02T02:25:21Z

AssetTargetFallback allows falling back to another project framework only if no compatible assets are found in the package.

The changes to lock file creation are almost all to refactor the code into smaller methods. The main change is to loop through creating the library.

Fixes NuGet/Home#5192

mishra14

Small changes then when ready!

mishra14 · 2017-06-02T03:25:37Z

src/NuGet.Clients/NuGet.SolutionRestoreManager/VsSolutionRestoreService.cs

-                    .Select(NuGetFramework.Parse)
-                    .ToList();
+            var ptf = MSBuildStringUtility.Split(GetPropertyValueOrNull(targetFrameworkInfo.Properties, PackageTargetFallback))
+                                          .Select(NuGetFramework.Parse)


fix indenting.

mishra14 · 2017-06-02T04:55:41Z

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

+            // These are only used for framework/RID combinations where content model handles everything.
+            // AssetTargetFallback frameworks will provide multiple criteria since all assets need to be
+            // evaluated before selecting the TFM to use.
+            var orderedCriteriaSets = new List<IReadOnlyList<SelectionCriteria>>(1);


use named parameter.

irrelevant to the context here

mishra14 · 2017-06-02T05:01:01Z

src/NuGet.Core/NuGet.Frameworks/AssetTargetFallbackFramework.cs

+    /// AssetTargetFallbackFramework only fallback when zero assets are selected. These do not 
+    /// auto fallback during GetNearest as FallbackFramework would.
+    /// </summary>
+#if NUGET_FRAMEWORKS_INTERNAL


@joelverhagen ??

What's up? This is so NuGet.Frameworks can be compiled into NuGet.Core without exposing new API surface area.

mishra14 · 2017-06-02T05:11:42Z

src/NuGet.Core/NuGet.Commands/Utility/AssetTargetFallbackUtility.cs

+        /// <summary>
+        /// Returns the fallback framework or the original.
+        /// </summary>
+        public static NuGetFramework GetFramework(NuGetFramework projectFramework, IEnumerable<NuGetFramework> packageTargetFallback, IEnumerable<NuGetFramework> assetTargetFallback)


Consider renaming to GetFallBackFramework

mishra14 · 2017-06-02T05:13:57Z

src/NuGet.Core/NuGet.Commands/Utility/AssetTargetFallbackUtility.cs

+            else if (packageTargetFallback?.Any() == true)
+            {
+                // PackageTargetFallback
+                return new FallbackFramework(projectFramework, packageTargetFallback.AsList());


consider renaming to PackageTargetFallbackFramework

mishra14 · 2017-06-02T05:15:45Z

src/NuGet.Core/NuGet.Commands/Utility/AssetTargetFallbackUtility.cs

+                packageTargetFallback,
+                assetTargetFallback);
+
+            if (assetTargetFallback?.Any() == true)


Use Count, it does not create an enumerator.

this is an IEnumerable

mishra14 · 2017-06-02T05:16:14Z

src/NuGet.Core/NuGet.Commands/Utility/AssetTargetFallbackUtility.cs

+                targetFrameworkInfo.Imports = assetTargetFallback.AsList();
+                targetFrameworkInfo.AssetTargetFallback = true;
+            }
+            else if (packageTargetFallback?.Any() == true)


Use Count, it does not create an enumerator.

mishra14 · 2017-06-02T05:30:32Z

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreNETCoreTest.cs

+                var r = Util.RestoreSolution(pathContext, expectedExitCode: 1);
+
+                // Assert
+                r.AllOutput.Should().Contain("AssetTargetFallback");


Assert on the error would be a better practice.

I'll update this

mishra14 · 2017-06-02T05:31:32Z

test/NuGet.Core.Tests/NuGet.ProjectModel.Test/PackageSpecTestUtility.cs

+{
+    public static class PackageSpecTestUtility
+    {
+        public static PackageSpec RoundTrip(this PackageSpec spec)


mishra14 · 2017-06-02T05:33:01Z

test/TestUtilities/Test.Utility/CommandRunnerResult.cs

@@ -12,6 +13,19 @@ public class CommandRunnerResult
        public string Item2 { get; }
        public string Item3 { get; }

+        public int ExitCode => Item1;


AssetTargetFallback allows falling back to another project framework only if no compatible assets are found in the package. Fixes NuGet/Home#5192 Fixes NuGet/Home#5268

alpaix

Also adding/enhancing tests for the projects would be great.

alpaix · 2017-06-02T16:04:04Z

...NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/LegacyPackageReferenceProject.cs

@@ -295,21 +295,26 @@ private static PackageReference ToPackageReference(LibraryDependency library, Nu
                .GetPackageReferencesAsync(targetFramework, CancellationToken.None))
                .ToList();

-            var packageTargetFallback = _vsProjectAdapter.PackageTargetFallback?.Split(new[] { ';' })
+            var splitChars = new[] { ';' };


static readonly?

I'm mixed on this. I'm not sure that the benefit of making this static out weighs creating this every time the assembly is loaded.

alpaix · 2017-06-02T16:06:10Z

...NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/LegacyPackageReferenceProject.cs

                .Select(NuGetFramework.Parse)
-                .ToList();
+                .ToList()


Although it wasn't covered there before, what happens in case of empty or invalid entries in the list?

NuGetFramework.Parse returns the Unsupported framework since it isn't unusual for future frameworks to come in. For fallback frameworks there is a check to verify that unsupported isn't passed and it will throw.

alpaix · 2017-06-02T16:08:14Z

src/NuGet.Clients/NuGet.SolutionRestoreManager/VsSolutionRestoreService.cs

-                    .ToList();
+            var ptf = MSBuildStringUtility.Split(GetPropertyValueOrNull(targetFrameworkInfo.Properties, PackageTargetFallback))
+                                          .Select(NuGetFramework.Parse)
+                                          .ToList();


The same question here about empty/invalid entries.

alpaix · 2017-06-02T16:17:49Z

src/NuGet.Core/NuGet.Frameworks/AssetTargetFallbackFramework.cs

+        /// <summary>
+        /// List framework to fall back to.
+        /// </summary>
+        public FallbackList Fallback


The convention is to name a collection bearing property with a plural noun to avoid confusion when reading the code. Maybe FallbackList?

I kept this the same as the existing convention in FallbackFramework. I think we should look at removing this and support for net40 now that it is very unlikely that NuGet 2.x will ship again.

alpaix · 2017-06-02T16:24:47Z

src/NuGet.Core/NuGet.Frameworks/AssetTargetFallbackFramework.cs

+        {
+            if (framework == null)
+            {
+                throw new ArgumentNullException("framework");


I'll update these

nameof isn't allowed in net40 unfortunately.

Not exactly. nameof is a C#6 language syntax it can be used when targeting net40.

Not according to Visual Studio 😄

alpaix · 2017-06-02T16:24:59Z

src/NuGet.Core/NuGet.Frameworks/AssetTargetFallbackFramework.cs

+
+            if (fallbackFrameworks == null)
+            {
+                throw new ArgumentNullException("fallbackFrameworks");


alpaix · 2017-06-02T16:25:29Z

src/NuGet.Core/NuGet.Frameworks/AssetTargetFallbackFramework.cs

+
+            if (fallbackFrameworks.Count == 0)
+            {
+                throw new ArgumentException("Empty fallbackFrameworks is invalid", "fallbackFrameworks");


Should the error message be localized?

this would be due to an internal issue, a user can't fix this

alpaix · 2017-06-02T16:26:40Z

src/NuGet.Core/NuGet.Frameworks/AssetTargetFallbackFramework.cs

+                var combiner = new HashCodeCombiner();
+
+                // Ensure that this is different from a FallbackFramework;
+                combiner.AddStringIgnoreCase("assettargetfallback");


also could be nameof(AssetTargetFallbackFramework)

alpaix · 2017-06-02T16:45:42Z

src/NuGet.Core/NuGet.ProjectModel/TargetFrameworkInformation.cs

@@ -22,6 +22,11 @@ public class TargetFrameworkInformation : IEquatable<TargetFrameworkInformation>
        public IList<NuGetFramework> Imports { get; set; } = new List<NuGetFramework>();

        /// <summary>
+        /// If True AssetTargetFallback behavior will be used for Imports.
+        /// </summary>
+        public bool AssetTargetFallback { get; set; }


It's a flag not an actual value. Maybe UseAssetTargetFallback?

This is written to the project.json equivalent, the rest of the properties aren't in this style currently. I'm going to leave this as the msbuild property equivalent for now.

emgarten requested review from rohit21agrawal, nkolev92, zhili1208, alpaix, mishra14 and jainaashish June 2, 2017 02:25

dnfclas added the cla-already-signed label Jun 2, 2017

emgarten force-pushed the dev-feature-logging-atf branch 3 times, most recently from eee7e9b to 01df9f3 Compare June 2, 2017 04:57

mishra14 approved these changes Jun 2, 2017

View reviewed changes

Add support for AssetTargetFallback

2be8291

AssetTargetFallback allows falling back to another project framework only if no compatible assets are found in the package. Fixes NuGet/Home#5192 Fixes NuGet/Home#5268

emgarten force-pushed the dev-feature-logging-atf branch from 01df9f3 to 2be8291 Compare June 2, 2017 05:54

alpaix reviewed Jun 2, 2017

View reviewed changes

emgarten closed this Jun 2, 2017

emgarten deleted the dev-feature-logging-atf branch June 2, 2017 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AssetTargetFallback #1436

Add support for AssetTargetFallback #1436

emgarten commented Jun 2, 2017

mishra14 left a comment

mishra14 Jun 2, 2017

mishra14 Jun 2, 2017

emgarten Jun 2, 2017

mishra14 Jun 2, 2017

joelverhagen Jun 5, 2017

mishra14 Jun 2, 2017

mishra14 Jun 2, 2017

mishra14 Jun 2, 2017

emgarten Jun 2, 2017

mishra14 Jun 2, 2017

mishra14 Jun 2, 2017

emgarten Jun 2, 2017

mishra14 Jun 2, 2017

mishra14 Jun 2, 2017

alpaix left a comment

alpaix Jun 2, 2017

emgarten Jun 2, 2017

alpaix Jun 2, 2017

emgarten Jun 2, 2017

alpaix Jun 2, 2017

alpaix Jun 2, 2017

emgarten Jun 2, 2017

alpaix Jun 2, 2017

emgarten Jun 2, 2017 •

edited

emgarten Jun 2, 2017

alpaix Jun 2, 2017

emgarten Jun 5, 2017

alpaix Jun 2, 2017

alpaix Jun 2, 2017

emgarten Jun 2, 2017

alpaix Jun 2, 2017

alpaix Jun 2, 2017

emgarten Jun 2, 2017

Add support for AssetTargetFallback #1436

Add support for AssetTargetFallback #1436

Conversation

emgarten commented Jun 2, 2017

mishra14 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpaix 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

emgarten Jun 2, 2017 • edited

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

emgarten Jun 2, 2017 •

edited