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.21k stars 1.35k forks source link

Allow TFM/TPM checks in a concise, understandable way #5171

Closed terrajobst closed 4 years ago

terrajobst commented 4 years ago

As part of the spec for the .NET 5 TFM work we identified an issue with TFM checks in conditions.

Background on MSBuild evaluation In SDK-style projects there are two kinds of MSBuild files that are automatically included into each project: * `*.props`: These files are included at the top of the user's project file and are used to define a set of default properties that the user's project file can use. * `*.targets`. These files are included at the bottom of the user's project file, usually meant to define build targets and additional properties/items that need to depend on properties defined by the user. Furthermore, MSBuild has a multi-pass evaluation model where properties are evaluated before items. Why is all of this important? Because it controls which properties the user can rely on in their project file. Often, a user wants to express a condition like "include this file if you're compiling for .NET 5 or higher". Logically one would like to express it like this: ```xml ``` but this doesn't work because that would be a string comparison, not a version comparison. Instead, the user has to write it like this: ```xml ``` This works for conditions on item groups because they are evaluated after properties. Since the user's project file defines the `TargetFramework` property, the SDK logic that expands it into the other properties such as `TargetFrameworkIdentifier` and `TargetFrameworkVersion` has to live in `*.targets`, i.e. at the bottom of the project file. That means these automatically expanded properties aren't available for the user when defining other properties. This happens to work for items because items are evaluated after all properties are evaluated.

Due to MSBuild evaluation order the user cannot define properties like this:

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

  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;netcoreapp3.1</TargetFramework>
  </PropertyGroup>

  <PropertyGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETStandard'">
    <SomeProperty>Some .NET Standard specific value<SomeProperty>
  </PropertyGroup>

  <PropertyGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
    <SomeProperty>Some .NET Core specific value<SomeProperty>
  </PropertyGroup>

</Project>

In the past, we've seen people working this around by using string processing functions against the TargetFramework property, which is less than ideal.

Option using attributes

Ideally, we'd expose functionality such that the user can do version checks:

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

  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;netcoreapp3.1</TargetFramework>
  </PropertyGroup>

  <PropertyGroup TargetFramework="netstandard">
    <SomeProperty>Some value that applies to all versions of .NET Standard<SomeProperty>
  </PropertyGroup>

  <PropertyGroup TargetFramework=">=netcoreapp2.0">
    <SomeProperty>Some value that applies to .NET Core 2.0 and later<SomeProperty>
  </PropertyGroup>

  <PropertyGroup TargetFramework="==net5.0-ios13.0">
    <SomeProperty>Some value that only applies to .NET 5 + iOS 13.0<SomeProperty>
  </PropertyGroup>

  <PropertyGroup TargetPlatform="windows">
    <SomeProperty>Some value that applies to all version of Windows<SomeProperty>
  </PropertyGroup>

  <PropertyGroup TargetPlatform=">=ios-12.0">
    <SomeProperty>Some value that applies to iOS 12.0 and later<SomeProperty>
  </PropertyGroup>

</Project>

The idea is:

Option via new syntax

We could also invent new syntax that allows parsing of constitutes like this:

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

  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;netcoreapp3.1</TargetFramework>
  </PropertyGroup>

  <PropertyGroup Condition="$(TargetFramework::Identifier)=='netstandard'">
    <SomeProperty>Some value that applies to all versions of .NET Standard<SomeProperty>
  </PropertyGroup>

  <PropertyGroup Condition="$(TargetFramework::Name)>='netcoreapp2.0'">
    <SomeProperty>Some value that applies to .NET Core 2.0 and later<SomeProperty>
  </PropertyGroup>

  <PropertyGroup Condition="$(TargetFramework::Name)=='net5.0-ios13.0'">
    <SomeProperty>Some value that only applies to .NET 5 + iOS 13.0<SomeProperty>
  </PropertyGroup>

  <PropertyGroup Condition="$(TargetFramework::Platform)=='windows'">
    <SomeProperty>Some value that applies to all version of Windows<SomeProperty>
  </PropertyGroup>

  <PropertyGroup Condition="$(TargetFramework::Platform)>='ios-12.0'">
    <SomeProperty>Some value that applies to iOS 12.0 and later<SomeProperty>
  </PropertyGroup>

</Project>

Option via functions

We could also just define new intrinsic functions on some type, but this will make using them a mouthful:

  <PropertyGroup Condition="`'$([MSBuild]::TargetFrameworkIdentifier($(TargetFramework)))' == '.NETStandard'`">
    <SomeProperty>Some value that applies to all versions of .NET Standard<SomeProperty>
  </PropertyGroup>

  <PropertyGroup Condition="`'$([MSBuild]::IsTargetFrameworkOrLater($(TargetFramework)))', 'net5.0'))`">
    <SomeProperty>Some value that applies to .NET 5 or later<SomeProperty>
  </PropertyGroup>

  <PropertyGroup Condition="`'$([MSBuild]::IsTargetPlatformOrLater($(TargetFramework)))', 'ios12.0'))`">
    <SomeProperty>Some value that applies to iOS 12 or later<SomeProperty>
  </PropertyGroup>

I am not married to any of these ideas; I'm just spitballing here. Thoughts?

terrajobst commented 4 years ago

To summarize some of the concerns:

@rainersigwald would something like this fly?

<PropertyGroup Condition="$IsCompatibleWith($(TargetFramework), 'ios12.0')">
    <SomeProperty>Some value that applies to iOS 12 or later<SomeProperty>
</PropertyGroup>

Basically, could we allow top-level functions?

rainersigwald commented 4 years ago

Yes, I think we could do

<PropertyGroup Condition="IsCompatibleWith($(TargetFramework), 'ios12.0')">
    <SomeProperty>Some value that applies to iOS 12 or later<SomeProperty>
</PropertyGroup>

which differs from yours only in the initial $.

Right now we have only Exists but it's extensible

https://github.com/microsoft/msbuild/blob/06567a7988c47e0ffe1ae5ad8831b7dd783a79e0/src/Deprecated/Engine/Conditionals/FunctionCallExpressionNode.cs#L36-L41

terrajobst commented 4 years ago

I dig it!

@dsplaisted @davkean @onovotny

clairernovotny commented 4 years ago

I would add RuntimeIdentifier as a thing that people may need to build over and condition over, especially things like P/Invokes. For that, you may want to add to the DefineConstants property and that'd hit the same issue as above.

terrajobst commented 4 years ago

What about these functions then?

Function Result
GetTargetFrameworkIdentifier('net45') '.NETFramework'
GetTargetFrameworkVersion('net45') '4.5'
GetTargetPlatformIdentifier('net45') ''
GetTargetPlatformIdentifier('net5.0-iso12.0') 'ios'
GetTargetPlatformVersion('net5.0-iso12.0') '12.0'
IsTargetFrameworkCompatible('net45', 'net46') True
IsTargetFrameworkCompatible('net46', 'net45') False
IsTargetFrameworkCompatible('net5.0', 'net5.0-ios') True
IsTargetFrameworkCompatible('net5.0-ios12.0', 'net5.0-ios11.0') False
IsTargetPlatformCompatible('ios', 'net5.0-ios11.0') True
IsTargetPlatformCompatible('ios', 'ios11.0') True
IsTargetPlatformCompatible('ios12.0', 'ios11.0') False

@onovotny

I would add RuntimeIdentifier as a thing that people may need to build over and condition over, especially things like P/Invokes. For that, you may want to add to the DefineConstants property and that'd hit the same issue as above.

You mean as a function?

filipnavara commented 4 years ago

I am a little bit concerned by how many components are currently being forced to understand the new format (MSBuild, SDK, NuGet).

Presumably you can calculate the properties from each identifier in TargetFrameworks before you dispatch to the inner build. On the other hand that would not solve the problem for projects that specify TargetFramework directly.

Just thinking aloud I guess. From user perspective the proposed functions above would likely work.

One thing that MSBuild.Sdk.Extras does is an ability to dispatch to inner builds based on RuntimeIdentifier. Naturally you want to make conditions on the inner build based on the RID (eg. "RID is Windows" for "rid == win-x64"). I suppose that is what @onovotny wanted to point out above.

Nirmal4G commented 4 years ago

Presumably you can calculate the properties from each identifier in TargetFrameworks before you dispatch to the inner build.

We can do that after dispatching it to the inner build too. Since, the property TargetFramework becomes global and is available in the beginning of the evaluation.

On the other hand that would not solve the problem for projects that specify TargetFramework directly.

There is no need for doing these comparisons (in the user side) if only TargetFramework is used. But custom functions could be useful in general if we're authoring MSBuild files.

terrajobst commented 4 years ago

@nirmal4g

On the other hand that would not solve the problem for projects that specify TargetFramework directly.

There is no need for doing these comparisons (in the user side) if only TargetFramework is used. But custom functions could be useful in general if we're authoring MSBuild files.

While that's not totally unreasonable, it makes authoring props and targets harder because it bifurcates the world. Ideally, you should be able to write conditions that work regardless of whether the project is a single target build or or multi target build.

terrajobst commented 4 years ago

@filipnavara

I am a little bit concerned by how many components are currently being forced to understand the new format (MSBuild, SDK, NuGet).

Note that this problem already exist today. The properties TargetFramework, TargetFrameworkIdentifier, TargetFrameworkVersion, TargetFrameworkProfile, TargetPlatformIdentifier, TargetPlatformVersion all exist, and with it the parsing problems.

filipnavara commented 4 years ago

@terrajobst I do understand the properties exist today. The parsing currently happens in SDK and NuGet code as far as I can tell. This would introduce a third place with the parsing logic. Please correct me if I am wrong. I am just concerned that three places with the parsing logic starts to be too much to maintain, especially if you consider that not all these projects are necessarily distributed as single cohesive package today. Visual Studio was still using the NetFX version of MSBuild by the time .NET Core 3 was developed which resulted in incompatible behavior between builds in VS and builds in dotnet build. Similarly, NuGet has different release and build propagation cycle from the rest of dotnet today.

ghogen commented 4 years ago

What about version ranges - for the docs platform, we have a concept of range, so instead of combining clauses, a range can be parsed ">= net45" ">= net45 && < net5" "net45" are all examples of ranges - a single IsInRange(…) function handles all.

Nirmal4G commented 4 years ago

@terrajobst

Ideally, you should be able to write conditions that work regardless of whether the project is a single target build or or multi target build.

I agree but this approach seems way into the .NET world. We have C++ and other project types too. Just like Tasks we could add custom functions as an MSBuild concept. Then, we can add our custom functions to .NET SDK rather than in MSBuild itself.

MSBuild is an orchestration engine and should always be that and no more.

rainersigwald commented 4 years ago

@filipnavara

This would introduce a third place with the parsing logic.

I wouldn't want to do this any way other than making the MSBuild functionality a thin wrapper over NuGet functionality. As you say, it's no good if the implementations diverge.

Visual Studio was still using the NetFX version of MSBuild by the time .NET Core 3 was developed which resulted in incompatible behavior between builds in VS and builds in dotnet build.

Can you give an example? We work pretty hard to make sure MSBuild, NuGet, and SDK versions match across matching versions of the products (for example, VS 16.3 and SDK 3.0.100).

filipnavara commented 4 years ago

Visual Studio was still using the NetFX version of MSBuild by the time .NET Core 3 was developed which resulted in incompatible behavior between builds in VS and builds in dotnet build.

Can you give an example? We work pretty hard to make sure MSBuild, NuGet, and SDK versions match across matching versions of the products (for example, VS 16.3 and SDK 3.0.100).

I am not necessarily saying it is still happening. We were consuming .NET Core 3.0 since the very first previews. When building from VS it was using MSBuild that supported binary resources with the old code that does de/serialization. Meanwhile dotnet build supported only string resources, the support for pre-serialized resources was added in one of the very last previews (preview 8 or 9 iirc). It was likely more or less the same MSBuild version but it was compiled in different configuration against different runtime and behaved differently.

dsplaisted commented 4 years ago

With something like IsTargetFrameworkCompatible, I don't think I'll ever remember which way the comparison goes. Just like with IsAssignableFrom. I'm not sure how to improve this though.

As for duplicated logic, I think this proposal will actually allow us to eliminate the duplication. The MSBuild intrinsics would be implemented by calling APIs from NuGet. Then we'd update the logic in the SDK which does its own TargetFramework parsing during evaluation to be implemented in terms of the MSBuild intrinsics.

@rainersigwald @nkolev92 Are there still concerns with MSBuild depending on and loading NuGet assemblies? I'm looking at the code and seeing comments like this: https://github.com/NuGet/NuGet.Client/blob/1c2681b16a0bb9be9271abe043a1cbf892761ef8/src/NuGet.Core/Microsoft.Build.NuGetSdkResolver/NuGetSdkResolver.cs#L23-L26

As well as this: https://github.com/microsoft/msbuild/blob/2d82e1a861d890fce68c8e2d42b569e5bbaf5687/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs#L122-L126

rainersigwald commented 4 years ago

Are there still concerns with MSBuild depending on and loading NuGet assemblies?

Yes, but I think they're resolvable:

  1. This will move the NuGet assembly DLL load into evaluation rather than execution, which will probably be flagged in VS perf systems. But I think it's justifiable for the new behavior.
  2. We've been continuously if sporadically moving in the direction that "NuGet is required for MSBuild". If we introduce an explicit reference and promote it through for example the Runtime package, I think we can just go with it. MSBuild repackagers might have to change to enable the new property functions. But they already couldn't build SDK projects, even for design-time builds, because those need NuGet too. We could probably apply a special-case error to the property function too (haven't thought it all the way through).
  3. I'm worried about dependency flow and introducing a new point of incoherency (MSBuild's NuGet reference not matching SDK/installer). But we have that in various places and can manage it.
Nirmal4G commented 4 years ago

@rainersigwald @nkolev92

All these are .NET related, not just NuGet specific, so why not under [DotNet]::. It is easy to understand that way!

Putting it under MSBuild, atleast for me, is not clear. Also once it's shipped, we can never change it, right? So, shouldn't these kind of decisions be made early on?

Also if there's a potential to have custom property functions in the future. Isn't it better to have an MSBuild language feature, so that internal/3rd party can provide custom functions along with the SDK package instead?

rainersigwald commented 4 years ago

so why not under [DotNet]::

That's a new concept that does not appear anywhere else. NuGet is the layer of the .NET stack where TargetFramework compatibility is defined.

Putting it under MSBuild, atleast for me, is not clear.

It is consistent with all the other property functions that expose non-BCL behavior. I think that's a strong argument in its favor.

custom property functions

This is not likely to happen:

  1. There's no obviously-correct design to make the functions available for binding.
  2. It would expand the ability to do hard-to-predict/hard-to-control things at evaluation time.
Nirmal4G commented 4 years ago

It is consistent with all the other property functions that expose non-BCL behavior.

Those existing properties can be used in C++ and other custom project types. These are .NET specific and have no meaning in other project types. Isn't it better to classify them as such. The only reason I'm insisting on this because as you've said it yourself, We can't change once this ships, EVER!!

I think that's a strong argument in its favor.

You're thinking in terms of System.* as BCL and MSBuild.* as non-BCL, I get that. I was going for the same thing but MSBuild.* is non-BCL but common to all project types and then...

This is where it gets interesting, when more and more people embrace MSBuild (Unless you don't want them to) they could have their own project system with custom functions similar to what .NET is doing here. When and If C++ projects move to Sdk style, they could have custom functions to make project files easier to author.

There's no obviously-correct design to make the functions available for binding.

We could use attributes on a method call to detect the custom functions in the referenced lib. On top of my head, this is what I came up with...

With similar to existing task-declaration

<Project>

  <!-- Example Task declaration -->
  <UsingTask Name="Micosoft.Build.Tasks.MSBuild" Assembly="Microsoft.Build.Tasks.dll" />
  <UsingTask Namespace="Micosoft.Build.Tasks" Assembly="Microsoft.Build.Tasks.dll" /> <!-- NEW: Import all tasks under this namespace -->

  <!-- Similar to UsingTask -->
  <UsingFunction Alias="MSBuild.GetVsInstallRoot()" Name="VisualStudio.Build.Extensions.MSBuildFunctions.GetInstalledPath()" Assembly="Microsoft.VisualStudio.Build.Extensions.dll" />

  <!-- [DotNet]:: -->
  <UsingFunction Alias="DotNet" Class="Microsoft.NET.Build.Extensions.MSBuildFunctions" Assembly="Microsoft.NET.Build.Extensions.dll" />

  <!-- [MSBuild]:: -->
  <UsingFunction Alias="MSBuild" Class="Microsoft.Build.Evaluation.IntrinsicFunctions" Assembly="Microsoft.Build.dll" />

  <!-- [SampleLib.MyClass]:: -->
  <UsingFunction Class="SampleLib.MyClass" Assembly="MSBuild.SampleLib.dll" />

<Project>
using MSBuild.Framework;

namespace SampleLib
{
  [IntrinsicFunctionContainer] // or something similar but you get the idea!
  public static class MyClass
  {
  }
}

It would expand the ability to do hard-to-predict/hard-to-control things at evaluation time.

Tasks and Sdks are in the same boat. Yet, we did make them work!

sfoslund commented 4 years ago

Fixed in #5234 and #5429

KirillOsenkov commented 4 years ago

Is the final design documented anywhere?

sfoslund commented 4 years ago

@KirillOsenkov not formally, but we essentially went with this comment.

clairernovotny commented 4 years ago

@sfoslund is there a doc bug/issue tracking the need to get this documented somewhere?

sfoslund commented 4 years ago

@clairernovotny issue is here: MicrosoftDocs/visualstudio-docs#5599

ViktorHofer commented 3 years ago

Sorry for replying to a closed thread but I'm wondering if the following behavior is right:

  <Target Name="X">
    <Message Importance="high" Text="net6.0 &lt;-- net5.0: $([MSBuild]::IsTargetFrameworkCompatible('net6.0', 'net5.0'))" />
    <Message Importance="high" Text="net5.0 &lt;-- net472: $([MSBuild]::IsTargetFrameworkCompatible('net5.0', 'net472'))" />
    <Message Importance="high" Text="net5.0 &lt;-- netstandard2.0: $([MSBuild]::IsTargetFrameworkCompatible('net5.0', 'netstandard2.0'))" />
    <Message Importance="high" Text="netstandard2.0 &lt;-- net5.0: $([MSBuild]::IsTargetFrameworkCompatible('netstandard2.0', 'net5.0'))" />
  </Target>

Output:

  net6.0 <-- net5.0: True
  net5.0 <-- net472: False
  net5.0 <-- netstandard2.0: True
  netstandard2.0 <-- net5.0: False

In the last case for example, shouldn't this be true?

ViktorHofer commented 3 years ago

Maybe I'm getting something wrong. What's the meaning of target and candidate of the passed in arguments of $([MSBuild]::IsTargetFrameworkCompatible(target, candidate))?

ViktorHofer commented 3 years ago

Ah I see, it's the other way around. Target can reference candidate:

  <Target Name="X">
    <Message Importance="high" Text="net6.0 --&gt; net5.0: $([MSBuild]::IsTargetFrameworkCompatible('net6.0', 'net5.0'))" />
    <Message Importance="high" Text="net6.0 --&gt; net6.0: $([MSBuild]::IsTargetFrameworkCompatible('net6.0', 'net6.0'))" />
    <Message Importance="high" Text="net6.0 --&gt; netcoreapp2.1: $([MSBuild]::IsTargetFrameworkCompatible('net6.0', 'netcoreapp2.1'))" />
    <Message Importance="high" Text="netcoreapp2.1 --&gt; net6.0: $([MSBuild]::IsTargetFrameworkCompatible('netcoreapp2.1', 'net6.0'))" />
    <Message Importance="high" Text="net5.0 --&gt; net472: $([MSBuild]::IsTargetFrameworkCompatible('net5.0', 'net472'))" />
    <Message Importance="high" Text="net472 --&gt; net5.0: $([MSBuild]::IsTargetFrameworkCompatible('net472', 'net5.0'))" />
    <Message Importance="high" Text="net472 --&gt; netstandard2.0: $([MSBuild]::IsTargetFrameworkCompatible('net472', 'netstandard2.0'))" />
    <Message Importance="high" Text="net5.0 --&gt; netstandard2.0: $([MSBuild]::IsTargetFrameworkCompatible('net5.0', 'netstandard2.0'))" />
    <Message Importance="high" Text="netstandard2.0 --&gt; net5.0: $([MSBuild]::IsTargetFrameworkCompatible('netstandard2.0', 'net5.0'))" />
    <Message Importance="high" Text="netstandard2.0 --&gt; net472: $([MSBuild]::IsTargetFrameworkCompatible('netstandard2.0', 'net472'))" />
  </Target>

Output:

--> means references

  net6.0 --> net5.0: True
  net5.0 --> net472: False
  net6.0 --> netcoreapp2.1: True
  netcoreapp2.1 --> net6.0: False
  net472 --> net5.0: False
  net472 --> netstandard2.0: True
  net5.0 --> netstandard2.0: True
  netstandard2.0 --> net5.0: False
  netstandard2.0 --> net472: False
terrajobst commented 3 years ago

Maybe I'm getting something wrong. What's the meaning of target and candidate of the passed in arguments of $([MSBuild]::IsTargetFrameworkCompatible(target, candidate))?

Yeah, this was brough up before. Sadly, for a two argument function there isn't a canonical way to indicate who can reference whom...

ViktorHofer commented 3 years ago

Documentation that shows up when googling the function's name would already be sufficient :) Do we have a tracking issue for the documentation part?

sfoslund commented 3 years ago

Yes, documentation will be released along with 16.8, see this comment: https://github.com/dotnet/msbuild/issues/5171#issuecomment-659492775