dotnet / project-system

The .NET Project System for Visual Studio
MIT License
972 stars 389 forks source link

P2P refs choose first tfm in multi-targting reference, not closest one in legacy project system #1162

Open srivatsn opened 7 years ago

srivatsn commented 7 years ago

From @onovotny on December 19, 2016 20:52

In my solution, I have a multi-targeting project creating the following outputs: netstandard1.2;netstandard1.3;net45;win81;wpa81;MonoAndroid70;Xamarin.iOS10

When I do a P2P reference from an iOS app, a NET 4.5 unit test project, etc, they all pick netstandard1.2 as shown on the properties path of the reference: ref

You can repro this by loading the following https://github.com/onovotny/microsoft-authentication-library-for-dotnet/commit/d41e237f3fd1f9b6da06064b6d31046b9c19fbd2.

Build src\Microsoft.Identity.Client and then look at the properties of the test project below (and of the other sample projects.

Copied from original issue: dotnet/sdk#535

srivatsn commented 7 years ago

From @nguerrera on January 13, 2017 2:48

@onovotny What I am seeing is that build actually selects the correct TFM, (change output level to detailed and see /reference arg to compiler) but the property pane does incorrectly show the path of the first target framework.

I presume that you have worse symptoms than a bad path in the properties pane by the description of "so unit tests run for now" in your workaround commit. However, when I make a unit test from the net45 app that asserts that a method which returns the TFM does what I expect, it works fine. Do you have a (hopefully simple) repro for this causing unit tests to fail?

One thing I'm now noticing on top of the property pane being incorrect is that IntelliSense is showing me completions based on the surface are of the first TFM and not the one that a real build would use. I presume that has the same root cause as the bad property pane value.

Based on these obeservations, I think this is better tracked on https://github.com/dotnet/roslyn-project-system. @mavasani, @davkean What do you think?

mavasani commented 7 years ago

@nguerrera I am unable to reproduce this. See the screenshot below for the small repro project that I created. I see correct reference path in the properties pane and combined intellisense works fine too.

  1. ClassLibrary1 targets netstandard1.3
  2. ClassLibrary2 targets net46;netstandard1.3
  3. P2P from ClassLibrary1 to ClassLibrary2
  4. Project reference path is shown as bin\Debug\netstandard1.3\ClassLibrary2.dll
  5. Combined intellisense in ClassLibrary2 shows completions for both net46 and netstandard1.3

image

I am also not very clear what you mean by IntelliSense is showing me completions based on the surface are of the first TFM. The completions in intellisense are based on all target frameworks (combined intellisense). The items in the project context bar on the top left corner, e.g. Classlibrary2(netstandard1.3), Classlibrary2(net46), are alphabetically sorted. I verified both of these do work in the latest D15PreRel build.

clairernovotny commented 7 years ago

@mavasani what happens if you open this commit/repo: https://github.com/onovotny/microsoft-authentication-library-for-dotnet/commit/d41e237f3fd1f9b6da06064b6d31046b9c19fbd2

nguerrera commented 7 years ago

Try it reversed.

C1 : net46 C2 : netstandard1.3:net46

C1 -> C2

mavasani commented 7 years ago

Nick, that is what I tried (wrote reverse in repro steps). Oren, I'll try your repro tonight

nguerrera commented 7 years ago

Then the screenshot shows the problem. Net46 should be preferred

mavasani commented 7 years ago

For me C1 targets netstandard and screenshot is as expected. I'll try C1 to target net46 and reverse TFs order in C2.

nguerrera commented 7 years ago

I'm confused. Then you did have it reversed from what I said.

mavasani commented 7 years ago

I wrote C2 -> C1 instead of C1 -> C2. Anyways, I'll try what you suggested, though not sure how that makes a difference. The referenced project had its second TF as the preferred one, and reference path was fine and combined intellisense is agnostic of the ordering.

clairernovotny commented 7 years ago

In the commit that I referenced in my repro, the net45 unit test library has a P2P ref to the multi-targeted library. In my screen shot, you can see that it shows the \bin\debug\netstandard1.2\foo.dll ref instead of the expected \bin\debug\net45\foo.lib.

nguerrera commented 7 years ago

The difference (I think) is when first is applicable but not best. Net46 can use netstandard but must should prefer net46, netstandard cannot use net46.

nguerrera commented 7 years ago

It looked to me yesterday like it was picking first applicable and not best.

mavasani commented 7 years ago

I tried with the below reverse setup and all seems to work fine.

  1. C1 - net46
  2. C2 - netstandard1.3;net46
  3. C1 -> C2

Reference properties shows path as "bin\debug\net46". I get combined intellisense for both targets in C2's source files. Note that the ordering of the project names in project context menu when source files for C2 are opened is always alphabetical (same as for source files belonging to shared projects).

mavasani commented 7 years ago

image

nguerrera commented 7 years ago

Hmm, ok, I will try again on the build I have and then again on latest. If I get a repro on latest, I'll publish the solution for you.

nguerrera commented 7 years ago

OK, I think I see why our repros vary. Mine was using a classic desktop project -> multi-targeted and I'm guessing your desktop project is SDK-based. I think it's only an issue for the old project system.

(Aside: Getting back to what I meant about IntelliSense -- my wording was bad and I was wrong about there being an issue, but I'd still like to clarify. Combined intellisense is for combining all of my target frameworks, not combining all of the target frameworks of my refs. What I thought I saw was it showing me my reference's API that are never visible to me. e.g. If I'm always picking net46 from my ref, I should not see the API that my ref has only on net462. However, I had an ifdef inverted and in fact IntelliSense is showing the correct thing just as build is doing the right thing.)

The only issue I see is that the old project system is showing the wrong path in the property pane. The new project system is showing the correct path there.

I've published the small solution I've been using for investigation to https://github.com/nguerrera/ReproForManish

mavasani commented 7 years ago

@onovotny I am unable to checkout that commit. I get the below error:

fatal: reference is not a tree: d41e237f3fd1f9b6da06064b6d31046b9c19fbd2

I checked commits history on https://github.com/onovotny/microsoft-authentication-library-for-dotnet/commits/dev and I don't see that commit either.

mavasani commented 7 years ago

@nguerrera Verified that Multitargeted.csproj P2P reference has the correct path in your repro solution on latest drops.

nguerrera commented 7 years ago

@mavasani For me, it is correct from SdkBasedNet46 (new project system) and incorrect from ClassicNet46 (old project system). I think this needs to be moved to VSO as an issue with the old project system.

mavasani commented 7 years ago

Thanks. This is now tracked by VSO 368795.

clairernovotny commented 7 years ago

@mavasani you should be able to see it here: https://github.com/onovotny/microsoft-authentication-library-for-dotnet/blob/72cc3f9682062811c3ebe17acc77ef11d6123d83/src/Microsoft.Identity.Client/Microsoft.Identity.Client.csproj#L3, but you need to change the order of the TFM's to put net45 at the end. Then look at the test project's dependency properties in VS.

mavasani commented 7 years ago

@onovotny Yes, I verified it repros when the old project system based project (net45) is involved. Hence, I moved the bug to the internal VSO repo tracking old project system bugs. Things work fine if your referencing project is based on the new SdkBasedNet46 project system.

davkean commented 7 years ago

Going to leave this tracked here.

nguerrera commented 7 years ago

Consider changing the bug title to reflect what is tracked.

PawelTroka commented 7 years ago

I can confirm this issue. Any plans of releasing it soon?

cclauson commented 6 years ago

I have a project that repros an issue: reprovsbuild.zip

projb is multitargeted, and references proja only if the TargetFramework is uap10.0. If I build projb from Visual Studio, proja is never built.

I reported this earlier here: https://developercommunity.visualstudio.com/content/problem/205121/vs-doesnt-build-referenced-project-when-excludeass.html was told that it's the same as this issue. Is this true? Also, the title of this issue states that it's a problem with the legacy project system, but I believe this project uses the new project system. Is there any workaround?

Thanks so much

cclauson commented 6 years ago

Ah, I figured out a workaround for my issue. It seems that VS will know about the dependency if it's a project reference during design time build. So I can condition it like this:

<ProjectReference Condition="'$(TargetFramework)' == 'uap10.0' or $(DesignTimeBuild) == 'true'" Include="..\proja\proja.csproj" />

This will cause VS to build the dependency, but not link against the built dll for the undesired target framework.

davkean commented 6 years ago

As a guess we’re not pushing the dependency to "solution build manager" that coordinates builds, as we're only factoring in the first TFM, a better workaround would be condition the reference output assembly:

     <ProjectReference Include="..\proja\proja.csproj">
        <ReferenceOutputAssembly Condition="'$(TargetFramework)' == 'uap10.0'">true</ReferenceOutputAssembly>
     </ProjectReference>

That avoids any TFM checks from being run during design-time build and prevents the API that from dll from being accessible in the other TFMs context.

Bart76 commented 5 years ago

I was referred to this issue from https://developercommunity.visualstudio.com/content/problem/283983/multitarget-project-gives-reference-warning.html

I am using Visual Studio 2017 version 15.9.6 and Microsoft .NET Core SDK 2.2.103 on a 64-bit Windows 10 (10.0.17134.523) machine.

I have a simple solution containing two projects:

Client has a reference to Library.

Everything works fine.

But the Error List shows an annoying warning for project Client: "The project 'Library' cannot be referenced. The referenced project is targeted to a different framework family (.NETCoreApp)."

Removing and adding the reference from Client to Library makes the warning disappear. But only until the project gets reloaded.

When restarting Visual Studio and reloading the solution, the initial warning is shorter: "The project 'Library' cannot be referenced.' After the first run, the sentence "The referenced project ... different framework family (.NETCoreApp)" is appended to the warning.

The warning goes away (and stays away) when I change the order of the target frameworks in the project file of Library so that it starts with "netstandard2.0" instead of "netcoreapp2.2". However, that's not really what I want. IMHO "netcoreapp2.2" should be the first value, since that is technically the most fitting one. ("netstandard2.0" was just added to make it referenceable from Client).

Since it works fine and it seems to be just a warning, I can live with it for now. But I do hope it will be resolved in the near future. I prefer to have a clean Error List. ;)

tmeschter commented 5 years ago

Tom's triage notes: If this still reproduces we should plan on fixing it. It will make it more difficult for customers to migrate to the new project types incrementally if they can't trust the build to do the right thing in mixed solutions.

tmeschter commented 5 years ago

Further notes:

Given that these don't affect the build we should also consider simply not fixing the issue, or just implementing some heuristics.

davkean commented 5 years ago

@tmeschter We should consider, if we were to fix this, to just have the project system opt out of validating references and have the build do it. This is the approach we took in the new project system.

tmeschter commented 5 years ago

@davkean Yes, that would be the simplest fix. However, it would only address the spurious warning in the Error List; it wouldn't fix the Path in the Properties window.

rolfik commented 5 years ago

I have the similar warning while referencing project A targetted to uap10.0.16299;netstandard2.0;net462 with Project reference in project B tagetted to .NET Framework 4.7.2 }old .csproj format):

Warning The project 'A' cannot be referenced. The referenced project is targeted to a different framework family (.NETCore)

Using Visual Studio 2019 16.3.1

msmolka commented 5 years ago

Still issue after 2 years?

CpBobette commented 4 years ago

Coming From #2777 I still have this issue using VS2019, and referencing P2P from UWP(targeting 17763) a cross-platform library.<TargetFrameworks>netstandard2.0;net461</TargetFrameworks>

Edit: Restarting VS and putting netstandard2.0 as the first target fixed the issue

Thieum commented 3 years ago

Edit: Restarting VS and putting netstandard2.0 as the first target fixed the issue