Add filtering to SdkSupportedTargetPlatformVersion validation. Fixes #38016. #38017

rolfbjarne · 2024-01-15T16:25:48Z

The SdkSupportedTargetPlatformVersion item group is used for (at least) two things:

Generate the _OR_GREATER preprocessing symbols:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets

Lines 230 to 237 in bfd2919

    
           <!-- Add OR_GREATER compilation symbols for target platforms. --> 
        
           <Target Name="GeneratePlatformCompatibleDefineConstants" 
        
                   Condition=" '$(DisableImplicitFrameworkDefines)' != 'true' and '$(TargetPlatformIdentifier)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)) " > 
        
             <ItemGroup> 
        
               <_SupportedPlatformCompatibleVersions Include="@(SdkSupportedTargetPlatformVersion)" Condition=" %(Identity) != '' and $([MSBuild]::VersionLessThanOrEquals(%(Identity), $(TargetPlatformVersion))) " /> 
        
               <_ImplicitDefineConstant Include="@(_SupportedPlatformCompatibleVersions->Distinct()->'$(TargetPlatformIdentifier.ToUpper())%(Identity)_OR_GREATER'->Replace('.', '_'))" /> 
        
             </ItemGroup> 
        
           </Target>

Validate the TargetPlatformVersion:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets

Lines 233 to 246 in bfd2919

    
           <Target Name="_CheckForInvalidTargetPlatformVersion" 
        
               BeforeTargets="_CheckForInvalidConfigurationAndPlatform" 
        
               DependsOnTargets="_NormalizeTargetPlatformVersion" 
        
               Condition="'$(TargetPlatformVersion)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)) and ('$(Language)' != 'C++' or '$(_EnablePackageReferencesInVCProjects)' == 'true')"> 
        
             <PropertyGroup> 
        
               <TargetPlatformVersionSupported Condition="'$(TargetPlatformVersionSupported)' == '' and '@(_ValidTargetPlatformVersion)' != ''" >true</TargetPlatformVersionSupported> 
        
               <_ValidTargetPlatformVersions Condition="'@(SdkSupportedTargetPlatformVersion)' != ''" >@(SdkSupportedTargetPlatformVersion, '%0a')</_ValidTargetPlatformVersions> 
        
               <_ValidTargetPlatformVersions Condition="'@(SdkSupportedTargetPlatformVersion)' == ''" >None</_ValidTargetPlatformVersions> 
        
             </PropertyGroup> 
        
             <NetSdkError Condition="'$(TargetPlatformVersionSupported)' != 'true'" 
        
                          ResourceName="InvalidTargetPlatformVersion" 
        
                          FormatArguments="$(TargetPlatformVersion);$(TargetPlatformIdentifier);$(_ValidTargetPlatformVersions)" /> 
        
           </Target>

The problem is that these two uses aren't equivalent.

Take for example the following scenario:

We release bindings for iOS 10, and a library developer takes advantage of the
bindings for the new iOS version, while at the same time supporting
multi-targeting to older platforms:

    #if IOS10_0_OR_GREATER
        UseNewApi ();
    #else
        UseOldApi ();
    #endif

Time passes, iOS 11 comes out, and we stop shipping bindings specifically for
iOS 10 (the APIs themselves would be included in the bindings for iOS 11). The
code above should continue to work, but iOS 10 is not a valid
TargetPlatformVersion anymore. However, with the current situation there's no
way to express this, because the moment we remove the "10.0" version from
SdkSupportedTargetPlatformVersion, the IOS10_0_OR_GREATER define isn't
generated anymore.

We discussed this in a meeting internally, and the suggestion that came up
was to use metadata to handle this situation.

So to solve this, I've added metadata to the SdkSupportedTargetPlatformVersion
item group to indicate that a particular value is only valid to produce the
_OR_GREATER processing symbol:

<ItemGroup>
    <SdkSupportedTargetPlatformVersion Include="10.0" OrGreaterHistoricalValue="true" />
    <SdkSupportedTargetPlatformVersion Include="12.0" />
</ItemGroup>

And then filter out these historical values when validating the
TargetPlatformVersion.

Fixes #38016.

…otnet#38016. The SdkSupportedTargetPlatformVersion item group is used for (at least) two things: 1. Generate the _OR_GREATER preprocessing symbols: https://github.com/dotnet/sdk/blob/bfd2919bc446cad68d11de90cec4025d3683591c/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets#L230-L237 2. Validate the TargetPlatformVersion: https://github.com/dotnet/sdk/blob/bfd2919bc446cad68d11de90cec4025d3683591c/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets#L233-L246 The problem is that these two uses aren't equivalent. Take for example the following scenario: We release bindings for iOS 10, and a library developer takes advantage of the bindings for the new iOS version, while at the same time supporting multi-targeting to older platforms: ```csharp #if IOS10_0_OR_GREATER UseNewApi (); #else UseOldApi (); #endif ``` Time passes, iOS 11 comes out, and we stop shipping bindings specifically for iOS 10 (the APIs themselves would be included in the bindings for iOS 11). The code above should continue to work, but iOS 10 is not a valid TargetPlatformVersion anymore. However, with the current situation there's no way to express this, because the moment we remove the "10.0" version from SdkSupportedTargetPlatformVersion, the IOS10_0_OR_GREATER define isn't generated anymore. We discussed this in a meeting internally, and the suggestion that came up was to use metadata to handle this situation. So to solve this, I've added metadata to the SdkSupportedTargetPlatformVersion item group to indicate that a particular value is only valid to produce the _OR_GREATER processing symbol: ```xml <ItemGroup> <SdkSupportedTargetPlatformVersion Include="10.0" OrGreaterHistoricalValue="true" /> <SdkSupportedTargetPlatformVersion Include="12.0" /> </ItemGroup> ``` And then filter out these historical values when validating the TargetPlatformVersion. Fixes dotnet#38016.

rolfbjarne · 2024-01-15T16:26:41Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets

    Condition="'$(TargetPlatformVersion)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)) and ('$(Language)' != 'C++' or '$(_EnablePackageReferencesInVCProjects)' == 'true')">
    <ItemGroup>
-      <_ValidTargetPlatformVersion Include="@(SdkSupportedTargetPlatformVersion)" Condition="'@(SdkSupportedTargetPlatformVersion)' != '' and $([MSBuild]::VersionEquals(%(Identity), $(TargetPlatformVersion)))" />
+      <_ApplicableTargetPlatformVersion Include="@(SdkSupportedTargetPlatformVersion)" Condition="'@(SdkSupportedTargetPlatformVersion)' != '' and '%(SdkSupportedTargetPlatformVersion.OrGreaterHistoricalValue)' != 'true'" RemoveMetadata="OrGreaterHistoricalValue" />


Suggestions welcome for a better name than OrGreaterHistoricalValue!

If these are really only for $(DefineConstants), maybe the name should be something like %(DefineConstantsOnly)?

This is a continuation of the problem here: dotnet/android#8569 (comment)

And using %(MaximumNETVersion) or %(MinimumNETVersion) wouldn't be sufficient?

/cc @dsplaisted

And using %(MaximumNETVersion) or %(MinimumNETVersion) wouldn't be sufficient?

We might want to have strange combination of valid values SdkSupportedTargetPlatformVersion. For a given iOS release, these might be the versions we'd want to support (all in .NET 8):

iOS 17.0: 17.0
iOS 17.1: 17.0 + 17.1
iOS 17.2: 17.0 + 17.2 (but not 17.1)

and the .NET version range check becomes weird in that case (we'd have to come up with something non-intuitive to exclude 17.1).

I think I like the name DefineConstantsOnly a lot better than OrGreaterHistoricalValue.

dsplaisted

Looks good. Can we switch to DefineConstantsOnly for the metadata name (or something else, if anyone has a better idea).

dsplaisted · 2024-01-18T23:06:41Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs

+                .And
+                .HaveStdOutContaining("NETSDK1140")
+                .And
+                .HaveStdOutContaining("111.0 is not a valid TargetPlatformVersion")


We try to avoid hard-coding expected strings from resources in tests, as that won't work if we are trying to test under localization. The tests should have access to the resources used in the messages, so you should be able to get the original resource string and format it to get the expected string.

dsplaisted · 2024-01-18T23:07:39Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets

    Condition="'$(TargetPlatformVersion)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)) and ('$(Language)' != 'C++' or '$(_EnablePackageReferencesInVCProjects)' == 'true')">
    <ItemGroup>
-      <_ValidTargetPlatformVersion Include="@(SdkSupportedTargetPlatformVersion)" Condition="'@(SdkSupportedTargetPlatformVersion)' != '' and $([MSBuild]::VersionEquals(%(Identity), $(TargetPlatformVersion)))" />
+      <_ApplicableTargetPlatformVersion Include="@(SdkSupportedTargetPlatformVersion)" Condition="'@(SdkSupportedTargetPlatformVersion)' != '' and '%(SdkSupportedTargetPlatformVersion.OrGreaterHistoricalValue)' != 'true'" RemoveMetadata="OrGreaterHistoricalValue" />


I think I like the name DefineConstantsOnly a lot better than OrGreaterHistoricalValue.

rolfbjarne · 2024-01-22T21:06:29Z

@dsplaisted I've updated the PR according to your comments.

dsplaisted

Looks good. Feel free to merge when ready, or let me know if you need me to click the button.

dsplaisted · 2024-01-22T21:17:50Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs

+                .And
+                .HaveStdOutContaining("NETSDK1140")
+                .And
+                .HaveStdOutContaining("111.0 is not a valid TargetPlatformVersion")


It would be good to make the string formatting change from the previous test here too.

Fixed, forgot I did the same thing twice.

ghost added Area-Infrastructure untriaged Request triage from a team member labels Jan 15, 2024

rolfbjarne commented Jan 15, 2024

View reviewed changes

dsplaisted approved these changes Jan 18, 2024

View reviewed changes

rolfbjarne added 2 commits January 22, 2024 22:03

Change metadata name according to reviews.

89d1a3e

Update tests to not hardcode translated string.

ffd47f8

jonathanpeppers approved these changes Jan 22, 2024

View reviewed changes

dsplaisted approved these changes Jan 22, 2024

View reviewed changes

Missed one case.

674e506

rolfbjarne merged commit 25b360d into dotnet:main Jan 23, 2024

rolfbjarne deleted the filter-tpv-validation branch January 23, 2024 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add filtering to SdkSupportedTargetPlatformVersion validation. Fixes #38016. #38017

Add filtering to SdkSupportedTargetPlatformVersion validation. Fixes #38016. #38017

Uh oh!

rolfbjarne commented Jan 15, 2024

Uh oh!

rolfbjarne Jan 15, 2024

Uh oh!

jonathanpeppers Jan 16, 2024

Uh oh!

rolfbjarne Jan 16, 2024

Uh oh!

dsplaisted Jan 18, 2024

Uh oh!

dsplaisted left a comment

Uh oh!

dsplaisted Jan 18, 2024

Uh oh!

dsplaisted Jan 18, 2024

Uh oh!

rolfbjarne commented Jan 22, 2024

Uh oh!

dsplaisted left a comment

Uh oh!

dsplaisted Jan 22, 2024

Uh oh!

rolfbjarne Jan 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	<!-- Add OR_GREATER compilation symbols for target platforms. -->
	<Target Name="GeneratePlatformCompatibleDefineConstants"
	Condition=" '$(DisableImplicitFrameworkDefines)' != 'true' and '$(TargetPlatformIdentifier)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)) " >
	<ItemGroup>
	<_SupportedPlatformCompatibleVersions Include="@(SdkSupportedTargetPlatformVersion)" Condition=" %(Identity) != '' and $([MSBuild]::VersionLessThanOrEquals(%(Identity), $(TargetPlatformVersion))) " />
	<_ImplicitDefineConstant Include="@(_SupportedPlatformCompatibleVersions->Distinct()->'$(TargetPlatformIdentifier.ToUpper())%(Identity)_OR_GREATER'->Replace('.', '_'))" />
	</ItemGroup>
	</Target>

	<Target Name="_CheckForInvalidTargetPlatformVersion"
	BeforeTargets="_CheckForInvalidConfigurationAndPlatform"
	DependsOnTargets="_NormalizeTargetPlatformVersion"
	Condition="'$(TargetPlatformVersion)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)) and ('$(Language)' != 'C++' or '$(_EnablePackageReferencesInVCProjects)' == 'true')">
	<PropertyGroup>
	<TargetPlatformVersionSupported Condition="'$(TargetPlatformVersionSupported)' == '' and '@(_ValidTargetPlatformVersion)' != ''" >true</TargetPlatformVersionSupported>
	<_ValidTargetPlatformVersions Condition="'@(SdkSupportedTargetPlatformVersion)' != ''" >@(SdkSupportedTargetPlatformVersion, '%0a')</_ValidTargetPlatformVersions>
	<_ValidTargetPlatformVersions Condition="'@(SdkSupportedTargetPlatformVersion)' == ''" >None</_ValidTargetPlatformVersions>
	</PropertyGroup>

	<NetSdkError Condition="'$(TargetPlatformVersionSupported)' != 'true'"
	ResourceName="InvalidTargetPlatformVersion"
	FormatArguments="$(TargetPlatformVersion);$(TargetPlatformIdentifier);$(_ValidTargetPlatformVersions)" />
	</Target>

Add filtering to SdkSupportedTargetPlatformVersion validation. Fixes #38016. #38017

Add filtering to SdkSupportedTargetPlatformVersion validation. Fixes #38016. #38017

Uh oh!

Conversation

rolfbjarne commented Jan 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rolfbjarne commented Jan 22, 2024

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants