dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.22k stars 1.35k forks source link

GetTargetPlatformIdentifier vs TargetPlatformIdentifier conditions #7359

Open ViktorHofer opened 2 years ago

ViktorHofer commented 2 years ago

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): https://github.com/dotnet/msbuild/blob/de1d7a295d709c3a748cc46cd5dc9bddd624ea46/src/Tasks/Microsoft.Common.CurrentVersion.targets#L90

  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 https://github.com/dotnet/runtime/pull/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 commented 2 years ago

cc @ericstj @safern

rainersigwald commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

rainersigwald commented 2 years ago

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.