Support for ContentFiles in generated props. #1061

emgarten · 2016-12-15T23:51:36Z

Copied the build task's content files logic into NuGet. Restore will now write out contentFile items to props. .PP files will come later.
Added build and buildMultiTargeting to the assets file. This puts all assets in place to simplify restore and removes the need to discover the assets for build separately. This assets will now count for compatibility.
Added buildMultiTargeting, this will replace buildCrossTargeting. Both will exist for now.
Combined RestoreResult and MSBuildRestoreResult. MSBuild is now 1st classed in the restore world. Previously it was not since XProj did not use it.
Added NuGet properties to the props file, this will help package authors writing targets understand what type of project they are in to bridge gaps between packages.config and PackageReference.
Simplified/cleaned up MSBuild targets/props writing for restore.

Fixes NuGet/Home#4098
Fixes NuGet/Home#3928
Fixes NuGet/Home#3683
Fixes NuGet/Home#3399
Fixes NuGet/Home#4116

//cc @alpaix @mishra14 @rohit21agrawal @nkolev92 @jainaashish

Fixes NuGet/Home#4098 Fixes NuGet/Home#3928 Fixes NuGet/Home#3683 Fixes NuGet/Home#4098 Fixes NuGet/Home#3399 Adding tests to verify files are written out on failure

alpaix · 2016-12-16T17:45:43Z

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


-            // Get assets for each framework
-            foreach (var projectFramework in project.TargetFrameworks.Select(f => f.FrameworkName))
+        public static string ReplacePathsWithMacros(string path)


Method's name and logic are quite specific to the MSBuild restore scenario. I'd keep it private.

alpaix · 2016-12-16T19:03:04Z

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

+        /// </summary>
+        public static void AddNuGetProperties(XDocument doc, IEnumerable<string> packageFolders, string repositoryRoot, RestoreOutputType outputType, bool success)
+        {
+            var projectStyle = "Unknown";


var projectStyle = "Unknown"; [](start = 12, length = 29)

Needs to be an enum. And public conversion methods.

I like the enum idea. This will be changed in a different PR to cover NuGet/Home#4047 and several similar issues where these values will now be used across the SDK, NuGet, and packages. This property had a different goal originally.

alpaix · 2016-12-16T20:22:27Z

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

+            bool success)
+        {
+            var firstImport = files.Where(file => file.Content != null)
+                .OrderByDescending(file => file.Path.EndsWith(PropsExtension) ? 1 : 0)


OrderByDescending [](start = 17, length = 17)

Why not just Where?

For project.json there could be only a targets file, or no files that will be written out at all. For PackageReference this will always be props however.

Ah.. That's clever. However I'd add a comment line here.

alpaix · 2016-12-16T20:34:43Z

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

                {
-                    var libraryIdentity = globalGroupEntry.Key;
-                    var buildGroupSet = globalGroupEntry.Value;
+                    // Use a simple string compare to check if the files match


Use a simple string compare to check if the files match [](start = 23, length = 55)

This should be a hash comparison. I'd suggest filing an issue for that.

Good call. This didn't change in this PR, it just moved. But now that contentFiles could potentially make these files larger proper hashing would be nice. NuGet/Home#4120

alpaix · 2016-12-16T20:42:21Z

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

-            // convert graph items to package dependency info list
-            var dependencies = ConvertToPackageDependencyInfo(graph.Flattened);
+            // Add additional conditionals for multi targeting
+            var multiTargetingFromMetadata = (request.Project.RestoreMetadata?.CrossTargeting == true);


CrossTargeting [](start = 79, length = 14)

Any plans to rename this property of ProjectRestoreMetadata as well?

This is only internal so it seemed unneeded for now. There are many fields that still have the old project.json names such as SuppressParent that should be renamed to the new PackageReference equivalent. All of those changes for PackageSpec could be better done in a single change.

alpaix · 2016-12-16T20:42:51Z

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


-            // sort graph nodes by dependencies order
-            var sortedItems = TopologicalSortUtility.SortPackagesByDependencyOrder(dependencies);
+            var isCrossTargeting = multiTargetingFromMetadata


isCrossTargeting [](start = 16, length = 16)

isMultiTargeting?

I can update it, internally it is all the same.

alpaix · 2016-12-16T20:44:24Z

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

+            string propsPath,
+            RestoreOutputType restoreType,
+            ILogger log,
+            CancellationToken token)


token [](start = 30, length = 5)

not needed here

alpaix · 2016-12-16T20:46:21Z

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

+            RestoreRequest request,
+            bool restoreSuccess,
+            ILogger log,
+            CancellationToken token)


token [](start = 30, length = 5)

Not needed. The method is not async

CancellationToken can be used without async. here is wasn't used, it was just kept from the previous result commit method. I'll remove it.

emgarten · 2016-12-16T22:16:14Z

@alpaix updated

alpaix · 2016-12-17T00:37:27Z

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


-                        var targetsGroup = new MSBuildRestoreImportGroup();
+        public static XElement GetProperty(string propertyName, string content)


GetProperty [](start = 31, length = 11)

GenerateProperty would be more consistent

alpaix · 2016-12-17T00:44:14Z

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

+                            GetProperty("RestoreSuccess", success.ToString()),
+                            GetProperty("RestoreTool", "NuGet"),
+                            GetProperty("NuGetPackageRoot", ReplacePathsWithMacros(repositoryRoot)),
+                            GetProperty("NuGetPackageFolders", string.Join(";", packageFolders)),


packageFolders [](start = 80, length = 14)

Noted this is an absolute path. Although it points to the same location as NuGetPackageRoot: C:\Users\alpanov\.nuget\packages\.

Should it be also substituted with a macro?

I considered this also. It would help make this more portable, but at the same time it would then be out of sync with the packageFolders list in project.assets.json. The goal here was to make that list available in MSBuild so that users could search those folders to find where NuGet downloaded the packages and work with them through MSBuild if needed.

For now I am going to leave it as an identical copy of what is in the assets file. I think we need some real user scenarios around this to decide further.

alpaix · 2016-12-17T00:50:23Z

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

+        public static XElement GenerateContentFilesItem(string path, LockFileContentFile item, string packageId, string packageVersion)
+        {
+            var entry = new XElement(Namespace + item.BuildAction.Value,
+                                new XAttribute("Include", path),


path [](start = 58, length = 4)

Beginning of import project path is substituted with the repository root. While the item include path is an absolute path. Looks inconsistent to me.

Good catch, it looks like the ordering is off when applying macros for content files, I'll update it.

emgarten · 2016-12-17T01:15:27Z

@alpaix updated

alpaix · 2016-12-17T01:21:33Z

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

+                            .Select(path => GetImportPath(path, repositoryRoot))
+                            .Select(GenerateImport));
+
+                    targets.AddRange(GetGroupsWithConditions(buildCrossTargetsGroup, isMultiTargeting, CrossTargetingCondition));


targets.AddRange [](start = 20, length = 16)

Same targets/props may be added multiple times, IE one time per each tfm.

Yes, that's by design and the existing behavior. If there is a need to optimize this I think it should be done separate from this. In theory this could combine everything together and combine the conditions, but all of these need to be in dependency order which makes this and combining language flags on content items non-trivial.

Forcing the TFM conditions also avoid scenarios where the user adds a new TFM to $(TargetFrameworks) but hasn't done a restore yet.

If we leave it this way msbuild will keep complaining:

1>C:\Users\alpanov\documents\visual studio 2017\Projects\ConsoleApp6\ConsoleApp6\obj\ConsoleApp6.csproj.nuget.g.props(15,5): warning MSB4011: "C:\Users\alpanov.nuget\packages\alex.contentfiles\1.0.4\buildMultiTargeting\Alex.ContentFiles.props" cannot be imported again. It was already imported at "C:\Users\alpanov\documents\visual studio 2017\Projects\ConsoleApp6\ConsoleApp6\obj\ConsoleApp6.csproj.nuget.g.props (12,5)". This is most likely a build authoring error. This subsequent import will be ignored. [C:\Users\alpanov\documents\visual studio 2017\Projects\ConsoleApp6\ConsoleApp6\ConsoleApp6.csproj]
1>C:\Users\alpanov\documents\visual studio 2017\Projects\ConsoleApp6\ConsoleApp6\obj\ConsoleApp6.csproj.nuget.g.targets(7,5): warning MSB4011: "C:\Users\alpanov.nuget\packages\alex.contentfiles\1.0.4\buildMultiTargeting\Alex.ContentFiles.targets" cannot be imported again. It was already imported at "C:\Users\alpanov\documents\visual studio 2017\Projects\ConsoleApp6\ConsoleApp6\obj\ConsoleApp6.csproj.nuget.g.targets (4,5)". This is most likely a build authoring error. This subsequent import will be ignored. [C:\Users\alpanov\documents\visual studio 2017\Projects\ConsoleApp6\ConsoleApp6\ConsoleApp6.csproj]

alpaix · 2016-12-17T01:25:18Z

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

+                                .Where(e => pkg.Value.Exists())
+                                .OrderBy(e => e.Path, StringComparer.Ordinal)
+                                .Select(e =>
+                                    new Tuple<LockFileTargetLibrary, LockFileContentFile, string>(


Tuple [](start = 40, length = 5)

nit: Tuple.Create(...)

alpaix · 2016-12-17T01:26:51Z

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

+                                    new Tuple<LockFileTargetLibrary, LockFileContentFile, string>(
+                                        item1: pkg.Key,
+                                        item2: e,
+                                        item3: pkg.Value.GetAbsolutePath(GetImportPath(e.Path, repositoryRoot)))))


GetImportPath [](start = 73, length = 13)

Should it be in reverse order:
GetImportPath(GetAbsolutePath(...))?

Fixed in the latest commit. I have no clue how this was compiling even, GetAbsolutePath does not take a string...

alpaix · 2016-12-17T01:28:59Z

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

-            PackageIdentity libraryIdentity,
-            ContentItemGroup buildItems,
-            TargetsAndProps targetsAndProps)
+        private static IEnumerable<MSBuildRestoreItemGroup> GetGroupsWithConditions(


GetGroupsWithConditions [](start = 60, length = 23)

=> GenerateGroupsWithConditions

alpaix

Looks ✨! Your call on eliminating duplicate imports.

dnfclas added the cla-already-signed label Dec 15, 2016

emgarten force-pushed the dev-emgarten-contentFiles2 branch 2 times, most recently from 30710ba to 18bb75f Compare December 16, 2016 07:59

Support for ContentFiles in generated props.

0b91475

Fixes NuGet/Home#4098 Fixes NuGet/Home#3928 Fixes NuGet/Home#3683 Fixes NuGet/Home#4098 Fixes NuGet/Home#3399 Adding tests to verify files are written out on failure

emgarten force-pushed the dev-emgarten-contentFiles2 branch from 18bb75f to 0b91475 Compare December 16, 2016 08:41

alpaix reviewed Dec 16, 2016

View reviewed changes

Test fix

d068ef1

alpaix reviewed Dec 16, 2016

View reviewed changes

Addressing PR feedback

60c5d6f

alpaix reviewed Dec 17, 2016

View reviewed changes

Addressing feedback part 2

b2c4540

alpaix reviewed Dec 17, 2016

View reviewed changes

alpaix approved these changes Dec 17, 2016

View reviewed changes

Addressing feedback

ccb19ca

emgarten closed this Dec 17, 2016

emgarten deleted the dev-emgarten-contentFiles2 branch December 17, 2016 05:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for ContentFiles in generated props. #1061

Support for ContentFiles in generated props. #1061

emgarten commented Dec 15, 2016 •

edited

Loading

alpaix Dec 16, 2016

alpaix Dec 16, 2016 •

edited

Loading

emgarten Dec 16, 2016

alpaix Dec 16, 2016

emgarten Dec 16, 2016

alpaix Dec 16, 2016

alpaix Dec 16, 2016

emgarten Dec 16, 2016

alpaix Dec 16, 2016

emgarten Dec 16, 2016

alpaix Dec 16, 2016

emgarten Dec 16, 2016

alpaix Dec 16, 2016

emgarten Dec 16, 2016

alpaix Dec 16, 2016

emgarten Dec 16, 2016

emgarten commented Dec 16, 2016

alpaix Dec 17, 2016

alpaix Dec 17, 2016

emgarten Dec 17, 2016

alpaix Dec 17, 2016

emgarten Dec 17, 2016

emgarten commented Dec 17, 2016

alpaix Dec 17, 2016

emgarten Dec 17, 2016

emgarten Dec 17, 2016

alpaix Dec 17, 2016

alpaix Dec 17, 2016

alpaix Dec 17, 2016

emgarten Dec 17, 2016

alpaix Dec 17, 2016

alpaix left a comment


		var targetsGroup = new MSBuildRestoreImportGroup();
		public static XElement GetProperty(string propertyName, string content)

Support for ContentFiles in generated props. #1061

Support for ContentFiles in generated props. #1061

Conversation

emgarten commented Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

alpaix Dec 16, 2016 • edited Loading

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

emgarten commented Dec 16, 2016

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 Dec 17, 2016

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

emgarten commented Dec 15, 2016 •

edited

Loading

alpaix Dec 16, 2016 •

edited

Loading