Add proposal for simpler multi-targeting conditions #291
Conversation
| - The property is one of `TargetFrameworkIdentifier`, `TargetFrameworkVersion`, `TargetPlatformIdentifier`, or `TargetPlatformVersion` | ||
| - The current property value is unset / empty | ||
| - The `MSBuildAutomaticallyParseTargetFramework` property is set to `True` | ||
| - Then MSBuild would read the `TargetFramework` property, and if it was non-empty call the appropriate NuGet target framework parsing function to get the value. It would not store the computed value of the property for future reads, simply return the computed value for the current property value read. |
There was a problem hiding this comment.
Tying this to TargetFramework makes sense for .NET SDK projects, but presents a problem for a Directory.Build.* file that is imported into a mixed repo with non-SDK projects.
Unfortunately, $(TargetFrameworkMoniker) is set late (in common.targets) so just considering it as well wouldn't be sufficient for property conditions.
| - The current property value is unset / empty | ||
| - The `MSBuildAutomaticallyParseTargetFramework` property is set to `True` | ||
| - Then MSBuild would read the `TargetFramework` property, and if it was non-empty call the appropriate NuGet target framework parsing function to get the value. It would not store the computed value of the property for future reads, simply return the computed value for the current property value read. | ||
| - The .NET SDK would set the `MSBuildAutomaticallyParseTargetFramework` property to `True` in its `.props` files to enable the new behavior for Sdk-style projects. |
There was a problem hiding this comment.
Would it make sense to add
- If one of the computed properties is set, error if the set value doesn't match the computed value.
? I'd like to be pretty conservative with the magic here.
There was a problem hiding this comment.
This may be tricky, because for target framework that's not .NET 5 and higher, the target platform defaults to Windows 7.0. I'll add it to the design though.
|
|
||
| To complicate matters, the target framework properties *are* set when evaluating conditions on items and `ItemGroup`s. This is because MSBuild evaluation has multiple passes so all properties are evaluated before items, regardless of the relative evaluation position of the properties to the items. This is complicated to explain we don't want normal MSBuild users to have to know about this, so we try to avoid taking advantage of this. Otherwise people would see conditions on items, apply the same pattern to properties, and not understand why it doesn't work. | ||
|
|
||
| So, the currently recommended solution is to use [built-in MSBuild functions](https://learn.microsoft.com/en-us/visualstudio/msbuild/property-functions?view=vs-2022#msbuild-targetframework-and-targetplatform-functions) to parse out the components of the TargetFramework property for comparison: `<SupportedOSPlatformVersion Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'ios'">11.0</SupportedOSPlatformVersion>`. There's also an `IsTargetFrameworkCompatible` function which is useful in some cases. |
There was a problem hiding this comment.
One thing that IsTargetFrameworkCompatible is very useful for is range computations, like "on versions less than X, include this package to provide behavior that is now built in".
A lot of this can probably be covered under the new proposal by '$(TargetFrameworkIdentifier)' == '.NETCoreApp' (or .NETFramework), but not everything.
I can't think of a way to extend this proposal to make those more graceful, but I'd like to mention it in case someone else can.
| <SupportedOSPlatformVersion Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'tizen'">6.5</SupportedOSPlatformVersion> | ||
| ``` | ||
|
|
||
| We propose updating MSBuild to allow the following to work: |
There was a problem hiding this comment.
just throwing this out here, no idea how workable this is but what if we had a new attribute instead that mimics MAUI's OnPlatform XAML helpers: https://learn.microsoft.com/dotnet/maui/xaml/fundamentals/essential-syntax?view=net-maui-7.0#platform-differences
<SupportedOSPlatformVersion OnPlatform="ios">11.0</SupportedOSPlatformVersion>
<SupportedOSPlatformVersion OnPlatform="maccatalyst">13.0</SupportedOSPlatformVersion>
...
There was a problem hiding this comment.
Interesting and similar to @terrajobst's initial proposal during the .NET 5 timeframe: dotnet/msbuild#5171 (see the "Option using attributes" section). Apparently we landed on option using functions and based on this proposal it sounds like we aren't happy with the current UX.
If we would add support for the attributes option, we should make sure that it not just works for TargetPlatformIdentifier but also for
- TargetPlatformVersion
- TargetFramework
- TargetFrameworkIdentifier
- TargetFrameworkVersion
That said, I think such new attributes add complexity and aren't worth the trade-off. I would instead invest in making plain vanilla msbuild conditions work on properties as well (this proposal) so that functions only need to be used in the minority of cases (i.e. IsTargetFrameworkCompatible).
Co-authored-by: Alexander Köplinger <[email protected]> Co-authored-by: Rainer Sigwald <[email protected]>
| - The property to be read is one of `TargetFrameworkIdentifier`, `TargetFrameworkVersion`, `TargetPlatformIdentifier`, or `TargetPlatformVersion` | ||
| - Then, if the current property value is unset / empty | ||
| - MSBuild would read the `TargetFramework` property, and if it was non-empty call the appropriate NuGet target framework parsing function to get the value. | ||
| - It would not store the computed value of the property for future reads, simply return the computed value for the current property value read. |
There was a problem hiding this comment.
Do you know how expensive this could become for projects that have a lot conditions with interfered TFM properties? Asking as larger repositories like dotnet/runtime heavily depend on them and it sounds like we are making the existing path more expensive with this proposal (i.e. in item conditions or in property conditions in targets files).
| - Otherwise, if the property to be read is set | ||
| - MSBuild would still read the `TargetFramework` property and parse the appropriate value. It would compare the parse result with the current value of the property. If they do not match, it would generate an evaluation error | ||
| - This is to ensure that the "magic" properties have a consistent value, whether that value comes from the new automatic parsing logic in MSBuild, the default parsing in the .NET SDK targets, or elsewhere. | ||
| - This may cause errors when [TargetFramework](https://github.com/NuGet/Home/issues/5154) [aliasing](https://github.com/NuGet/Home/pull/12124) is used. If TargetFramework aliasinng is being used and the resultant TargetFramework component properties are different than the result would be from a standard parse of the `TargetFramework`, then the aliasing logic should also turn off `MSBuildAutomaticallyParseTargetFramework`. |
There was a problem hiding this comment.
| - This may cause errors when [TargetFramework](https://github.com/NuGet/Home/issues/5154) [aliasing](https://github.com/NuGet/Home/pull/12124) is used. If TargetFramework aliasinng is being used and the resultant TargetFramework component properties are different than the result would be from a standard parse of the `TargetFramework`, then the aliasing logic should also turn off `MSBuildAutomaticallyParseTargetFramework`. | |
| - This may cause errors when [TargetFramework](https://github.com/NuGet/Home/issues/5154) [aliasing](https://github.com/NuGet/Home/pull/12124) is used. If TargetFramework aliasing is being used and the resultant TargetFramework component properties are different than the result would be from a standard parse of the `TargetFramework`, then the aliasing logic should also turn off `MSBuildAutomaticallyParseTargetFramework`. |
This proposes making conditions for multi-targeted projects simpler. Instead of this:
You would be able to do this:
This should be more concise and understandable
Rendered Proposal