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

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

Open
srivatsn opened this Issue Jan 13, 2017 · 28 comments

Comments

Projects
None yet
8 participants
@srivatsn
Contributor

srivatsn commented Jan 13, 2017

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 onovotny/microsoft-authentication-library-for-dotnet@d41e237.

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

This comment has been minimized.

Show comment
Hide comment
@srivatsn

srivatsn Jan 13, 2017

Contributor

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?

Contributor

srivatsn commented Jan 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 14, 2017

Contributor

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

Contributor

mavasani commented Jan 14, 2017

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

@onovotny

This comment has been minimized.

Show comment
Hide comment
@onovotny
Member

onovotny commented Jan 14, 2017

@mavasani what happens if you open this commit/repo: onovotny/microsoft-authentication-library-for-dotnet@d41e237

@nguerrera

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera Jan 14, 2017

Member

Try it reversed.

C1 : net46
C2 : netstandard1.3:net46

C1 -> C2

Member

nguerrera commented Jan 14, 2017

Try it reversed.

C1 : net46
C2 : netstandard1.3:net46

C1 -> C2

@mavasani

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 14, 2017

Contributor

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

Contributor

mavasani commented Jan 14, 2017

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

@nguerrera

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera Jan 14, 2017

Member

Then the screenshot shows the problem. Net46 should be preferred

Member

nguerrera commented Jan 14, 2017

Then the screenshot shows the problem. Net46 should be preferred

@mavasani

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 14, 2017

Contributor

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

Contributor

mavasani commented Jan 14, 2017

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

@nguerrera

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera Jan 14, 2017

Member

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

Member

nguerrera commented Jan 14, 2017

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

@mavasani

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 14, 2017

Contributor

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.

Contributor

mavasani commented Jan 14, 2017

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.

@onovotny

This comment has been minimized.

Show comment
Hide comment
@onovotny

onovotny Jan 14, 2017

Member

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.

Member

onovotny commented Jan 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera Jan 14, 2017

Member

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.

Member

nguerrera commented Jan 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera Jan 14, 2017

Member

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

Member

nguerrera commented Jan 14, 2017

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

@mavasani

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 14, 2017

Contributor

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).

Contributor

mavasani commented Jan 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 14, 2017

Contributor

image

Contributor

mavasani commented Jan 14, 2017

image

@nguerrera

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera Jan 14, 2017

Member
Member

nguerrera commented Jan 14, 2017

@nguerrera

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera Jan 14, 2017

Member

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

Member

nguerrera commented Jan 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 16, 2017

Contributor

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

Contributor

mavasani commented Jan 16, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 16, 2017

Contributor

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

Contributor

mavasani commented Jan 16, 2017

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

@nguerrera

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera Jan 16, 2017

Member

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

Member

nguerrera commented Jan 16, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 16, 2017

Contributor

Thanks. This is now tracked by VSO 368795.

Contributor

mavasani commented Jan 16, 2017

Thanks. This is now tracked by VSO 368795.

@onovotny

This comment has been minimized.

Show comment
Hide comment
@onovotny

onovotny Jan 16, 2017

Member

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

Member

onovotny commented Jan 16, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mavasani

mavasani Jan 16, 2017

Contributor

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

Contributor

mavasani commented Jan 16, 2017

@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 davkean reopened this May 24, 2017

@davkean

This comment has been minimized.

Show comment
Hide comment
@davkean

davkean May 24, 2017

Member

Going to leave this tracked here.

Member

davkean commented May 24, 2017

Going to leave this tracked here.

@nguerrera

This comment has been minimized.

Show comment
Hide comment
@nguerrera

nguerrera May 24, 2017

Member

Consider changing the bug title to reflect what is tracked.

Member

nguerrera commented May 24, 2017

Consider changing the bug title to reflect what is tracked.

@davkean davkean changed the title from P2P refs choose first tfm in multi-targting reference, not closest one to P2P refs choose first tfm in multi-targting reference, not closest one in legacy project system May 25, 2017

@PawelTroka

This comment has been minimized.

Show comment
Hide comment
@PawelTroka

PawelTroka Jul 2, 2017

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

PawelTroka commented Jul 2, 2017

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

@cclauson

This comment has been minimized.

Show comment
Hide comment
@cclauson

cclauson Sep 13, 2018

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 Sep 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@cclauson

cclauson Sep 14, 2018

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.

cclauson commented Sep 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@davkean

davkean Sep 14, 2018

Member

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.

Member

davkean commented Sep 14, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment