From 08ff456902e154af980ecc170b9b56c0e7c96645 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Fri, 12 Jan 2024 17:18:46 +0100 Subject: [PATCH 1/4] Add filtering to SdkSupportedTargetPlatformVersion validation. Fixes #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 ``` And then filter out these historical values when validating the TargetPlatformVersion. Fixes https://github.com/dotnet/sdk/issues/38016. --- ...osoft.NET.TargetFrameworkInference.targets | 7 +- ...venThatWeWantToBuildWithATargetPlatform.cs | 77 +++++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets index b39881a1da1b..5f177cd7d669 100644 --- a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets +++ b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets @@ -222,7 +222,8 @@ Copyright (c) .NET Foundation. All rights reserved. BeforeTargets="ProcessFrameworkReferences" Condition="'$(TargetPlatformVersion)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)) and ('$(Language)' != 'C++' or '$(_EnablePackageReferencesInVCProjects)' == 'true')"> - <_ValidTargetPlatformVersion Include="@(SdkSupportedTargetPlatformVersion)" Condition="'@(SdkSupportedTargetPlatformVersion)' != '' and $([MSBuild]::VersionEquals(%(Identity), $(TargetPlatformVersion)))" /> + <_ApplicableTargetPlatformVersion Include="@(SdkSupportedTargetPlatformVersion)" Condition="'@(SdkSupportedTargetPlatformVersion)' != '' and '%(SdkSupportedTargetPlatformVersion.OrGreaterHistoricalValue)' != 'true'" RemoveMetadata="OrGreaterHistoricalValue" /> + <_ValidTargetPlatformVersion Include="@(_ApplicableTargetPlatformVersion)" Condition="'@(_ApplicableTargetPlatformVersion)' != '' and $([MSBuild]::VersionEquals(%(Identity), $(TargetPlatformVersion)))" /> @@ -236,8 +237,8 @@ Copyright (c) .NET Foundation. All rights reserved. Condition="'$(TargetPlatformVersion)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)) and ('$(Language)' != 'C++' or '$(_EnablePackageReferencesInVCProjects)' == 'true')"> true - <_ValidTargetPlatformVersions Condition="'@(SdkSupportedTargetPlatformVersion)' != ''" >@(SdkSupportedTargetPlatformVersion, '%0a') - <_ValidTargetPlatformVersions Condition="'@(SdkSupportedTargetPlatformVersion)' == ''" >None + <_ValidTargetPlatformVersions Condition="'@(_ApplicableTargetPlatformVersion)' != ''" >@(_ApplicableTargetPlatformVersion, '%0a') + <_ValidTargetPlatformVersions Condition="'@(_ApplicableTargetPlatformVersion)' == ''" >None + + + + + + 111.0 + ios + true + + +"; + + File.WriteAllText(Path.Combine(testAsset.TestRoot, "Directory.Build.targets"), DirectoryBuildTargetsContent); + + var buildCommand = new BuildCommand(testAsset); + buildCommand.Execute() + .Should() + .Fail() + .And + .HaveStdOutContaining("NETSDK1140") + .And + .HaveStdOutContaining("111.0 is not a valid TargetPlatformVersion") + .And + .HaveStdOutContaining("222.0"); + } + + [Fact] + public void It_fails_if_targetplatformversion_is_invalid() + { + var testProject = new TestProject() + { + Name = "It_fails_if_targetplatformversion_is_invalid", + TargetFrameworks = ToolsetInfo.CurrentTargetFramework, + }; + var testAsset = _testAssetsManager.CreateTestProject(testProject); + + + string DirectoryBuildTargetsContent = $@" + + + + + + 111.0 + ios + true + + +"; + + File.WriteAllText(Path.Combine(testAsset.TestRoot, "Directory.Build.targets"), DirectoryBuildTargetsContent); + + var buildCommand = new BuildCommand(testAsset); + buildCommand.Execute() + .Should() + .Fail() + .And + .HaveStdOutContaining("NETSDK1140") + .And + .HaveStdOutContaining("111.0 is not a valid TargetPlatformVersion") + .And + .HaveStdOutContaining("222.0"); + } } } From 89d1a3e94a6010eac632fc4500fdec643cf913e9 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Mon, 22 Jan 2024 19:24:39 +0100 Subject: [PATCH 2/4] Change metadata name according to reviews. --- .../targets/Microsoft.NET.TargetFrameworkInference.targets | 2 +- .../GivenThatWeWantToBuildWithATargetPlatform.cs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets index 5f177cd7d669..f07970633e3f 100644 --- a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets +++ b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets @@ -222,7 +222,7 @@ Copyright (c) .NET Foundation. All rights reserved. BeforeTargets="ProcessFrameworkReferences" Condition="'$(TargetPlatformVersion)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)) and ('$(Language)' != 'C++' or '$(_EnablePackageReferencesInVCProjects)' == 'true')"> - <_ApplicableTargetPlatformVersion Include="@(SdkSupportedTargetPlatformVersion)" Condition="'@(SdkSupportedTargetPlatformVersion)' != '' and '%(SdkSupportedTargetPlatformVersion.OrGreaterHistoricalValue)' != 'true'" RemoveMetadata="OrGreaterHistoricalValue" /> + <_ApplicableTargetPlatformVersion Include="@(SdkSupportedTargetPlatformVersion)" Condition="'@(SdkSupportedTargetPlatformVersion)' != '' and '%(SdkSupportedTargetPlatformVersion.DefineConstantsOnly)' != 'true'" RemoveMetadata="DefineConstantsOnly" /> <_ValidTargetPlatformVersion Include="@(_ApplicableTargetPlatformVersion)" Condition="'@(_ApplicableTargetPlatformVersion)' != '' and $([MSBuild]::VersionEquals(%(Identity), $(TargetPlatformVersion)))" /> diff --git a/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs b/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs index 6940298465cc..ba4b1ed47bb3 100644 --- a/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs +++ b/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs @@ -92,11 +92,11 @@ public void It_fails_on_unsupported_os() } [Fact] - public void It_fails_if_targetplatformversion_is_historical() + public void It_fails_if_targetplatformversion_is_constant_only() { var testProject = new TestProject() { - Name = "It_fails_if_targetplatform_version_is_historical", + Name = "It_fails_if_targetplatformversion_is_constant_only", TargetFrameworks = ToolsetInfo.CurrentTargetFramework, }; var testAsset = _testAssetsManager.CreateTestProject(testProject); @@ -105,7 +105,7 @@ public void It_fails_if_targetplatformversion_is_historical() string DirectoryBuildTargetsContent = $@" - + From ffd47f8bf09f92f14097a8e2089539d3f72a852f Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Mon, 22 Jan 2024 19:28:13 +0100 Subject: [PATCH 3/4] Update tests to not hardcode translated string. --- .../GivenThatWeWantToBuildWithATargetPlatform.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs b/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs index ba4b1ed47bb3..3be2e5b6b278 100644 --- a/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs +++ b/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using Microsoft.NET.Build.Tasks; + namespace Microsoft.NET.Build.Tests { public class GivenThatWeWantToBuildWithATargetPlatform : SdkTest @@ -125,7 +127,7 @@ public void It_fails_if_targetplatformversion_is_constant_only() .And .HaveStdOutContaining("NETSDK1140") .And - .HaveStdOutContaining("111.0 is not a valid TargetPlatformVersion") + .HaveStdOutContaining(string.Format(Strings.InvalidTargetPlatformVersion, "111.0", "ios", "222.0").Split ('\n', '\r') [0]) .And .HaveStdOutContaining("222.0"); } From 674e50654b70b8e3f40f28517d525a1a28f38817 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Mon, 22 Jan 2024 22:19:58 +0100 Subject: [PATCH 4/4] Missed one case. --- .../GivenThatWeWantToBuildWithATargetPlatform.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs b/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs index 3be2e5b6b278..0ef98703ac67 100644 --- a/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs +++ b/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildWithATargetPlatform.cs @@ -165,7 +165,7 @@ public void It_fails_if_targetplatformversion_is_invalid() .And .HaveStdOutContaining("NETSDK1140") .And - .HaveStdOutContaining("111.0 is not a valid TargetPlatformVersion") + .HaveStdOutContaining(string.Format(Strings.InvalidTargetPlatformVersion, "111.0", "ios", "222.0").Split ('\n', '\r') [0]) .And .HaveStdOutContaining("222.0"); }