Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add skip imports conditions to ImportBefore and ImportAfter props/targets #28

Merged
merged 1 commit into from
Feb 16, 2017

Conversation

natidea
Copy link
Contributor

@natidea natidea commented Feb 16, 2017

Related PRs/Issues

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

dotnet/sdk#870
VS change: PR 57278

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 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:

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 @emgarten

@dnfclas
Copy link

dnfclas commented Feb 16, 2017

Hi @natidea, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@emgarten emgarten merged commit 5a8c149 into dotnet:dev Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants