Skip import of Microsoft.NuGet.props and Microsoft.NuGet.targets #870

natidea · 2017-02-15T09:37:26Z

Fixes dotnet/project-system#1474 and dotnet/project-system#1508

Customer scenario

Customers restoring projects with reference to certain packages (e.g. System.ValueTuple) will see hundreds of downgrade warnings, when they restore from Visual Studio. However they don't see those same warnings when restoring from the CLI.

The root cause is that the VS targets automatically import Microsoft.NuGet.targets, which contains legacy properties and targets associated with project.json. This targets file is specified as an automatic "ImportAfter" Microsoft.Common.targets for all project types. Other errors are sometimes reported because of the inclusion of this file (dotnet/project-system#1508). The downgrade warnings occur because more RIDs are restored than are specified because of this property in Microsoft.NuGet.targets:

<RuntimeIdentifiers>win;win-x86;win-x64</RuntimeIdentifiers>

The CLI dropped this file from their version of ImportAfter, however VS cannot drop it since it is used by other project types. This change adds some conditions allowing us to skip importing this file in .NET Core projects.

There are two parts to this fix:

a VS side change which adds conditions to the imports: PR 57278
this SDK PR sets the properties for .NET Core projects

Bugs this fixes:

dotnet/project-system#1474 and dotnet/project-system#1508

Risk

Low. The risk may come from us accidentally depending on properties defined in Microsoft.NuGet.targets but all relevant properties have already been copied over to our targets. Furthermore the CLI has been operating without a dependency on this for some time so this ensures parity between the VS and CLI experience.

Performance impact

None

Is this a regression from a previous update?

No

How was the bug found?

Dogfooding

/cc @srivatsn @dsplaisted @nguerrera

…ntifier is not specified

natidea · 2017-02-15T09:39:54Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildALibrary.cs

@@ -577,5 +577,15 @@ public void It_passes_ridless_target_to_compiler()
                .Should()
                .Pass();
        }
+
+        [Fact]
+        public void It_restores_only_ridless_tfm()


Note this test passes even without this fix because it uses dotnet.exe which already excludes Microsoft.NuGet.targets. But this seems like a good scenario to ensure we don't regress.

I think you'll get coverage from the full msbuild runs in Jenkins.

I think you'll get coverage from the full msbuild runs in Jenkins.

Yes, if this isn't failing in the full MSBuild Jenkins run, then I think it's not testing what we think it is (since the corresponding change to the NuGet targets isn't on the Jevkins machine). I'd like us to understand why this is the case.

You can run tests against full MSBuild locally with build -FullMSBuild.

build -FullMSBuild did not fail.

Does "Full MSBuild" mean the sdk and targets under %VSINSTALLDIR% or under .dotnet_cli. I assume its under .dotnet_cli. In this case, these are the same "ImportAfter" targets used by the CLI i.e. they have already removed Microsoft.NuGet.targets (see .dotnet_cli\sdk\1.0.0-rc4-004771\15.0\Microsoft.Common.targets\ImportAfter\Microsoft.NuGet.ImportAfter.targets)

I inspected the project.assets.json generated for the test under bin\Debug\Tests, and it only contains one (ridless) targets entry, so the test is correctly passing, but would fail if we were using the MSBuild under %VSINSTALLDIR%.

Full msbuild means the one in VS. Pasting my comment to this thread as well:

I assume it's not failing because the Jenkins machines won't have the corresponding change in NuGet.targets that adds a condition to skip import with these properties. When we move Jenkins to a build with the VS side change then it'll start failing

OK figured it out. For some reason, the multiple rid problem only repros on apps targeting netcoreapp1.0, not on libraries targeting netstandard1.5. I guess some combination of the conditions in Microsoft.NuGet.targets is only affecting these projects, but its not obvious which ones. (To confirm, New NetStandard Library -> Add System.ValueTuple, does not produce any warnings.)

I've moved the test to GivenThatWeWantToBuildANetCoreApp and modified it to check values from the HelloWorld app. Now build passes, but build -FullMSBuild fails. I'll push this change to confirm Jenkins fails, then disable the test until we update Jenkins.

nguerrera · 2017-02-15T12:06:19Z

The downgrade warnings occur because more RIDs are restored than are specified because of this property in Microsoft.NuGet.targets:

So the downgrades will still occur for projects that add RIDs themselves? If so, this only lessens the case where you'll see them and we still haven't gotten to the root of the issue. I thought there was actually a problem with the authoring of packages like System.ValueTuple.

I just needed to separately add default RIDs to restore of multitargeting project for #847, so if I've understood, the same behavior would show up in that case (only deterministically in both VS and CLI).

srivatsn · 2017-02-15T15:40:45Z

Yes this only lessens the situations where this happens. The ValueTuple package itself is being fixed and #821 tracks doing something about the downgrade warnings for other packages (not sure what).

davidfowl · 2017-02-15T15:42:57Z

Wow this will fix the 5000 warnings we kept seeing in corefxlab! /cc @pakrym

pakrym · 2017-02-15T16:17:09Z

@davidfowl those were fixed by dotnet/corefxlab#1218 but this fixes the root cause

natidea · 2017-02-15T16:46:16Z

@dotnet-bot test this please

srivatsn · 2017-02-15T17:11:25Z

Tagging @MattGertz for approval.

dsplaisted · 2017-02-15T22:25:28Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildALibrary.cs

@@ -577,5 +577,15 @@ public void It_passes_ridless_target_to_compiler()
                .Should()
                .Pass();
        }
+
+        [Fact]
+        public void It_restores_only_ridless_tfm()


I think you'll get coverage from the full msbuild runs in Jenkins.

Yes, if this isn't failing in the full MSBuild Jenkins run, then I think it's not testing what we think it is (since the corresponding change to the NuGet targets isn't on the Jevkins machine). I'd like us to understand why this is the case.

You can run tests against full MSBuild locally with build -FullMSBuild.

srivatsn · 2017-02-15T22:48:53Z

I assume it's not failing because the Jenkins machines won't have the corresponding change in NuGet.targets that adds a condition to skip import with these properties. When we move Jenkins to a build with the VS side change then it'll start failing

natidea · 2017-02-15T22:53:08Z

@srivatsn its the other way round. The test expects only one target definition. If we were running with MSBuild under %VSINSTALLDIR% we would get 3 target definitions, until we apply the VS-side change.

srivatsn · 2017-02-15T22:55:24Z

Ah yes.. then I agree with Daniel. It's fishy that it isn't failing with the full msbuild.

…of NETStandard library

natidea · 2017-02-16T00:38:12Z

Jenkins FullFramework tests failed as expected:
https://ci.dot.net/job/dotnet_sdk/job/master/job/debug_windows_nt_fullframework_prtest/35/
https://ci.dot.net/job/dotnet_sdk/job/master/job/release_windows_nt_fullframework_prtest/35/

…eapps dotnet#874

dsplaisted

LGTM. You could disable the test only when running on full framework, as we do here, for example.

…eapps when UsingFullFrameworkMSBuild dotnet#874

natidea · 2017-02-16T01:19:26Z

Approved to merge via 385238

…tAfter props/targets dotnet/sdk#870 dotnet/project-system#1474 dotnet/project-system#1508

…0190811.2 (dotnet#870) - Microsoft.AspNetCore.Mvc.Analyzers - 3.0.0-preview9.19411.2 - Microsoft.AspNetCore.Mvc.Api.Analyzers - 3.0.0-preview9.19411.2 - Microsoft.AspNetCore.Analyzers - 3.0.0-preview9.19411.2 - Microsoft.AspNetCore.Components.Analyzers - 3.0.0-preview9.19411.2

natidea added 2 commits February 14, 2017 20:03

Skip import of Microsoft.NuGet.props and Microsoft.NuGet.targets

bdfeb0f

Add test that only ridless tfm is restored by default when RuntimeIde…

87883ec

…ntifier is not specified

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

natidea commented Feb 15, 2017

View reviewed changes

nguerrera mentioned this pull request Feb 15, 2017

Remove requirement for specifying a RID in desktop exe projects #847

Merged

srivatsn approved these changes Feb 15, 2017

View reviewed changes

MattGertz added the Pending Shiproom Approval label Feb 15, 2017

pakrym mentioned this pull request Feb 15, 2017

NuGet Restore + VS performance / scalability on solutions with warnings (was: corfxlab repro has some issues with Solution Open and Restore) NuGet/Home#4617

Closed

nguerrera approved these changes Feb 15, 2017

View reviewed changes

dsplaisted requested changes Feb 15, 2017

View reviewed changes

Move test for restoring only ridless tfm to build NETCoreApp instead …

e1fa677

…of NETStandard library

Disable test that msbuild only restores one RID by default for netcor…

8696076

…eapps dotnet#874

dsplaisted approved these changes Feb 16, 2017

View reviewed changes

Disable test that msbuild only restores one RID by default for netcor…

22fcace

…eapps when UsingFullFrameworkMSBuild dotnet#874

natidea mentioned this pull request Feb 16, 2017

Reenable test that msbuild only restores one RID by default for netcoreapps #874

Closed

natidea added the Approved to merge label Feb 16, 2017

natidea merged commit 67783a8 into dotnet:master Feb 16, 2017

natidea added a commit to natidea/NuGet.BuildTasks that referenced this pull request Feb 16, 2017

Add skip imports conditions to Microsoft.NuGet ImportBefore and Impor…

54b95c9

…tAfter props/targets dotnet/sdk#870 dotnet/project-system#1474 dotnet/project-system#1508

natidea mentioned this pull request Feb 16, 2017

Add skip imports conditions to ImportBefore and ImportAfter props/targets dotnet/NuGet.BuildTasks#28

Merged

natidea mentioned this pull request Feb 16, 2017

Old legacy NuGet targets are running during builds causing errors/warnings dotnet/project-system#1508

Closed

emgarten pushed a commit to dotnet/NuGet.BuildTasks that referenced this pull request Feb 16, 2017

Add skip imports conditions to Microsoft.NuGet ImportBefore and Impor…

5a8c149

…tAfter props/targets dotnet/sdk#870 dotnet/project-system#1474 dotnet/project-system#1508

Skip import of Microsoft.NuGet.props and Microsoft.NuGet.targets #870

Skip import of Microsoft.NuGet.props and Microsoft.NuGet.targets #870

Uh oh!

Conversation

natidea commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natidea Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

nguerrera Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

dsplaisted Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

natidea Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

srivatsn Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

natidea Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

nguerrera commented Feb 15, 2017

Uh oh!

srivatsn commented Feb 15, 2017

Uh oh!

davidfowl commented Feb 15, 2017

Uh oh!

pakrym commented Feb 15, 2017

Uh oh!

natidea commented Feb 15, 2017

Uh oh!

srivatsn commented Feb 15, 2017

Uh oh!

dsplaisted Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

srivatsn commented Feb 15, 2017

Uh oh!

natidea commented Feb 15, 2017

Uh oh!

srivatsn commented Feb 15, 2017

Uh oh!

natidea commented Feb 16, 2017

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

natidea commented Feb 16, 2017

Uh oh!

Uh oh!

natidea commented Feb 15, 2017 •

edited

Loading