Support for processing metaproj references from a solution file #1259

emgarten · 2017-03-23T07:26:57Z

This change adds a task to modify project references coming from a solution file. When project build order is used the solution file read by restore contains links to .metaproj files instead of the actual projects. The actual project can be found by removing the metaproj extension.

Cleaning up _SplitProjectReferencesByFileExistence, the outputs from this are not used since ProjectReference is used direcetly. There is no reason to depend on this external target.

Fixes NuGet/Home#4869
Fixes NuGet/Home#4578

Tracking for the integration tests around this is here: NuGet/Home#4875 (post migration)

alpaix · 2017-03-23T17:12:17Z

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

+        {
+            // Log inputs
+            var log = new MSBuildLogger(Log);
+            log.LogDebug($"(in) ProjectReferences '{string.Join(";", ProjectReferences.Select(p => p.ItemSpec))}'");


Can ItemSpec be null or empty?

It can't be null

Then it's inconsistent with line 50 where you check the same value for null.

It checks for empty also

alpaix · 2017-03-23T17:13:41Z

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

@@ -0,0 +1,74 @@
+using System;


I think we need to decide whether header is needed or not and be consistent about it.

We should remove it from all files to reduce the daily overhead of this or automate it. Currently the tool from .NET that can do this doesn't work on PackageReference or project.json projects.

alpaix · 2017-03-23T17:14:18Z

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

+
+namespace NuGet.Build.Tasks
+{
+    public class GetRestoreSolutionProjectsTask : Task


Add xml-doc

alpaix · 2017-03-23T17:15:31Z

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

+
+            // Filter obvious duplicates without considering OS case sensitivity.
+            // This will be filtered further when creating the spec.
+            var seen = new HashSet<string>(StringComparer.Ordinal);


Case-sensitive on purpose?

Looks like this can be cleaned up

alpaix · 2017-03-23T17:16:42Z

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

+            // Filter obvious duplicates without considering OS case sensitivity.
+            // This will be filtered further when creating the spec.
+            var seen = new HashSet<string>(StringComparer.Ordinal);
+            var parentDirectory = Path.GetDirectoryName(SolutionFilePath);


Can this fail if invalid string was provided for SolutionFilePath?

Can the path be relative?

It is guaranteed to be the full path https://github.com/NuGet/NuGet.Client/blob/5fd37322b4f4c73b81b4c33b0c7628852e9da900/src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets#L156

It's not guaranteed though the target won't change in future or this task won't be called from within different target. All inputs should be validated regardless.

alpaix · 2017-03-23T17:17:45Z

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

+
+            foreach (var project in ProjectReferences)
+            {
+                if (string.IsNullOrEmpty(project.ItemSpec))


Should it fail or warn on invalid value?

It should not be, but if it is the error/warning would be very unhelpful since it would not refer to anything. This protects against that.

alpaix · 2017-03-23T17:20:18Z

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

+                    continue;
+                }
+
+                var projectPath = Path.Combine(parentDirectory, project.ItemSpec);


Normalize the path in case relative project path provided.

I'll update this, however the consumer of this is MSBuild which handles this fine internally.

alpaix

Address the feedback. Add unit-tests for new class.

rohit21agrawal · 2017-03-23T18:21:05Z

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

+        public ITaskItem[] ProjectReferences { get; set; }
+
+        /// <summary>
+        /// Root project path used for resolving the absolute path.


root project path? or solution file path?

Root metaproj from the solution.. I can make this more clear :)

rohit21agrawal · 2017-03-23T18:22:44Z

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

+                if (projectPath.EndsWith(MetaProjExtension, StringComparison.OrdinalIgnoreCase))
+                {
+                    // Remove .metaproj from the path.
+                    projectPath = projectPath.Substring(0, projectPath.Length - MetaProjExtension.Length);


can you instead do Path.Combine(Path.GetDirectoryName(file), Path.GetFileNameWithoutExtension(file) )

I did that initially but simplified to this. We know here exactly what needs to happen, an extra helper isn't needed.

Either way would work fine though

i'd prefer being more verbose and call the helpers but your call.

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

This change adds a task to modify project references coming from a solution file. When project build order is used the solution file read by restore contains links to .metaproj files instead of the actual projects. The actual project can be found by removing the metaproj extension. Cleaning up _SplitProjectReferencesByFileExistence, the outputs from this are not used since ProjectReference is used direcetly. There is no reason to depend on this external target. Fixes NuGet/Home#4869 Fixes NuGet/Home#4578

emgarten · 2017-03-23T19:08:52Z

@alpaix unit tests are tracked in NuGet/Home#4875

natemcmaster · 2017-03-23T20:28:53Z

👏

dnfclas added the cla-already-signed label Mar 23, 2017

emgarten requested review from rohit21agrawal, nkolev92, zhili1208, alpaix, mishra14 and jainaashish March 23, 2017 07:27

alpaix reviewed Mar 23, 2017

View reviewed changes

alpaix suggested changes Mar 23, 2017

View reviewed changes

emgarten force-pushed the dev-emgarten-metaproj branch from 5fd3732 to c2d4353 Compare March 23, 2017 18:18

rohit21agrawal reviewed Mar 23, 2017

View reviewed changes

rohit21agrawal approved these changes Mar 23, 2017

View reviewed changes

emgarten force-pushed the dev-emgarten-metaproj branch from c2d4353 to af6eff5 Compare March 23, 2017 18:28

alpaix approved these changes Mar 23, 2017

View reviewed changes

emgarten closed this Mar 23, 2017

emgarten deleted the dev-emgarten-metaproj branch March 23, 2017 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for processing metaproj references from a solution file #1259

Support for processing metaproj references from a solution file #1259

emgarten commented Mar 23, 2017

alpaix Mar 23, 2017

emgarten Mar 23, 2017

alpaix Mar 23, 2017

emgarten Mar 23, 2017

alpaix Mar 23, 2017

emgarten Mar 23, 2017

alpaix Mar 23, 2017

alpaix Mar 23, 2017

emgarten Mar 23, 2017

alpaix Mar 23, 2017

alpaix Mar 23, 2017

emgarten Mar 23, 2017

alpaix Mar 23, 2017

alpaix Mar 23, 2017

emgarten Mar 23, 2017 •

edited

Loading

alpaix Mar 23, 2017

emgarten Mar 23, 2017 •

edited

Loading

alpaix left a comment

rohit21agrawal Mar 23, 2017

emgarten Mar 23, 2017

rohit21agrawal Mar 23, 2017

emgarten Mar 23, 2017

emgarten Mar 23, 2017

rohit21agrawal Mar 23, 2017

emgarten commented Mar 23, 2017

natemcmaster commented Mar 23, 2017

Support for processing metaproj references from a solution file #1259

Support for processing metaproj references from a solution file #1259

Conversation

emgarten commented Mar 23, 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

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 Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emgarten Mar 23, 2017 • edited Loading

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

emgarten commented Mar 23, 2017

natemcmaster commented Mar 23, 2017

emgarten Mar 23, 2017 •

edited

Loading

emgarten Mar 23, 2017 •

edited

Loading