Don't use Targets* helper properties in libs

[ViktorHofer]

Depends on #64610, dotnet/arcade#8459

High level goals

1. Allow libraries projects to efficiently/correctly target a single target framework. Today it's not possible for projects to use
   TargetFramework over TargetFrameworks even when only a single target framework is used: [Question] Why test libraries that target single TFM use TargetFrameworkS (plural)? #47155. Because of that, we add ~250 project outer-build evaluations (100ms in average per outer build) to the overall build. These evaluations impact both incremental restore, incremental build and the overall build.
2. It makes dotnet/runtime libraries use the same common path as our customers when a project needs to condition on platforms.
3. Tooling works best when we don't rely on global properties being set. Today, the Targets* properties rely on the TargetFramework property being passed in globally during the dispatch from an outer to the inner builds. During design time builds and eventually in other scenarios, the TargetFramework property isn't passed in globally and instead is dynamically injected after the first project's property group. This change will avoid the dependency on the global property and will better support tooling like Visual Studio and Visual Studio Code. Nasty hacks like

   runtime/eng/BeforeTargetFrameworkInference.targets

   Line 3 in 04bb6eb

   <Import Project="$(MSBuildThisDirectory)targetframeworksuffix.props" Condition="'$(DesignTimeBuild)' == 'true'" />

   can be removed entirely.

More detailed summary

This change makes it possible to migrate 200+ (ref+src) projects to use
TargetFramework instead of TargetFrameworks which avoids the additional
outer build evaluation and invocation which ultimately makes the overall
build faster. As a side effect, this removes yet another artificial difference
between the libraries projects and customer projects. The conditions being
used in our projects are mostly TargetFrameworkIdentifier,
TargetPlatformIdentifier and IsTargetFrameworkCompatible. As a .NET
developer I would use exactly the same conditions in my own project.

Targets* properties (i.e. TargetsWindows, TargetsAnyOS, TargetsUnix,
etc.) rely on the TargetFramework property which usually are set
inside a project. The TargetFramework property is only
available before a project specifies it if it's explicitly set in a props
file or if the project is cross-targeting and the outer-build dispatches
into the inner-build. During the dispatch, the TargetFramework property
is passed in as a global property.

Until now that behavior wasn't a problem because every libraries project
cross-targeted (by setting the TargetFrameworks property) even though
many only include a single TargetFramework (i.e. NetCoreAppCurrent).

To allow projects to use the TargetFramework property instead of
TargetFrameworks, the Targets* helper properties can't be calculated
anymore early in a props file as the TargetFramework property isn't set
at that time.

In general, the guidance by the SDK/msbuild team is to not read from the
TargetFramework property before the project sets it
(in a property group). That effectively means that the TargetFramework
property shouldn't be used in props files at all.

Therefore these helper properties can't be used anymore for property
conditions and I'm replacing their usage with TargetPlatformIdentifier
comparisons for both properties and items.
In nearly all cases, the Targets* helper properties can be replaced with
TargetPlatformIdentifier checks on items and in the few cases where
TargetsUnix or TargetsLinux marks multiple tfms as compatible, the exact
tfms must be used instead for the TargetPlatformIdentifier comparison.

Whenever a project needs to condition properties on the platform, I'm
first setting the TargetPlatformIdentifier the same way the SDK sets it
so that the SDK later doesn't need to set it again to avoid the
additional expensive msbuild function call.

[ghost]

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Targets* properties (i.e. TargetsWindows, TargetsAnyOS, TargetsUnix,
etc.) rely on the TargetFramework property which usually are set
inside a project. The TargetFramework property is only
available before a project specifies it if it's explicitly set in a props
file or if the project is cross-targeting and the outer-build dispatches
into the inner-build. During the dispatch, the TargetFramework property
is passed in as a global property.

Until now that behavior wasn't a problem because every libraries project
cross-targeted (by setting the TargetFrameworks property) even though
many only include a single TargetFramework (i.e. NetCoreAppCurrent).

To allow projects to use the TargetFramework property instead of
TargetFrameworks, the Targets* helper properties can't be calculated
anymore early in a props file as the TargetFramework property isn't set
at that time. Changing

In general, the guidance by the SDK/msbuild team is to not read from the
TargetFramework property before the project sets it
(in a property group). That effectively means that the TargetFramework
property shouldn't be used in props files at all.

Therefore these helper properties can't be used anymore for property
conditions and I'm replacing their usage with TargetPlatformIdentifier
comparisons.
Whenever a project needs to condition something on the platform, I'm
first setting the TargetPlatformIdentifier the same way the SDK sets it
so that the SDK later doesn't need to set it again to avoid the
additional expensive msbuild function call.

In the very few projects which include netstandard2.0-X as a TFM, the
custom property TargetFrameworkSuffix needs to be used as the suffix is
stripped from the TargetFramework property. So netstandard2.0-windows
and netstandard2.0 are set to the same value in an inner build:
netstandard2.0.

This change makes it possible to migrate 200+ (ref+src) projects to use
TargetFramework instead of TargetFrameworks which avoids the additional
outer build evaluation and invocation which ultimately makes the overall
build noticeably faster.

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	area-Infrastructure-libraries
Milestone:	-

[ericstj]

Similar to your other PR, we need to document what our rules are and if possible try to catch incorrect use. I'm supportive of the restriction here, but it will be a very subtle distinction that you can use Targets* in an ItemGroup but not PropertyGroup. I can imagine folks introducing bugs that might slip through PR or at least cause a significant amount of head-scratching.

[ViktorHofer]

I'm supportive of the restriction here, but it will be a very subtle distinction that you can use Targets* in an ItemGroup but not PropertyGroup. I can imagine folks introducing bugs that might slip through PR or at least cause a significant amount of head-scratching.

Agreed and I'm a bit worried about this as well. I can definitely add/update documentation to point out this restriction but I doubt this is something that can be protected/validated without writing an msbuild formatter/analyzer.

Do you have any more ideas how to best roll this change out? Ideally all platform conditions, regardless of where they are applied on (property, item, import) would use the TargetFrameworkIdentifier property. Unfortunately I don't think that is possible in all cases today as NuGet doesn't understand compatibility between platforms/RIDs today.
Would it make sense to switch existing item conditions that map 1:1 to platforms like Android, iOS, tvOS, MacCatalyst, Linux and Windows as well over to use TargetPlatformIdentifier and only keep the few Targets* helper properties that are used for inheritance checks like TargetsUnix?

[ericstj]

without writing an msbuild formatter/analyzer.

Might be worth it at some point if we have enough conventions / style to justify it.

Ideally all platform conditions, regardless of where they are applied on (property, item, import) would use the TargetFrameworkIdentifier property

Agreed. At first this is what I thought you were doing, but then I realized you were only doing PropertyGroups

inheritance checks like TargetsUnix

I wonder how many of these we have? If it's small enough we could just say remove them all and make the few cases that use TargetsUnix slightly more complicated by checking all the values that are actually targeted.

Do you have any more ideas

I could imagine all our projects have some boilerplate that parses the platform. Right now you've copy-pasted that boilerplate (TargetPlatformIdentifier). I could imagine we put that boilerplate in props file and instead insert an import. That props file could do whatever parsing we want. We could have a target that makes sure that import is present whenever a project has platform-specific TargetFramework values. I'm not sure if I like it or hate it, but it does define a process that is self-validating.

[ViktorHofer]

I believe this change is now ready as well but let me do some additional validation and verification tomorrow. Also I'm waiting for the Helix test results to see if I got something wrong.

[joperezr]

That could be an option, yes. That said, it is not uncommon for a project that multi-targets to use the Targets* properties both for properties and for conditionally compiling items, so I suppose in those cases it would be weird to have different conditions for properties and items. That said it would be interesting to hear what @ViktorHofer thinks about that.

Also, to be fair and something I didn't comment above, is that the fact that you need to look into the TargetFrameworks property when conditionally compiling files is not really new, nor is it introduced by these changes. I do agree that it is a bit different the way it is addressed, but in the past you also had this issue. For example, if you had a project that targeted: net7.0 and net7.0-windows where your "unix" build is effectively the platform-independent configuration, you already wouldn't be able to use conditions like:

<ItemGroup Condition="'$(TargetsUnix)' == 'true'">
   <!-- Add my unix compile items here -->

Since your Unix build was actually your AnyOS build, so you would actually need to instead use: '$(TargetsAnyOS)' == 'true'

[ViktorHofer]

How bad would it be if we moved these "helper" properties into a .targets file, and made the rule "You can't use Targets* properties in property evaluation"? And then we based these "helper" properties off of TargetFramework/TargetPlatformIdentifier/etc. This would be symmetrical to TargetFrameworkIdentifier today - you can't use that property in property evaluation for the same reason.

Initially I just switched properties over to use TPI instead of Targets* properties. Eric then later commented that it is confusing to have TPI conditions for properties and Targets* conditions for items and I agree with him on that. In addition to that I think it's valuable to use the same concept for platform conditions that customers follow and that we document: https://docs.microsoft.com/en-us/visualstudio/msbuild/property-functions?view=vs-2022#msbuild-targetframework-and-targetplatform-functions. Before .NET 5, TPI wasn't suitable to use as target frameworks didn't support encoding platforms in them and the Targets* helper properties had to be used.

Then, we could still have "readable" conditions in most places, which @stephentoub is asking for. And if a project does need to condition properties based on TargetPlatformIdentifier, we do it like you have the proposed changes here.

What we are observing here is a limitation that exists in the product and should be fixed there. MSBuild/NuGet doesn't support checking compatibility between platforms yet. Example: $([MSBuild]::TargetPlatformCompatible('$(TargetFramework)', 'unix'))

so it'd be nice if that were included in this PR. A separate commit is fine.

Of course. I wanted to keep the PR "simple" but let me add the change to better demonstrate the pros of this.

Separately, I'm not clear on changes like this one, which is removing a netstandard2.1 build apparently?

@stephentoub that was an unrelated clean-up. The netstandard2.1 tfm is unnecessary as both the netstandard2.0 and the netstandard2.1 builds are PNSEs. If it helps I can extract this change into a separate PR. I agree that it would have helped if I had left a comment to explain this change. Will keep this in mind for the next time.

[eerhardt]

What we are observing here is a limitation that exists in the product and should be fixed there. MSBuild/NuGet doesn't support checking compatibility between platforms yet. Example: $([MSBuild]::TargetPlatformCompatible('$(TargetFramework)', 'unix'))

Agreed. But until we get that functionality in the product, there's nothing wrong with us having some "helper" functions to make our code clean/readable.

[ViktorHofer]

Changes like this one are also harder to understand (at least for me) without digging in further to what's going on:
Before:
After:

@stephentoub I reverted all the PNSE change. I thought those might be easier to read but apparently it's the other way around and I introduced unnecessary complexity.

Agreed. But until we get that functionality in the product, there's nothing wrong with us having some "helper" functions to make our code clean/readable.

@eerhardt I think mixing platform conditions (TPI + Targets*) is more confusing than applying platform conditions with the TPI property only (even though inheritance isn't supported with TPI and they might be more verbose). Imagine the following case:

A project has platform specific properties and items:

<PropertyGroup>
  <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Linux;$(NetCoreAppCurrent)-OSX</TargetFrameworks>
</PropertyGroup>

<!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. -->
<PropertyGroup>
  <TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
  <GeneratePlatformNotSupportedMessage Condition="'$(TargetPlatformIdentifier)' == 'windows'">...</GeneratePlatformNotSupportedMessage>
</PropertyGroup>

<ItemGroup Condition="'$(TargetsUnix)' == 'true'">
  <Compile Include="..." />
</ItemGroup>

vs.

<PropertyGroup>
  <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Linux;$(NetCoreAppCurrent)-OSX</TargetFrameworks>
</PropertyGroup>

<!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. -->
<PropertyGroup>
  <TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
  <GeneratePlatformNotSupportedMessage Condition="'$(TargetPlatformIdentifier)' == 'windows'">...</GeneratePlatformNotSupportedMessage>
</PropertyGroup>

<ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'Linux' or '$(TargetPlatformIdentifier)' == 'OSX'">
  <Compile Include="..." />
</ItemGroup>

Today only two Targets* properties which support inheritance exist (TargetsUnix and TargetsLinux for Android) and only very few projects depend on that feature. If we think that this change makes the projects that leverage this feature unreadable, we could define such a helper property per project in a property group so that it can be used both for property and item conditions. That said, based on what I saw while making these changes, I don't think that's necessary.

Another fundamental problem with the libraries specific Targets* properties is that they have a totally different meaning than the Targets* properties that are repo-wide defined, i.e. TargetsMobile. The former is inferred from the TargetFramework and hence means the platform that the target framework targets. The latter is inferred from the TargetOS and is just a shorthand form for ' $(TargetOS)' == '...' conditions. Because of their equal prefix, people used them equally even though they mean something totall different.
As an example here's one of such mistakes in main today:

runtime/src/libraries/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj

Lines 12 to 14 in 1c80769

	<ItemGroup Condition="'$(TargetsMobile)' != 'true'">
	<ILLinkSubstitutionsXmls Include="$(ILLinkDirectory)ILLink.Substitutions.NonMobile.xml" />
	</ItemGroup>

This condition is only true when the TargetOS is one of the mobile OSs but not when the target framework platform is a mobile one. That means that if you compile this library i.e. on a Windows OS the condition will be false vs. when you compile the library on a mobile OS.

[akoeplinger]

In general this seems like a good change, I don't think getting rid of the TargetsUnix inheritance thing is a big deal apart from the potential readability concerns that Stephen mentioned.

[safern]

So it seems like we reached an agreement on taking this change? If so, I will start prepping the email to send out to the contributors to notify them about it and merge it.

[eerhardt]

So it seems like we reached an agreement on taking this change?

No objection from me.

[ViktorHofer]

@safern The .NET Framework leg is currently failing. The root cause is understood (currently the .NETFramework leg is overbuilding which will be fixed with dotnet/arcade#8459) and I will push a commit into this PR probably tomorrow, when the arcade update is consumable. Can you meanwhile please send out the mail and let me know here on GH when that's done? (I currently don't read my work mails 🌵). Thanks a lot everybody for the continued support and your feedback.

@stephentoub I agree that your readability concern is an actual issue. Still, as it only affects a small number of projects I think we should live with it for the sake of the improvement that this PR offers, but drive towards a feature in the sdk/msbuild/nuget that eventually will allow us form compatibility conditions based on platforms (as proposed earlier in this thread).

[safern]

Can you meanwhile please send out the mail and let me know here on GH when that's done?

Yup, I'll do that now.

[safern]

Actually it is already late here on a Friday, so I think my email would just be missed by a ton of people, we don't you go ahead and merge over the weekend and I can send the email on Monday morning?

[ViktorHofer]

Actually it is already late here on a Friday, so I think my email would just be missed by a ton of people, we don't you go ahead and merge over the weekend and I can send the email on Monday morning?

@safern just to close the loop on this, did you already send out the notice for this change?

[safern]

Thanks for the reminder, I just sent the notice out 😄

[ViktorHofer]

Thanks for sending the mail out Santi. I just quickly read over it and I fear that it might cause unnecessary confusion and might trigger negative reactions. I don't think me responding to it would be a wise choice as I'm still on leave for the next two months and as you are the sender so I kindly ask you if you would mind sending out a quick follow-up pointing people to the doc:

As the previous message might have caused some confusion, please see https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/project-guidelines.md#targetframework-conditions which nicely summarizes how to condition on specific target frameworks and platforms.

Thank you.

[safern]

Thanks, @ViktorHofer, I don't know if you missed it but I did include a pointer to the doc:

For more details about how to use these conditions, please refer to this document: runtime/project-guidelines.md at main · dotnet/runtime (github.com)

[ViktorHofer]

Yes I missed that part in your mail. Then any remaining confusion on this topic should already be covered by the link to the doc. Thanks Santi