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.

Sign up for GitHub

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

Jump to bottom

Using a ProjectReference which requires AssetTargetFallback to succeed, should warn. #7137

Closed
nguerrera opened this issue Jul 20, 2018 · 15 comments
Closed

Using a ProjectReference which requires AssetTargetFallback to succeed, should warn. #7137

nguerrera opened this issue Jul 20, 2018 · 15 comments
Assignees
@mishra14
Labels
Functionality:Restore Priority:1 High priority issues that must be resolved in the current sprint. Type:Bug
Milestone
5.0

Comments

@nguerrera
Copy link

Microsoft Visual Studio Enterprise 2017 Int Preview (Dogfood)
Version 15.8.0 Preview 5.0 [27913.3.d15.8]
VisualStudio.15.IntPreview/15.8.0-pre.5.0+27913.3.d15.8
Microsoft .NET Framework
Version 4.7.03056

Repro steps

  • Add project reference from .NET Core project to .NET Framework 4.6.1 project
  • Restore / Build

Expected

Very similar warning to this for the PackageReference case

Warning NU1701 Package 'XAct.Wintellect.PowerCollections 0.0.1' was restored using '.NETFramework,Version=v4.6.1' instead of the project target framework '.NETCoreApp,Version=v2.1'. This package may not be fully compatible with your project. ConsoleApp30 C:\Users\nicholg\source\repos\ConsoleApp30\ConsoleApp30\ConsoleApp30.csproj 1

Actual

No warning

This defeats the rationale for why it was deemed OK to make the net461 asset target fallback a default.

cc @rrelyea @mishra14 @rainersigwald @dsplaisted @terrajobst
@nguerrera nguerrera mentioned this issue Jul 20, 2018
Closed
@mishra14 mishra14 added Type:DCR Design Change Request Functionality:Restore labels Jul 21, 2018
@mishra14
Copy link
Contributor

@nguerrera currently, msbuild passes us a project tfm and a set of tfms to check compatibility and we return a bool indicating success or failure. We will need to work with @rrainersigwald to change that if needed.

@rrelyea had scenarios (wpf + uwp) where such P2P references make sense without the need for warnings.

@nguerrera
Copy link
Author

nguerrera commented Jul 21, 2018 •
edited
Loading

You could log a warning from the task. We spent a lot of time talking about this for package refs and the warning was a big part of pulling the trigger on the AssetTargetFallback default. I don't see how it could be any different for projects, but I'm listening.

@nguerrera nguerrera mentioned this issue Jul 21, 2018
Closed
@nguerrera
Copy link
Author

I would like this to be considered for 15.8 because I don't want to end up in a situation where we decide not to make a "breaking change" to add the warning that should always have been there.

@nguerrera
Copy link
Author

cc @DamianEdwards

@terrajobst
Copy link

terrajobst commented Jul 21, 2018 •
edited
Loading

I'm surprised to learn this actually works as I was under the impression that this will fail. We must at the very least issue a warning as @nguerrera suggests.

However, I believe our original design asked for this to be an error. The rationale being that the NuGet case is for references outside your control while P2Ps are expected to be under your control and we want to force developers to convert/modernize from the bottom up. However, I understand that making this an error now would be breaking change. But I don't think silent success here is acceptable behavior.

@nguerrera
Copy link
Author

However, I understand that making this an error now would be breaking change.

This is new in 15.8 so technically hasn't shipped. I think there should be a way to do this with project references, I just don't think it should happen without any diagnostics.

@terrajobst
Copy link

terrajobst commented Jul 21, 2018 •
edited
Loading

This is new in 15.8 so technically hasn't shipped. I think there should be a way to do this with project references, I just don't think it should happen without any diagnostics.

That's fair. Not sure I wholeheartedly agree, but I could live with a warning, consistent with the NuGet experience. But silence is bad.

@mishra14 mishra14 added the Priority:1 High priority issues that must be resolved in the current sprint. label Jul 21, 2018
@mishra14 mishra14 added this to the 4.8 milestone Jul 21, 2018
@mishra14
Copy link
Contributor

tagging @rrelyea for 4.8 visibility.

@mishra14 mishra14 self-assigned this Jul 23, 2018
@rrelyea rrelyea modified the milestones: 4.8, 4.9 Jul 24, 2018
@rrelyea
Copy link
Contributor

rrelyea commented Jul 24, 2018

Given that this isn’t as easy to fall into compared to a package…the user has to go out of their way to add ATF into the referenced project, I think we should be fine in 15.8 with a doc change.

We hope to make this a warning in 15.9.

(sent email too)

@nguerrera
Copy link
Author

nguerrera commented Jul 24, 2018 •
edited
Loading

…the user has to go out of their way to add ATF into the referenced project, I think we should be fine in 15.8 with a doc change

I don't understand this. Look at the repro steps again. What is out of their way? The problem is ATF is default in .NETCore and .NETStandard projects.

@nguerrera
Copy link
Author

This might be a clearer repro for my last point than what is listed:

  1. File -> New Project -> .NET Core -> Console App (.NET Core)
  2. File -> New Project -> Windows Desktop -> Class Library (.NET Framework)
  3. Add reference from (1) to (2)
  4. Restore and build

@rrelyea rrelyea modified the milestones: 4.9, 4.8 Jul 24, 2018
@rrelyea
Copy link
Contributor

rrelyea commented Jul 24, 2018

Bringing back for discussion.

@terrajobst
Copy link

terrajobst commented Jul 25, 2018 •
edited
Loading

Agree with @nguerrera. The problem is that ATF's default is net461 for netstandard20+. Thus, unless we warn the P2P will just work by default.

@rrelyea
Copy link
Contributor

rrelyea commented Jul 25, 2018

@mishra14 - please investigate this fix this morning.

This was referenced Jul 25, 2018
Merged
Open
mishra14 pushed a commit to NuGet/NuGet.Client that referenced this issue Jul 27, 2018
Adding warning for P2P reference with ATF in GetReferenceNearestTarge…
8abf12d 
…tFrameworkTask (#2366)

## 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 and VS
@rrelyea rrelyea modified the milestones: 4.8, 5.0, 4.9 Jul 27, 2018
@rrelyea
Copy link
Contributor

rrelyea commented Jul 27, 2018

Currently checked into 16.0/5.0 (dev branch).
Will consider 15.9 as well, so moving to 15.9 for now, to consider.

@rrelyea rrelyea modified the milestones: 4.9, 5.0 Nov 20, 2018
@rrelyea rrelyea changed the title No warning for AssetTargetFallback via P2P Using a projectreference which requires AssetTargetFallback to succeed, should warn. Jan 25, 2019
@rrelyea rrelyea added Type:Bug and removed Type:DCR Design Change Request labels Jan 25, 2019
@rrelyea rrelyea changed the title Using a projectreference which requires AssetTargetFallback to succeed, should warn. Using a ProjectReference which requires AssetTargetFallback to succeed, should warn. Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mishra14 mishra14

Labels
Functionality:Restore Priority:1 High priority issues that must be resolved in the current sprint. Type:Bug
Projects
None yet
Milestone
5.0
Development

No branches or pull requests
4 participants
@nguerrera @terrajobst @rrelyea @mishra14