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

Project-to-project reference fails to detect correct multi-target framework if <TargetFramework> is defined #7856

Open
jonathanou opened this issue Jul 29, 2022 · 14 comments

Comments

@jonathanou
Copy link

If a project has both <TargetFramework> and <TargetFrameworks> defined, other projects cannot correctly reference the project's target framework that is different than what <TargetFramework> specifies.

Building the project with both properties defined does correctly produce the binaries for each targeted framework. It's just project-to-project references that doesn't work.

Create project A (ClassLibrary1) like so:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net6.0-windows</TargetFramework>
    <TargetFrameworks>net6.0;net6.0-windows</TargetFrameworks>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

</Project>

and project B (ClassLibrary2) like so:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <ProjectReference Include="..\ClassLibrary1\ClassLibrary1.csproj" />
  </ItemGroup>

</Project>

Project B (ClassLibrary2) will fail to build with the following error:

Project '..\ClassLibrary1\ClassLibrary1.csproj' targets 'net6.0-windows'. It cannot be referenced by a project that targets '.NETCoreApp,Version=v6.0'.	ClassLibrary2	C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets	1806	

From the binlog, it seems MSBuild will treat project A as having a single target framework:

image

If I remove <TargetFramework> in project A, then MSBuild correctly treats project A as having multiple target frameworks:

image

@Nirmal4G
Copy link
Contributor

From the binlog, it seems MSBuild will treat project A as having a single target framework

Yes, and that's the expected behavior! Do you need to specify singular TF when you already specified TFS?

@jonathanou
Copy link
Author

Do you need to specify singular TF when you already specified TFS?

My company's build infrastructure specifies a default TF via Directory.Build.props file, so I am not in control of TF having a value that seems to contradict TFS.

To me, it seems even if TFS conflicts with TF, TFS should take precedence? Certainly when I build the project, TFS overrides TF as expected, as I see multiple binaries being built.

@Nirmal4G
Copy link
Contributor

When you build the project ClassLibrary1 separately, TFS takes precedence and multi-build comes into play and the inner build replaces the TF specified in the project file, even if TF is some garbage value. It builds and it produces multiple outputs.

But when using project reference, the resolution logic thinks it's a single target build because TF takes precedence there. Yes, this is more of a design issue than an implementation bug. I don't know if the team would allow the change since it's a behavior change, and a huge breaking change at that.

In the meantime, the specific problem with your setup considering the current project reference behavior is that you're trying to reference Windows-specific implementation to a cross-platform library which is not allowed. If you're expecting the project to build as net6.0 instead of net6.0-windows, you can modify the ClassLibrary2 project file as shown below...

<ProjectReference Include="..\ClassLibrary1\ClassLibrary1.csproj" SetTargetFramework="TargetFramework=$(TargetFramework)" />

@jonathanou
Copy link
Author

If you're expecting the project to build as net6.0 instead of net6.0-windows, you can modify the ClassLibrary2 project file as shown below...

Thanks for the tip! While it works, the disadvantage is every new project reference made to ClassLibrary1 will need this special workaround. It doesn't "just work".

I am currently manually unsetting the target framework in my ClassLibrary1 csproj file by doing this:

<TargetFramework></TargetFramework>
<TargetFrameworks>net6.0;net6.0-windows</TargetFramework>

This allows other projects to reference ClassLibrary1 normally.

However, unsetting the target framework is also not ideal, since every project in our solution that wants to multi-target now needs to know to manually unset the target framework.

But when using project reference, the resolution logic thinks it's a single target build because TF takes precedence there. Yes, this is more of a design issue than an implementation bug. I don't know if the team would allow the change since it's a behavior change, and a huge breaking change at that.

In addition to the build preferring TFS over TF, the Visual Studio UI also shows TFS over TF if both are defined. Here is the ClassLibrary1 properties UI:

image

It just seems to be that if both TF and TFS are defined, TFS generally trumps TF. I feel like the current project reference behavior preferring TF over TFS seems inconsistent with how other areas of MSBuild works.

Hopefully changing the project reference behavior can be considered!

@Nirmal4G
Copy link
Contributor

IMO, The precedence of TFS over TF should be consistent. I'm actually in favor of making the ProjectReference-protocol to support multi-target resolution even if a singular target has already been defined. But it comes with its own set of problems; not that we can't solve it.

However, unsetting the target framework is also not ideal, since every project in our solution that wants to multi-target now needs to know to manually unset the target framework.

I get it but understand that if TFS takes precedence always, setting the TF centrally doesn't make all the projects build to that. It'll build for all targets and that might take up your build time.

From what I understand, the role of the TF in your setup is more like a default target rather than a single target. If that's the case and you have a mix of projects that have both TF and TFS, rename all TF to TFS. Even a single target can be specified in TFS. There's a slight overhead involved in build time but it's worth in maintaining the source better.

@jonathanou
Copy link
Author

jonathanou commented Jul 30, 2022

If that's the case and you have a mix of projects that have both TF and TFS, rename all TF to TFS. Even a single target can be specified in TFS.

That's a good point! I did not think about that. One potential problem I can currently think of is if any project decides to override the default target framework with a different, single, TF, they must know to put the override in TFS, not TF. But the good thing is modifying TF through Visual Studio project UI will correctly modify TFS in the csproj file (since Visual Studio prefers TFS if it is defined in the project).

I think a lot of these problems could be avoided if TFS is used as the only way to specify TF, even if the project only intends to target a single TF. But I guess it's probably too late to remove support for <TargetFramework> altogether at this point...

@Nirmal4G
Copy link
Contributor

I think they left TF alone since a lot of application projects use single target. Only libraries use multiple targets. As I already mentioned, it has a performance penalty, minuscule but still an overhead to be considered, especially in large solutions (> 100 single target projects). Anyway, it's a trade-off that falls on us, maintainers!

@rainersigwald
Copy link
Member

Project files should specify only TargetFramework or TargetFrameworks, and not both.

My company's build infrastructure specifies a default TF via Directory.Build.props file, so I am not in control of TF having a value that seems to contradict TFS.

Unsetting via <TargetFramework /> is probably the best you can do here.

I think a lot of these problems could be avoided if TFS is used as the only way to specify TF, even if the project only intends to target a single TF. But I guess it's probably too late to remove support for <TargetFramework> altogether at this point...

There is also a performance implication: the "outer" build triggered when there are only TargetFrameworks collects information about the individual TargetFrameworks to build, where the "inner" build of a single TF doesn't have to care about that.

We spent some time in team triage today trying to think of a way to warn or message the "TF xor TFs" requirement but didn't come up with anything that sounded particularly great.

@Nirmal4G
Copy link
Contributor

Nirmal4G commented Aug 4, 2022

We spent some time in team triage today trying to think of a way to warn or message the "TF xor TFs" requirement but didn't come up with anything that sounded particularly great.

@dsplaisted I think the SDK team should come up with something, be it a warning or ProjectReference behaviour change. Any solution is fine as long as it's understandable from User PoV.

@stan-sz
Copy link
Contributor

stan-sz commented Apr 2, 2024

@rainersigwald a simple verification of unnecessary single TF value in TFs property could be:

<Target Name="ValidateTargetFrameworks" AfterTargets="_ComputeTargetFrameworkItems" Condition=" '$(TargetFrameworks)' != '' ">
  <Warning Condition="'@(_TargetFramework->Count())' &lt;= 1"
           File="$(MSBuildProjectFile)"
           Text="&lt;TargetFrameworks&gt; property requires multiple frameworks. For a single value use &lt;TargetFramework&gt;" />
  <Warning Condition="'@(_TargetFramework->Count())' != '@(_TargetFrameworkNormalized->Count())' "
           File="$(MSBuildProjectFile)"
           Text="Remove duplicates from &lt;TargetFrameworks&gt;" />
</Target>

Do you think it could be shipped as a MSBuild change wave feature?

@rainersigwald
Copy link
Member

That sounds like a breaking change (adding a warning) for a legal situation (doing a "foreach" with one item isn't ideal but doesn't cause very many problems). I would expect a very high bar for such a thing. (Adding it as a BuildCheck/analyzer would make sense to me.)

@KalleOlaviNiemitalo
Copy link

In one C# project, I deliberately set TargetFrameworks with only one framework because I wanted to Directory.Build.props to read TargetFramework in the inner build and set OutputPath and some other things.

@stan-sz
Copy link
Contributor

stan-sz commented Apr 5, 2024

That sounds like a breaking change (adding a warning) for a legal situation (doing a "foreach" with one item isn't ideal but doesn't cause very many problems). I would expect a very high bar for such a thing. (Adding it as a BuildCheck/analyzer would make sense to me.)

Pinning your comment to #1777 :)

@IanKemp
Copy link

IanKemp commented Dec 5, 2024

Project files should specify only TargetFramework or TargetFrameworks, and not both.

Then the tooling should enforce this, not act in a completely illogical way of silently ignoring TFS when both it and TF are provided. If TFS took precedence over TF (as would make sense per the principle of least astonishment), or TF was merged into TFS, then this would not be a problem.

If implementing the aforementioned logical behaviour is a breaking change, then add a flag we can set in Directory.Build.props or our csprojs to opt in to that behaviour. But don't leave this known issue to sit for 2+ years without any action being taken.

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

No branches or pull requests

8 participants