Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Upgrade to Windows SDK 10.0.22000.0 #10869

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Upgrade to Windows SDK 10.0.22000.0 #10869

wants to merge 3 commits into from

Conversation

JunielKatarn
Copy link
Contributor

@JunielKatarn JunielKatarn commented Nov 10, 2022 •
edited

Description

Upgrade to Windows SDK 10.0.22000.0

Type of Change

  • New feature (non-breaking change which adds functionality)

Why

  1. Required for Support for ARM64EC platform in Desktop DLL #10870.
    ARM64EC requires Windows SDK version 10.0.22000.0 to correctly handle intrinsics includes.
    See https://developercommunity.visualstudio.com/t/arm64ec-toolchain-doesnt-work/1460523\

    Windows SDK 10.0.19041.0 does not ship softintrin.h, causing compilation errors on the affected platform.
  2. Visual Studio 2022 defaults to Windows SDK 10.0.22000.0.
    If there is no other specific need to use 10.0.19041.0, the upgrade will also reduce the dev dependency size.

What

Replaced references to Windows SDK version 10.0.19041.0 with version 10.0.22000.0.

Microsoft Reviewers: Open in CodeFlow

@JunielKatarn JunielKatarn requested review from a team as code owners November 10, 2022 02:11
JunielKatarn
JunielKatarn commented Nov 10, 2022
View reviewed changes
packages/@react-native-windows/cli/src/runWindows/utils/build.ts
@@ -24,12 +24,12 @@ export async function buildSolution(
buildLogDirectory?: string,
singleproc?: boolean,
) {
const minVersion = new Version(10, 0, 19041, 0);
const minVersion = new Version(10, 0, 22000, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep 19041 as a minimum version?

JunielKatarn
JunielKatarn commented Nov 10, 2022
View reviewed changes
vnext/Microsoft.ReactNative.Managed.CodeGen.UnitTests/AnalysisTestBase.cs
@@ -90,8 +90,8 @@ protected async Task<(CodeAnalyzer, INamedTypeSymbol)> AnalyzeFileAsync(string c
// of the unitest runners are allowed to pick up a variable from the build file. And given the many
// ways one can run inttests, this seemed to be the most reasonable out of a lot of poor options.
var win10SdkFolder = @"C:\Program Files (x86)\Windows Kits\10";
#if win10SdkVersion10_0_19041_0
var win10SdkVersion = "10.0.19041.0";
#if win10SdkVersion10_0_22000_0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this macro defined automatically?

JunielKatarn
JunielKatarn commented Nov 10, 2022
View reviewed changes
vnext/Scripts/rnw-dependencies.ps1
@@ -61,7 +61,7 @@ $vsComponents = @('Microsoft.Component.MSBuild',
'Microsoft.VisualStudio.Component.VC.Tools.x86.x64',
'Microsoft.VisualStudio.ComponentGroup.UWP.Support',
'Microsoft.VisualStudio.ComponentGroup.NativeDesktop.Core',
'Microsoft.VisualStudio.Component.Windows10SDK.19041');
'Microsoft.VisualStudio.Component.Windows10SDK.22000');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing PR validation.

Do we need to apply an update to the build agent image?

jonthysell
jonthysell requested changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@jonthysell jonthysell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please hold on this, it's not as easy (or desirable) to just do a find-replace.

@ghost ghost added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Nov 10, 2022
@JunielKatarn
Copy link
Contributor Author

Please hold on this, it's not as easy (or desirable) to just do a find-replace.

Sure thing. Reverting to draft for now.

Is there a suggested sequence to carry this type of upgrade?

@ghost ghost removed the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Nov 10, 2022
@JunielKatarn JunielKatarn marked this pull request as draft November 10, 2022 20:40
@jonthysell
Copy link
Contributor

jonthysell commented Nov 14, 2022 •
edited

Please hold on this, it's not as easy (or desirable) to just do a find-replace.

Sure thing. Reverting to draft for now.

Is there a suggested sequence to carry this type of upgrade?

Well, ideally we don't want to upgrade everyone unless there's some clear requirement. Since Desktop needs it, I think it would be easier to simply override the version when building the Desktop and leave UWP to default targeting 19041.

We set the SDK versions in External\Microsoft.ReactNative.WindowsSdk.Default.props, if they're not already set. The Desktop projects import that props sheet via React.cpp.props: https://github.com/microsoft/react-native-windows/blob/main/vnext/PropertySheets/React.Cpp.props#L5

So I think what we want is for the Desktop projects to specify the new SDK version sometime before importing React.Cpp.props? Or maybe specify a different version within Microsoft.ReactNative.WindowsSdk.Default.props if it detects it's a Desktop project doing the import (not sure if there's some property that can be detected).

@JunielKatarn
Copy link
Contributor Author

Well, ideally we don't want to upgrade everyone unless there's some clear requirement. Since Desktop needs it, I think it would be easier to simply override the version when building the Desktop and leave UWP to default targeting 19041.

That is certainly an option.

I figured it would be convenient to have both variants on the same SDK version.
As stated in the PR description, reducing the installation footprint (VS 2022 defaults to 22000 for the UWP workflow) seemed like a good reason.

Given the min SDK version remains unchanged, what's the criteria to upgrade the target SDK version?
If I read you right, do we prefer to never upgrade unless forced to?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonthysell jonthysell jonthysell requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@JunielKatarn @jonthysell