-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Support CsWinRT 3.0 multi-targeting and WindowsSdkPackageMinimumRevision #50264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support CsWinRT 3.0 multi-targeting and WindowsSdkPackageMinimumRevision #50264
Conversation
…iate between CSWinRT 2 and 3
src/Tasks/Microsoft.NET.Build.Tasks/ResolveRuntimePackAssets.cs
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Windows.props
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Windows.targets
Outdated
Show resolved
Hide resolved
test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildAWindowsDesktopProject.cs
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets
Outdated
Show resolved
Hide resolved
<_SupportedPlatformCompatibleVersions Include="@(SdkSupportedTargetPlatformVersion)" | ||
Condition=" %(Identity) != '' and '%(SdkSupportedTargetPlatformVersion.NormalizedSupportedTargetPlatformVersion)' == '' and $([MSBuild]::VersionLessThanOrEquals(%(Identity), $(TargetPlatformVersion))) " /> | ||
<_SupportedPlatformCompatibleVersions Include="@(SdkSupportedTargetPlatformVersion->'%(NormalizedSupportedTargetPlatformVersion)')" | ||
Condition=" '%(SdkSupportedTargetPlatformVersion.NormalizedSupportedTargetPlatformVersion)' != '' and $([MSBuild]::VersionLessThanOrEquals('%(SdkSupportedTargetPlatformVersion.NormalizedSupportedTargetPlatformVersion)', $(TargetPlatformVersion))) " /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this result in duplicate _SupportedPlatformCompatibleVersions
items, since we get one from the .0 SdkSupportedTargetPlatformVersion
and one from the .1
one. If so, should we fix that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When _SupportedPlatformCompatibleVersions
is used right below, it is used as @(_SupportedPlatformCompatibleVersions->Distinct()
so that removes the duplicates.
<PropertyGroup> | ||
<!-- If the Windows targets wasn't called to default it, then just default it to the TargetPlatformVersion. --> | ||
<EffectiveTargetPlatformVersion Condition="'$(EffectiveTargetPlatformVersion)' == ''">$(TargetPlatformVersion)</EffectiveTargetPlatformVersion> | ||
</PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the EffectiveTargetPlatformVersion
is set in Microsoft.NET.TargetFrameworkInference.targets, which is unconditionally imported. So do we need this here? Can we make it so that TargetFrameworkInference always sets this property instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially made Microsoft.NET.TargetFrameworkInference.targets
always default it, but then I ran into issues with Directory.Build.Targets
can set WindowsPlatformVersion
and that was after the TargetFrameworkInference.targets
, so I had to handle that case in Microsoft.Net.Windows.targets
which is based on EffectiveTargetPlatformVersion
not being set. Given the use of Directory.Build.targets
which probably can also be used to set the platform, I put this here to catch such scenarios.
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets
Outdated
Show resolved
Hide resolved
...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.WindowsSdkSupportedTargetPlatforms.props
Outdated
Show resolved
Hide resolved
} | ||
|
||
[WindowsOnlyFact] | ||
public void ItWarnsWhenBuildingAProjectTargetingCsWinRT3_0() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't published a Ref pack with the new CSWinRT profiles yet, so these projects will be referencing a profile which doesn't exist in the ref pack. I would have expected that to error but I guess maybe it silently just doesn't reference any assemblies.
What should the behavior be in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will confirm locally what happens but I would be fine if the behavior is basically no profiles get included and thereby no assemblies referenced or if it errored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like behavior is no profiles get included. It does error due to CsWinRTGen missing which this test doesn't run into due to CsWinRTGenerateInteropAssembly
is set to false. I assume this behavior is fine.
Copilot prompt #1: Replace the #class:'Microsoft.DotNet.Build.Tasks.OverrideAndCreateBundledNETCoreAppPackageVersion':1164-10242 Task. The new version should take as parameters the path to the stage 0 bundled versions file as well as the version from stage 2. It should copy some version numbers from items in the stage 0 version to the stage 2 version, and then save the updated stage 2 version. It should update items where the TargetFramework metadata is not the latest target framework supported. There are multiple item types that need to be updated, and they need to be matched on specific metadata. KnownFrameworkReference: match on Include, TargetFramework metadata. Update LatestRuntimeFrameworkVersion and TargetingPackVersion KnownAppHostPack: match on Include, TargetFramework metadata. Update AppHostPackVersion KnownCrossgen2Pack: match on Include, TargetFramework. Update Crossgen2PackVersion KnownILCompilerPack: match on Include, TargetFramework. Update ILCompilerPackVersion KnownRuntimePack: match on Include, TargetFramework, RuntimePackLabels. Update LatestRuntimeFrameworkVersion KnownILLinkPack: match on Include, TargetFramework. Update ILLinkPackVersion. If there is more than one match based on the specified metadata, generate an error. For the metadata which is not used to match and is not updated, if the value differs between the stage 0 and stage 2 versions, log a message. Copilot prompt dotnet#2: Don't look at just the first ItemGroup in each file, load items from all ItemGroups found. When looking for the latest target framework, use the NuGetFramework class to parse the TargetFramework values, and to compare them.
@marcpopMSFT I've pushed changes to this PR that significantly change OverrideAndCreateBundledNETCoreAppPackageVersion, which you enabled in #50418. The problem with the previous logic is that it basically uses the stage 0 bundled versions for stage 2 with some version numbers updated. This was necessary when the real bundled versions was in the separate installer repo. Now that the repos are merged though, we generate the fully correct bundled versions file in the SDK. If we replace that with the stage 0 version, then when we make changes to what's in the bundled versions file, they won't be available for tests until the updates flow back through stage 0. So what the logic now does is preserve most of what is in the newly built bundled versions file, and only copy over version numbers for downlevel runtimes from stage 0. I think that should solve the issues. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
<FrameworkReference Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '8.0')) and '$(TargetPlatformIdentifier)' == 'Windows' and '$(UseUwp)' == 'true' " Include="Microsoft.Windows.SDK.NET.Ref.Xaml" /> | ||
<ItemGroup Condition=" '$(IncludeWindowsSDKRefFrameworkReferences)' == 'true' And | ||
'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '8.0')) and '$(TargetPlatformIdentifier)' == 'Windows' and '$(UseUwp)' == 'true' "> | ||
<FrameworkReference Condition="'$(_TargetPlatformVersionUsesCsWinRT3)' != 'true'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should one of these be for Microsoft.Windows.SDK.NET.Ref.CsWinRT3.Windows
, or is the UseUwp
only applicable to XAML and this pulls in the 2.0 and 3.0 Xaml reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseUwp is only applicable to the Xaml profile which has the inbox XAML APIs.
using System.Xml.Linq; | ||
using Microsoft.Build.Framework; | ||
using Microsoft.Build.Utilities; | ||
using NuGet.Versioning; | ||
using System.Collections.Generic; | ||
using NuGet.Frameworks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: sort usings
{ | ||
["Program.cs"] = """ | ||
#if CSWINRT3_0 | ||
#error CSWINRT3_0 is defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this type of test catch situations where a package has the wrong minor version, for example, something that should have been versioned with .1 was versioned with .0 so the targets treat it as a 2.0 package, but it's actually a 3.0 package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is more if you are building a project with .0, make sure that none of the CSWINRT_3 things are defined.
This is a continuation of #49721
This PR allows multi-targeting CsWinRT 3.0, by leveraging the build number of the target Windows SDK.
That is, e.g.:
net10.0-windows10.0.22621.0
---> CsWinRT 2.xnet10.0-windows10.0.22621.1
---> CsWinRT 3.0In the implementation, rather than using separate ref packs for CSWinRT 2 and 3, which the previous PR did, this PR uses profiles of the Microsoft.Windows.SDK.NET.Ref framework reference to differentiate between the versions of CSWinRT.
This also adds support for a WindowsSdkPackageMinimumRevision property.