-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add CsWinRT projection producing project for deployment API #4949
Conversation
src/AppInstallerCLI.sln
Outdated
{1F56BECB-D65D-4BBA-8788-6671B251392A}.Release|ARM64.ActiveCfg = Release|Any CPU | ||
{1F56BECB-D65D-4BBA-8788-6671B251392A}.Release|ARM64.Build.0 = Release|Any CPU | ||
{1F56BECB-D65D-4BBA-8788-6671B251392A}.Release|x64.ActiveCfg = Release|Any CPU | ||
{1F56BECB-D65D-4BBA-8788-6671B251392A}.Release|x64.Build.0 = Release|Any CPU | ||
{1F56BECB-D65D-4BBA-8788-6671B251392A}.Release|x86.ActiveCfg = Release|Any CPU | ||
{1F56BECB-D65D-4BBA-8788-6671B251392A}.Release|x86.Build.0 = Release|Any CPU | ||
{1F56BECB-D65D-4BBA-8788-6671B251392A}.ReleaseStatic|ARM64.ActiveCfg = Release|Any CPU |
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.
Not from this change but shouldn't this be also ReleaseStatic
? Looks like it is for Microsoft.WinGet.Client.Engine, which isn't actually built for ARM64, but the inconsistency just jumped out at me...
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.
Nice!! Left a few notes 🙂
Forcing this version to continue using an older CsWinRT release while issues are resolved. | ||
!!! Remove this on the next Microsoft.Windows.CsWinRT package version update. !!! | ||
--> | ||
<WindowsSdkPackageVersion>10.0.22000.34</WindowsSdkPackageVersion> |
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 should be .53 for .NET 8, and .52 for .NET 6
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.
Good reminder that there is a different number for 6 vs 8. But we had to use an older one due to a bug; I can see if that bug has been fixed since.
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 can't use .34 anyway because it's not AOT compatible 😅
I suspect whatever that was, it should be fine on the latest versions.
<ImplicitUsings>enable</ImplicitUsings> | ||
<Nullable>enable</Nullable> | ||
<OutputPath>$(SolutionDir)$(Platform)\$(Configuration)\$(MSBuildProjectName)\</OutputPath> | ||
<Configurations>Debug;Release;ReleaseStatic</Configurations> |
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 should also add:
<EmbedUntrackedSources>true</EmbedUntrackedSources>
<ImplicitUsings>enable</ImplicitUsings> | ||
<Nullable>enable</Nullable> | ||
<OutputPath>$(SolutionDir)$(Platform)\$(Configuration)\$(MSBuildProjectName)\</OutputPath> | ||
<Configurations>Debug;Release;ReleaseStatic</Configurations> |
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.
On .NET 8, we should also add:
<IsAotCompatible>true</IsAotCompatible>
<DisableRuntimeMarshalling>true</DisableRuntimeMarshalling>
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.Windows.CsWinRT" Version="2.0.4" /> |
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.
Wrong version:
<PackageReference Include="Microsoft.Windows.CsWinRT" Version="2.0.4" /> | |
<PackageReference Include="Microsoft.Windows.CsWinRT" Version="2.1.6" /> | |
<WindowsSdkPackageVersion>10.0.22000.34</WindowsSdkPackageVersion> | ||
</PropertyGroup> | ||
|
||
<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.
For the .targets in the NuGet package, we should add this:
<WindowsMetadataReference Include="<WINMD_PATH>" IsOutOfProcess="true" />
Where "<WINMD_PATH>" is the right path to the .winmd in the NuGet package.
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFrameworks>net6.0-windows10.0.22000.0;net8.0-windows10.0.22000.0</TargetFrameworks> |
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.
On second thought, I think we should do .NET 8 only.
.NET 6 is literally going EOL next Tuesday 😄
This comment has been minimized.
This comment has been minimized.
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.
Why did you remove WindowsSdkPackageVersion
? We still need it, just set it to:
- .53 for .NET 6
- .54 for .NET 8
Does CsWinRT not choose the appropriate value by default based on its own version and the TFM? |
No, it's the .NET SDK that picks the projection version, not CsWinRT. And we haven't shipped a .NET SDK using the latest projections yet, so we can just manually specify that version for now, and remove it in the future. But really there's no harm hardcoding it in projections projects, as long as you don't forget to go back and remove it or update it later on 🙂 |
This comment has been minimized.
This comment has been minimized.
…dling for 2.1 changes
This comment has been minimized.
This comment has been minimized.
<!-- | ||
!!! Remove or update this on the next Microsoft.Windows.CsWinRT package version update. !!! | ||
--> | ||
<WindowsSdkPackageVersion>10.0.22000.54</WindowsSdkPackageVersion> |
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.
<WindowsSdkPackageVersion>10.0.22000.54</WindowsSdkPackageVersion> | |
<WindowsSdkPackageVersion>10.0.22000.53</WindowsSdkPackageVersion> | |
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.
But why not the version that supports .NET 8 and UWP?
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.
.53 also supports .NET 8. The UWP support is not exclusive to .54 (the phrasing in our release notes isn't entirely accurate). The .54 version just also has the projections for Windows.UI.Xaml.*
types, but we're not using them here, so they're just not needed. Using .53 will still work just fine in UWP apps using .NET 9, don't worry 🙂
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.
LGTM!
I am glad that WinGet's deployment api uses the new CsWinRT, so would you like to ask if the new Projection dll with CsWinRT will be included in the winget nuget distribution in the next release? ========================非常欣慰 WinGet 的 deployment api 使用新的 CsWinRT,所以想问一下,下一个版本会在 winget nuget 发行包中包含新带有 CsWinRT 的 Projection dll 吗? |
Change
Produce a CsWinRT projection for Microsoft.Management.Deployment.
Move to CsWinRT 2.1.6 across the solution.
Microsoft Reviewers: Open in CodeFlow