In Legacy Package Ref case, use only per tfm dependencies #1652

nkolev92 · 2017-08-15T23:08:44Z

This PR partially addresses NuGet/Home#5444
To fully fix the above issue, I will create another PR which specifically targets the way we calculate the hash for no-op comparison.

In the case of Legacy Package Reference our code has been used inconsistently where in most cases we used top level dependencies, while we built the package spec with both top level/ per tfm in VS.
In the exe only top level dependencies were used.

Now I have changed all of those to use Per TFM dependencies only. This will make the dg specs created among VS and exe "almost" the same.

Scenario Change
This changes the dependency flowing behavior when projects are incompatible.
say, net45 project references a net46 projects, before this change, we'd throw an incompatibility error during restore but also flow the dependencies. Now they won't flow since the frameworks are incompatible.
This is not a big concern cause no one could rely on this, because the scenario is not buildable

rohit21agrawal · 2017-08-16T18:17:51Z

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

@@ -664,12 +664,6 @@ private void PopulateProjectAndPackageReferences(PackageBuilder packageBuilder,
            // From the package spec, we know the direct package dependencies of this project.
            foreach (var framework in assetsFile.PackageSpec.TargetFrameworks)
            {
-                // First, add each of the generic package dependencies to the framework-specific list.


investigate if this is really necessary.

rohit21agrawal · 2017-08-16T18:20:08Z

src/NuGet.Core/NuGet.ProjectModel/PackageSpecOperations.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// Copyright (c) .NET Foundation. All rights reserved.


revert this one.

rohit21agrawal · 2017-08-16T18:20:57Z

src/NuGet.Core/NuGet.ProjectModel/PackageSpecOperations.cs

@@ -13,8 +13,11 @@ namespace NuGet.ProjectModel
 {
    public static class PackageSpecOperations
    {
-        public static void AddOrUpdateDependency(PackageSpec spec, PackageDependency dependency)


we are removing a public api that has potential to break sdk/cli when they update.
i would prefer if you added an overload and make this method call the overloaded method.

I checked that, and it's not used.
I'd normally prefer to cleanup APIs that aren't super important for other users, but I'll revert this.

rohit21agrawal · 2017-08-16T18:22:18Z

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

-    <!-- Only return values for NETCore PackageReference projects -->
-    <ItemGroup Condition=" '$(RestoreProjectStyle)' == 'PackageReference' AND '$(_RestoreTargetFrameworkItemsHasValues)' == 'true' ">
+    <!-- Only return values for PackageReference projects -->
+    <ItemGroup Condition=" '$(RestoreProjectStyle)' == 'PackageReference' AND '$(RestoreProjectStyle)' == 'PackageReference' ">


why do you need the same condition twice?

We don't lol.
Nice catch.
I misread it completely.

nkolev92 · 2017-08-16T22:36:14Z

🔔 @rohit21agrawal @jainaashish @mishra14 @zhili1208 @PatoBeltran

rohit21agrawal · 2017-08-16T22:41:34Z

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

@@ -427,12 +427,8 @@ Copyright (c) .NET Foundation. All rights reserved.
    DependsOnTargets="_GetRestoreProjectStyle;_GetRestoreTargetFrameworksOutput"
    Returns="@(_RestoreTargetFrameworkItems)">

-    <PropertyGroup>
-      <_RestoreTargetFrameworkItemsHasValues Condition=" '$(TargetFramework)' != '' OR '$(TargetFrameworks)' != '' ">true</_RestoreTargetFrameworkItemsHasValues>


this isn't used anywhere?

It's just a local one used in the below item group as a condition. It was used to restrict setting the target frameworks to net core only.
By changing it now the target frameworks are set in the legacy case as well.

rohit21agrawal

just one minor question, otherwise looks good!

jainaashish · 2017-08-17T00:43:01Z

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs

@@ -2381,7 +2381,14 @@ var projectUniqueNamesForBuildIntToUpdate
                }
                else if (action.NuGetProjectActionType == NuGetProjectActionType.Install)
                {
-                    PackageSpecOperations.AddOrUpdateDependency(updatedPackageSpec, action.PackageIdentity);
+                    if (updatedPackageSpec.RestoreMetadata.ProjectStyle == ProjectStyle.ProjectJson)


Would it make more sense if you check for PackageReference and pass tfms. And else case use the old call. That will keep the existing behavior and only change PR

jainaashish

Small correction to be consistent with existing behavior. Otherwise looks good.

nkolev92 · 2017-08-17T02:17:11Z

@jainaashish 🔔

jainaashish

LGTM

nkolev92 · 2017-08-18T20:18:01Z

This PR currently has some failing EndToEnd tests due to the following issues:
NuGet/Home#5778 and NuGet/Home#5777
Once the templates have been fixed 5778, this should be safe to merge in.

Depending on the decision in NuGet/Home#5777 there might need to improvements in this, but that's a separate issue.

…he legacy case

emgarten

Make this change in MSBuildRestoreUtility.AddPackageReferences

…Build related perf concerns

nkolev92 · 2017-09-07T22:06:19Z

@emgarten 🔔

emgarten · 2017-09-07T23:09:34Z

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

@@ -330,7 +330,6 @@ private static PackageReference ToPackageReference(LibraryDependency library, Nu
                Owners = new string[] { },
                Tags = new string[] { },
                ContentFiles = new string[] { },
-                Dependencies = packageReferences,
                FilePath = _projectFullPath,


Where do the package dependencies get populated now?

emgarten

🚀

dnfclas added the cla-already-signed label Aug 15, 2017

nkolev92 requested review from emgarten, rohit21agrawal, PatoBeltran, zhili1208, mishra14 and jainaashish August 15, 2017 23:18

nkolev92 force-pushed the dev-nkolev92-fixlegacynoop branch from 64ddc9d to fdf51d5 Compare August 16, 2017 16:31

rohit21agrawal reviewed Aug 16, 2017

View reviewed changes

rohit21agrawal approved these changes Aug 16, 2017

View reviewed changes

jainaashish reviewed Aug 17, 2017

View reviewed changes

jainaashish suggested changes Aug 17, 2017

View reviewed changes

nkolev92 mentioned this pull request Aug 17, 2017

Change the way that the hash is calculated #1659

Merged

jainaashish approved these changes Aug 17, 2017

View reviewed changes

nkolev92 mentioned this pull request Aug 18, 2017

E2E tests have incompatible templates for LSL tests NuGet/Home#5778

Closed

2 tasks

nkolev92 mentioned this pull request Aug 18, 2017

Change template to net46, add non lsl tests as well #1664

Merged

nkolev92 force-pushed the dev-nkolev92-fixlegacynoop branch from 02f3f86 to fb755c9 Compare September 5, 2017 23:57

nkolev92 added 6 commits September 6, 2017 14:47

init work

7d77e16

cleanup

9a4ebb8

cleanup commments

2976f3f

fix more code

2716fe8

fix more code

e4b553e

fix compile error

a779f61

nkolev92 added 14 commits September 6, 2017 14:47

fix test

3ac06ad

cleanup

21fe8e9

Change the dg spec built by MSBuild/nuget.exe to include per tfm in t…

bdf7a66

…he legacy case

bom removal

a515e09

cleanup

98ccd61

revert migrator

765137f

revert spec val utility

51fe0a6

only have a pack ref check

cee3866

remove extra condition

a2b6e0d

revert pack task logic code

b13634b

revert lock file simple change

d554b48

dont remove api as it might be used by SDK/CLI

69d370e

revert package spec operations cleanup

f369ebd

change check to only add per tfm to pack ref projects

bd9c195

nkolev92 force-pushed the dev-nkolev92-fixlegacynoop branch from fb755c9 to bd9c195 Compare September 6, 2017 21:48

emgarten suggested changes Sep 7, 2017

View reviewed changes

set the legacy pack ref dependencies per tfm in NuGet, to minimize Ms…

4538f84

…Build related perf concerns

emgarten reviewed Sep 7, 2017

View reviewed changes

emgarten approved these changes Sep 7, 2017

View reviewed changes

nkolev92 merged commit f9569ac into dev Sep 8, 2017

nkolev92 deleted the dev-nkolev92-fixlegacynoop branch September 21, 2017 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In Legacy Package Ref case, use only per tfm dependencies #1652

In Legacy Package Ref case, use only per tfm dependencies #1652

nkolev92 commented Aug 15, 2017 •

edited

Loading

rohit21agrawal Aug 16, 2017

rohit21agrawal Aug 16, 2017

rohit21agrawal Aug 16, 2017

nkolev92 Aug 16, 2017

rohit21agrawal Aug 16, 2017

nkolev92 Aug 16, 2017

nkolev92 commented Aug 16, 2017

rohit21agrawal Aug 16, 2017

nkolev92 Aug 16, 2017

rohit21agrawal left a comment

jainaashish Aug 17, 2017

jainaashish left a comment

nkolev92 commented Aug 17, 2017

jainaashish left a comment

nkolev92 commented Aug 18, 2017

emgarten left a comment

nkolev92 commented Sep 7, 2017

emgarten Sep 7, 2017

emgarten Sep 7, 2017

emgarten left a comment

		@@ -1,4 +1,4 @@
		// Copyright (c) .NET Foundation. All rights reserved.
		// Copyright (c) .NET Foundation. All rights reserved.

In Legacy Package Ref case, use only per tfm dependencies #1652

In Legacy Package Ref case, use only per tfm dependencies #1652

Conversation

nkolev92 commented Aug 15, 2017 • 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

nkolev92 commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohit21agrawal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainaashish left a comment

Choose a reason for hiding this comment

nkolev92 commented Aug 17, 2017

jainaashish left a comment

Choose a reason for hiding this comment

nkolev92 commented Aug 18, 2017

emgarten left a comment

Choose a reason for hiding this comment

nkolev92 commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emgarten left a comment

Choose a reason for hiding this comment

nkolev92 commented Aug 15, 2017 •

edited

Loading