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

GetTargetPlatformIdentifier vs TargetPlatformIdentifier conditions #7359

Open
ViktorHofer opened this issue Feb 2, 2022 · 8 comments
Open
Labels

Comments

@ViktorHofer
Copy link
Member

ViktorHofer commented Feb 2, 2022

Based on @terrajobst's proposal which was implemented with .NET 5, I see a behavior difference when calling the GetTargetPlatformIdentifier('$(TargetFramework)') function or when using the $(TargetPlatformIdentifier) property:

<PropertyGroup>
  <TargetFrameworks>net6.0-windows;net6.0;net48</TargetFrameworks>
  <!-- True for net6.0 and net48 -->
  <DefineConst Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == ''">$(DefineConst);TargetsAnyOS</DefineConst>
</PropertyGroup>

<ItemGroup>
  <!-- True for net6.0 only. -->
  <Compile Include="a.cs" Condition="'$(TargetPlatformIdentifier)' == ''" />
</ItemGroup>

Reason for that is that msbuild defaults the $(TargetPlatformIdentifier) property to "windows" for any tfm older than net5.0 (.NETFramework, .NETStandard, .NETCoreApp <= 3.1):

<TargetPlatformIdentifier Condition="'$(TargetPlatformIdentifier)' == '' and '$(_EnableDefaultWindowsPlatform)' != 'false'">Windows</TargetPlatformIdentifier>

  1. Is it possible to disable setting a default "windows" TargetPlatformIdentifier (via the _EnableDefaultWindowsPlatform property) or does nuget/msbuild heavily rely on it being set? Asking for .NETStandard and .NETFramework tfms which based on their alias representation (net48 and netstandard2.0 vs net5.0-windows) don't include a platform.
  2. What is the official guidance around platform conditions? Should items call into the GetTargetPlatformIdentifier intrinsic function or use the TargetPlatformIdentifier property instead? In large projects with different platforms like in dotnet/runtime, would multiple GetTargetPlatformIdentifier invocations slow down the project's evaluation performance?
  3. When conditionally setting a property inside the project file based on the platform, the GetTargetPlatformIdentifier function must be called as the TargetPlatformIdentifier property isn't available at that time. Isn't it super confusing that ie net48 returns an empty result when calling the function but "windows" when reading form the TargetPlatformIdentifier property either from an item or from a property inside a target file?

I'm posting this as I stumbled upon this behavior difference while working on dotnet/runtime#64500. I'm unsure how to explain to devs on the team why .NETStandard and .NETFramework tfms sometimes have a "windows" platform (when using the TPI property) and sometimes not (when using the TPI function).

@terrajobst @rainersigwald

EDIT:
As an additional note, such a behavior difference is not observable when invoking the GetTargetFrameworkIdentifier function and reading from the TargetFrameworkIdentifier property.

@ViktorHofer ViktorHofer added the needs-triage Have yet to determine what bucket this goes in. label Feb 2, 2022
@ViktorHofer
Copy link
Member Author

cc @ericstj @safern

@rainersigwald
Copy link
Member

This looks like an awkward mismatch between the new world and the old--the TargetPlatformIdentifier default to Windows has been there since dev11. It was made optional and turned off for all SDK projects that target .NET 5.0+ in the 5.0.100 SDK: dotnet/sdk#12612, resolving dotnet/sdk#11233.

In large projects with different platforms like in dotnet/runtime, would multiple GetTargetPlatformIdentifier invocations slow down the project's evaluation performance?

Yes, but I hope not significantly.

I lean toward suggesting "never use $(TargetPlatformIdentifier); always use the property function. @dsplaisted, @baronfel, opinions on this whole mess?

@baronfel
Copy link
Member

baronfel commented Feb 2, 2022

That seems like sane guidance to me, after reading the previous threads. Props/Targets ordering is hard, and using the functions ensures consistency in a much easier way. It definitely seems like something that should be documented on the existing Best Practices or Customize your Build sections, and something that we should incorporate into the guidance/task examples that we're working on.

@ViktorHofer
Copy link
Member Author

I lean toward suggesting "never use $(TargetPlatformIdentifier); always use the property function. @dsplaisted, @baronfel, opinions on this whole mess?

Unfortunately the intrinsic function is quite verbose

  • $([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) vs
  • '$(TargetPlatformIdentifier)'

and it shows up as expensive in my profilevaluation binlog.

I lean toward suggesting "never use $(TargetPlatformIdentifier); always use the property function. @dsplaisted, @baronfel, opinions on this whole mess?

@rainersigwald do you know if it's possible to turn that legacy behavior off for .NETStandard and .NETFramework tfms? The internal property exists _EnableDefaultWindowsPlatform which could be set to false but I don't know if that would break anything fundamental underneath.

@ViktorHofer
Copy link
Member Author

Disabled the default windows platform via the _EnableDefaultWindowsPlatform switch and couldn't observe any difference in the assembly itself (neither in its metadata) or in the produced package. Maybe it's ok to use that switch?

@dsplaisted
Copy link
Member

We didn't want to change the default target platform of Windows for all projects that targeted existing frameworks, as that would likely break some of them. But for your own projects it should be safe to set _EnableDefaultWindowsPlatform to false.

I typically use $(TargetPlatformIdentifier) when possible instead of the property function because it is more concise. However, it is complicated to understand when you can use one versus the other.

@ViktorHofer
Copy link
Member Author

Thanks for clarifying. Would you accept a PR that makes the _EnableDefaultWindowsPlatform property "public" (remove the underscore from it)?

@rainersigwald
Copy link
Member

Unfortunately the intrinsic function is quite verbose

  • $([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) vs
  • '$(TargetPlatformIdentifier)'

and it shows up as expensive in my profilevaluation binlog.

The verbosity isn't great, but I may have some ideas about speeding things up. What are you running profileevaluation on specifically? I would like to poke at it.

@benvillalobos benvillalobos removed the needs-triage Have yet to determine what bucket this goes in. label Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants