Add AssignReferenceProperties task for MSBuild #1737

panopticoncentral · 2017-10-06T23:27:55Z

Bug

Link: dotnet/msbuild#1276
Regression: No

Fix

Details: This adds a new task, AssignReferenceProperties, that allows MSBuild to specify a project's target framework and that project's references, and have NuGet resolve what target frameworks should be used for each reference. This avoids having to add logic into MSBuild to figure out the compatibility matrix.

Testing/Validation

Tests Added: Yes
Validation done: Tests + been validated against MSBuild modified to use the task.

dnfclas · 2017-10-06T23:27:59Z

@panopticoncentral,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

nguerrera · 2017-10-09T17:21:27Z

src/NuGet.Core/NuGet.Build.Tasks/AssignReferencePropertiesTask.cs

+
+            var possibleTargetFrameworks = MSBuildStringUtility.Split(targetFramework);
+            var possibleNuGetFrameworks = possibleTargetFrameworks.Select(ParseFramework).ToList();
+            var nearestNuGetFramework = NuGetFrameworkUtility.GetNearest(possibleTargetFrameworks, currentProjectTargetFramework, NuGetFramework.Parse);


Does this preserve the spelling of the target framework in possibleTargetFrameworks: e.g. net4.0 vs net40?

yes, this utility returns the original object back that is associated with the nearest framework, in this case it is the original string

nguerrera · 2017-10-09T17:24:29Z

src/NuGet.Core/NuGet.Build.Tasks/AssignReferencePropertiesTask.cs

+        {
+            var itemWithProperties = new TaskItem(project);
+
+            var targetFramework = project.GetMetadata("TargetFrameworks");


nit: This should be named targetFrameworks

Fixed.

In reply to: 143530143 [](ancestors = 143530143)

nguerrera · 2017-10-09T17:48:22Z

src/NuGet.Core/NuGet.Build.Tasks/AssignReferencePropertiesTask.cs

+                    itemWithProperties.SetMetadata("UndefineProperties",
+                        string.IsNullOrEmpty(undefineProperties)
+                            ? "TargetFramework"
+                            : undefineProperties + ";TargetFramework");


IMHO, this is moving too much build logic in to NuGet. I was expecting the nuget task to be responsible for calculating nearest target framework and nothing else.

I suspect some handling of Rid-agnostic is missing here, which adds to my concern that we're handling this sort of thing here in nuget.

nguerrera

See above

emgarten

minor questions/comments

emgarten · 2017-10-09T18:46:27Z

src/NuGet.Core/NuGet.Build.Tasks/AssignReferencePropertiesTask.cs

+
+            var possibleTargetFrameworks = MSBuildStringUtility.Split(targetFramework);
+            var possibleNuGetFrameworks = possibleTargetFrameworks.Select(ParseFramework).ToList();
+            var nearestNuGetFramework = NuGetFrameworkUtility.GetNearest(possibleTargetFrameworks, currentProjectTargetFramework, NuGetFramework.Parse);


yes, this utility returns the original object back that is associated with the nearest framework, in this case it is the original string

emgarten · 2017-10-09T18:59:02Z

src/NuGet.Core/NuGet.Build.Tasks/AssignReferencePropertiesTask.cs

+            }
+
+            var possibleTargetFrameworks = MSBuildStringUtility.Split(targetFramework);
+            var possibleNuGetFrameworks = possibleTargetFrameworks.Select(ParseFramework).ToList();


Is this trying to validate all frameworks up front? Should this be put in the else block where it is used instead?

Actually, I don't know that there's value in doing this at all, to be honest.

In reply to: 143551005 [](ancestors = 143551005)

emgarten · 2017-10-09T18:59:59Z

src/NuGet.Core/NuGet.Build.Tasks/AssignReferencePropertiesTask.cs

+        {
+            var framework = NuGetFramework.Parse(name);
+
+            if (framework == null)


.Parse will return NuGetFramework.UnsupportedFramework for unknown frameworks, it will never return null.

emgarten · 2017-10-09T19:10:08Z

src/NuGet.Core/NuGet.Build.Tasks/AssignReferencePropertiesTask.cs

+
+            if (nearestNuGetFramework != null)
+            {
+                if (TryConvertItemMetadataToBool(project, "HasSingleTargetFramework", out var singleTargetFramework) && singleTargetFramework)


if (MSBuildStringUtility.IsTrue(project.GetMetadata("HasSingleTargetFramework")))

I think this could be simplified to the above, it should only need to check if this value is true otherwise is false. I might be missing something here though, I don't see where the 2nd bool to see if the metadata was there at all comes into play here.

emgarten · 2017-10-09T19:13:02Z

src/NuGet.Core/NuGet.Build.Tasks/AssignReferencePropertiesTask.cs

+                Log.LogError(string.Format(Strings.NoCompatibleTargetFramework, CurrentProjectTargetFramework, string.Join("; ", possibleNuGetFrameworks)));
+            }
+
+            return itemWithProperties;


nit: log output params?

dsplaisted · 2017-10-09T23:53:52Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

@@ -81,6 +81,7 @@ Copyright (c) .NET Foundation. All rights reserved.
  <UsingTask TaskName="NuGet.Build.Tasks.GetRestoreSolutionProjectsTask" AssemblyFile="$(RestoreTaskAssemblyFile)" />
  <UsingTask TaskName="NuGet.Build.Tasks.GetRestoreSettingsTask" AssemblyFile="$(RestoreTaskAssemblyFile)" />
  <UsingTask TaskName="NuGet.Build.Tasks.WarnForInvalidProjectsTask" AssemblyFile="$(RestoreTaskAssemblyFile)" />
+  <UsingTask TaskName="NuGet.Build.Tasks.AssignReferenceProperties" AssemblyFile="$(RestoreTaskAssemblyFile)" />


This needs to be updated to the new task name with Task on the end.

davidfowl · 2017-10-10T18:13:03Z

src/NuGet.Core/NuGet.Build.Tasks/AssignReferencePropertiesTask.cs

+                return false;
+            }
+
+            AssignedProjects = AnnotatedProjectReferences.Select(project => AssignPropertiesForSingleReference(project, frameworkToMatch)).ToArray();


Allocations?

nguerrera · 2017-10-10T18:50:33Z

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

@@ -117,6 +117,9 @@
  <resheader name="writer">
    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
  </resheader>
+  <data name="NoCompatibleTargetFramework" xml:space="preserve">
+    <value>Cannot find a compatible framework for '{0}' in the following frameworks: {1}.</value>


We should use the same error message as before here:

https://github.com/dotnet/sdk/blob/d7730b73a5bea6c1e7f5d178711322082c93d96e/src/Tasks/Common/Resources/Strings.resx#L124

emgarten · 2017-10-10T19:31:31Z

@panopticoncentral Would it make more sense to have a task that only finds the nearest framework from a set of strings? Or would it impact perf to have to make multiple calls to this task instead?

I'm fine with this change overall, my thought is just that if there was a task that just found the nearest compatible framework it could save adding another task in the future.

nguerrera · 2017-10-10T19:30:57Z

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

@@ -118,7 +118,7 @@
    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
  </resheader>
  <data name="NoCompatibleTargetFramework" xml:space="preserve">
-    <value>Cannot find a compatible framework for '{0}' in the following frameworks: {1}.</value>
+    <value>Project '{0}' targets '{2}'. It cannot be referenced by a project that targets '{1}'.</value>


Looks like the placeholder ordering is different here than the call site.

I reordered the callsite, so the message is:
Project '{0}' targets '{2}'. It cannot be referenced by a project that targets '{1}'.
and the callsite is:
string.Format(Strings.NoCompatibleTargetFramework, project.ItemSpec, CurrentProjectTargetFramework, targetFrameworks)

It looked correct in the unit tests.

You're right, I read wrong.

panopticoncentral · 2017-10-10T19:37:43Z

@emgarten We did talk about that but we felt it was better performance to do it all at once.

AndyGerlicher · 2017-10-10T22:15:29Z

Looks good. Verified works in my testing.

AndyGerlicher and others added 3 commits October 5, 2017 13:28

Add AssignReferenceProperties Task

ace7dbf

Add task to NuGet.targets

bd1338c

Update code & add tests

9b63e4f

dnfclas added the cla-already-signed label Oct 6, 2017

AndyGerlicher mentioned this pull request Oct 8, 2017

Double Evaluation Fix dotnet/msbuild#2595

Merged

nguerrera reviewed Oct 9, 2017

View reviewed changes

nguerrera suggested changes Oct 9, 2017

View reviewed changes

emgarten suggested changes Oct 9, 2017

View reviewed changes

emgarten reviewed Oct 9, 2017

View reviewed changes

dsplaisted reviewed Oct 9, 2017

View reviewed changes

Respond to comments

5b98244

davidfowl reviewed Oct 10, 2017

View reviewed changes

nguerrera reviewed Oct 10, 2017

View reviewed changes

Update based on discussion + comments

2841fd2

nguerrera reviewed Oct 10, 2017

View reviewed changes

nguerrera approved these changes Oct 10, 2017

View reviewed changes

Rename task based on update

cf35571

emgarten approved these changes Oct 10, 2017

View reviewed changes

AndyGerlicher approved these changes Oct 10, 2017

View reviewed changes

emgarten merged commit dda0205 into NuGet:dev Oct 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AssignReferenceProperties task for MSBuild #1737

Add AssignReferenceProperties task for MSBuild #1737

panopticoncentral commented Oct 6, 2017

dnfclas commented Oct 6, 2017

nguerrera Oct 9, 2017

emgarten Oct 9, 2017

nguerrera Oct 9, 2017

panopticoncentral Oct 9, 2017

nguerrera Oct 9, 2017

nguerrera Oct 9, 2017

nguerrera left a comment

emgarten left a comment

emgarten Oct 9, 2017

emgarten Oct 9, 2017

panopticoncentral Oct 10, 2017

emgarten Oct 9, 2017

emgarten Oct 9, 2017

emgarten Oct 9, 2017

dsplaisted Oct 9, 2017

davidfowl Oct 10, 2017

nguerrera Oct 10, 2017

emgarten commented Oct 10, 2017 •

edited

nguerrera Oct 10, 2017

panopticoncentral Oct 10, 2017

nguerrera Oct 10, 2017

panopticoncentral commented Oct 10, 2017

AndyGerlicher commented Oct 10, 2017

Add AssignReferenceProperties task for MSBuild #1737

Add AssignReferenceProperties task for MSBuild #1737

Conversation

panopticoncentral commented Oct 6, 2017

Bug

Fix

Testing/Validation

dnfclas commented Oct 6, 2017

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

nguerrera left a comment

Choose a reason for hiding this comment

emgarten 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

emgarten commented Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panopticoncentral commented Oct 10, 2017

AndyGerlicher commented Oct 10, 2017

emgarten commented Oct 10, 2017 •

edited