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

Adding warning for P2P reference with ATF in GetReferenceNearestTargetFrameworkTask #2366

Merged
merged 4 commits into from
Jul 27, 2018

Conversation

mishra14
Copy link
Contributor

Bug

Fixes: NuGet/Home#7137
Regression: No

Fix

Details: As part of #1983 we enabled msbuild to check fallback frameworks while checking compatibility between 2 projects. However, AssetTargetFallback based compatibility in packages logs warning. This PR adds a warning in GetReferenceNearestTargetFrameworkTask which is called on build time by msbuild.

Testing/Validation

Tests Added: Yes
Validation done: manual commandline

@mishra14
Copy link
Contributor Author

cc @rrelyea

@jainaashish
Copy link
Contributor

As discussed, need to enable this for restore inside VS as well. GetReferenceNearestTargetFrameworkTask task doesn't get executed inside VS.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine in general.

Verify the VS experience etc.

@@ -13,6 +13,7 @@ public class GetReferenceNearestTargetFrameworkTaskTest
{
private const int DEBUG_MESSAGE_COUNT_INPUT = 3;
private const int DEBUG_MESSAGE_COUNT_INPUT_OUTPUT = DEBUG_MESSAGE_COUNT_INPUT + 1;
//"Project '' was restored using '.NETFramework,Version=v4.6' instead of the project target framework '.NETCoreApp,Version=v2.0'. This project may not be fully compatible with your project."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this anymore.

@@ -64,11 +66,12 @@ public override bool Execute()
return !Log.HasLoggedErrors;
}

var frameworksToMatch = new List<NuGetFramework>();
var fallbackNuGetFrameworks = new List<NuGetFramework>();
NuGetFramework projectNuGetFramework;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare it in the out statement?

@mishra14
Copy link
Contributor Author

@jainaashish I discussed this with @rainersigwald and GetReferenceNearestTargetFrameworkTask gets invoked in VS as well. This change will log warnings in VS as well.

@@ -117,6 +117,12 @@
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="ImportsFallbackWarning" xml:space="preserve">
<value>Project '{0}' was restored using '{1}' instead of the project target framework '{2}'. This project may not be fully compatible with your project.</value>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not too clear about the warning message for couple of reasons:

  1. This is raised as part of build, not restore so may be more appropriate to use resolve instead of restore.
  2. Host project should also be mentioned otherwise it will be pain to relate it in command line.

May be something like
Project {0} P2P reference {1} resolved using {2} instead of the project target framework {3}. This.......... {0} - Host Project {1} - P2P reference {2} - fallback tfm {3} - original project tfm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or

Project '{0}' is referenced using target framework '{1}' instead of this project's target framework '{2}'. This project may not be fully compatible with your project.

?

warning.ProjectPath = CurrentProjectName;

// log NU1702 for ATF on project reference
logger.Log(warning);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to understand how are we skipping this warning when customer has no warn this? or will it be MSBuild who will take care of that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as this code is unique to this situation, MSBuild now has a general-purpose property MSBuildWarningsAsMessages that can suppress messages by code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msbuild now has support for thhis - dotnet/msbuild#1928

<MSBuildWarningsAsMessages>NU1702</MSBuildWarningsAsMessages>

Copy link
Contributor

@jainaashish jainaashish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just a small suggestion to improve warning message. Current message seemed lil confusing.

@mishra14 mishra14 merged commit 8abf12d into dev Jul 27, 2018
@mishra14 mishra14 deleted the dev-anmishr-p2patfwarning branch July 27, 2018 00:33
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.

4 participants